Opened 3 years ago

Last modified 3 months ago

#23744 new defect

sched: Notification events have different meaning for each scheduler

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

Description

As an example, KIST controls how much and when channel data is pushed on the network which means this wants to write event used by the Vanilla scheduler as "queued cell but no space on the outbuf" is not something that is coherent with KIST.

A channel is scheduled in when it has cells in the queue, then they are flushed one by one by the KIST scheduler until the kernel says "no more space". Then, that channel is put back in the channel pending list and will get handled at the next tick of KIST.

So, we really don't need KIST to be notified of this event from connection_flushed_some() which is used by Vanilla to try to flush more cells in the outbuf because the outbuf has room for it.

Where it is useful is the second callsite of that even in channel_tls_handle_state_change_on_orconn() which notifies the scheduler that it might be in need of flushing some stuff. In the case of a brand new channel just opening, its state is "IDLE" and that even will then put it in "waiting for cells" after.

That being said, what needs to happened:

  1. Make the notification event a per-scheduler thing because KIST and Vanilla have different semantic for those events really. We should of course avoid as much as we can of code duplication and thus some "generic event handler" do make sense if they share the same semantic.
  1. Add a "channel state is open" notification event instead of "wants to write" which is really only true in very specific cases in channel_tls_handle_state_change_on_orconn(). That way, the scheduler can take a decision on the status of the channel instead of blind guessing it is waiting for cells.
  1. Nullify the "wants to write" event for KIST considering (2) so it stops possibly scheduling channels that do not need at all to be scheduled.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by dgoulet

Sponsor: SponsorV

Tagging all scheduler (and KIST) related tickets for SponsorV

comment:2 Changed 3 years ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

Taking the scheduler related tickets.

comment:3 Changed 3 years ago by dgoulet

Parent ID: 23993

comment:4 Changed 3 years ago by dgoulet

Parent ID: 23993#23993

comment:5 Changed 2 years ago by dgoulet

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

This is simply too huge for now to be in 033.

comment:6 Changed 2 years ago by nickm

Keywords: 034-triage-20180328 added

comment:7 Changed 2 years ago by nickm

Keywords: 034-removed-20180328 added

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

comment:8 Changed 2 years ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:9 Changed 12 months ago by gaba

Removing sponsor V as we do not have more time to include this tickets in the sponsor.

comment:10 Changed 12 months ago by gaba

Sponsor: SponsorV

Removing sponsor from tickets that we do not have time to fit in the remain of this sponsorship.

comment:11 Changed 12 months ago by gaba

Owner: dgoulet deleted
Status: acceptedassigned

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

comment:12 Changed 5 months ago by dgoulet

So (1) is really about making these:

void scheduler_channel_wants_writes(channel_t *chan);
MOCK_DECL(void, scheduler_channel_doesnt_want_writes, (channel_t *chan));
MOCK_DECL(void, scheduler_channel_has_waiting_cells, (channel_t *chan));

... function calls in scheduler_t which becomes specific per-scheduler type.

(2) would resolve #32388 that is scheduler could initialize properly a channel when it opens instead of living the bug.

(3) will come with (1).

Bottom line, (1) is what needs to be fixed with this ticket. It is pretty much simple. Code duplication is at risk but we can always have common code shared with something like scheduler_common.c.

comment:13 Changed 4 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

comment:14 Changed 3 months ago by teor

Status: assignednew

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

Note: See TracTickets for help on using tickets.