Opened 3 years ago

Last modified 8 months ago

#24554 new enhancement

sched: Have per-scheduler type data in a channel_t

Reported by: dgoulet Owned by:
Priority: Very High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sched, 034-triage-20180328, 034-removed-20180328, tor-chan, network-team-roadmap-2020Q1
Cc: Actual Points:
Parent ID: #23993 Points:
Reviewer: Sponsor:


Right now in channel_t, we have a sched_heap_idx and scheduler_state. Both are specific to the scheduler layer and are per-channel. The KIST scheduler also keeps data per-channel in a global state it keeps some other place.

Because of #23744, which is a ticket about the scheduler state not applying to both Vanilla and KIST, we need a way to have a interface that allows any "scheduler data per-channel" to be specific to a type of scheduler (KIST or Vanilla).

To achieve compartmentalization of whatever state the scheduler needs to keep, we need an interface that allows a specific scheduler to store data per-channel instead of in a global state which, in the case of KIST, would avoid two things:

  1. Huge amount of memory allocation at runtime that needs to be done by the main loop (this is currently a problem with KIST code).
  1. Avoid data duplication or/and synchronization problem between the channel subsystem and the scheduler subsystem.

Finally, by doing so, we can build an opaque interface for the specific scheduler information data structure in order to make them *ONLY* accessible from the per-type scheduler code. This means a builtin protection to prevent any other layer in the tor to change or access the channel's scheduler state.

Once we have this capability, we can go ahead and fix #23744 and move the sched_heap_idx and scheduler_state out of a channel object so ONLY the scheduler has access to those and be able to have different semantic for different scheduler type.

Child Tickets

Change History (18)

comment:1 Changed 3 years ago by dgoulet

Status: assignedneeds_review

See branch: ticket24554_033_01

comment:2 Changed 3 years ago by dgoulet

Parent ID: #23993

comment:3 Changed 3 years ago by pastly

Reviewer: pastly

comment:4 Changed 3 years ago by pastly

Status: needs_reviewneeds_revision

Left comments on oniongit.

  • One easy to fix compile issue.
  • Some rambling thoughts about the consequences of ignoring a chan with invalid sched_info in the middle of a scheduling run.

Marking needs_revision for the compile issue.

comment:5 Changed 3 years ago by dgoulet

pastly did review and I fixed stuff. I'll rebase this on latest master and open a new merge request for upstream merge.

comment:6 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

See branch: ticket24554_033_02.
Oniongit MR:

One thing to notice is that the top commit is actually the commit for #23579. It ain't very complicated, removes code duplication and lays the ground work for future tickets like #23744 and the work to move the scheduler state from the channel_t object into sched_info_t *only* for Vanilla because KIST doesn't need it.

This is a slow and complicated work towards getting the scheduler subsystem much simpler, robust and easier to test. All this is part of #23993 parent ticket.

comment:7 Changed 3 years ago by dgoulet

Reviewer: pastly

(Removing pastly since he did a first review and might not want to do a second ;)

comment:8 Changed 3 years ago by dgoulet

Quick note. Moving sched_heap_idx out of channel_t and into sched_info_t is a bit more difficult because we use this offsetof() strategy with priority list so sched_heap_idx needs to offset from the channel_t object which is what is stored in the list. One possible solution would be to keep a pointer to the channel inside the sched_info_t and store those objects instead.

comment:9 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

I need to run this a bit more on top of #24665 and #24666.

comment:10 Changed 3 years ago by dgoulet

Priority: MediumVery High

comment:11 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

comment:12 Changed 3 years ago by nickm

Keywords: 034-triage-20180328 added

comment:13 Changed 3 years ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:14 Changed 3 years ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

comment:15 Changed 17 months ago by gaba

Owner: dgoulet deleted
Status: needs_revisionassigned

dgoulet will assign himself to the ones he is working on right now.

comment:16 Changed 10 months ago by dgoulet

Keywords: tor-chan added

Branch ticket24554_033_02 has all the things! Wow.

It is way too big. It should be broken down into smaller pieces (tickets) and then properly tested.

This should really go in tor because it will help greatly to reduce our technical debt around the scheduler subsystem. But also, one particular thing, is that it removes the heap memory allocation done for each channel at each KIST mainloop. Under pressure, this leads to memory fragmentation quite a bit, and it has been observed.

comment:17 Changed 9 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

comment:18 Changed 8 months ago by teor

Status: assignednew

Change tickets that are assigned to nobody to "new".

Note: See TracTickets for help on using tickets.