Opened 6 months ago

Last modified 6 weeks ago

#24448 new defect

Channeltls adds the connection write event to main loop when writing a packed cell

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

Description

This is another problem in the line of channel subsystem and KIST. It is a difficult problem because tor is wired to use libevent connection's write and read event where KIST needs to control what is put on the network at what time.

So, here is the callstack that leads to the channeltls layer adding the conn->write_event to the main loop:

channel_tls_write_packed_cell_method()
 -> connection_buf_add()
   -> connection_write_to_buf_commit()
     -> connection_start_writing()
       -> event_add(conn->write_event, NULL)

Basically, when we flush cell(s) from a circuit, we do call back into the channel subsystem to write the packed cell (only packed cell on a circuit queue). The libevent write event is then added to the main loop. Then KIST scheduler kicks in a tick later (10 ms) and can also try to write.

I don't see a direct impact here but we could avoid an entire "write_event" callback on a connection with the KIST scheduler which seems to me like a good optimization especially on fast relays.

Child Tickets

Change History (5)

comment:1 Changed 6 months ago by dgoulet

Ok so because of #24449, having the libevent event scheduled is what saving the KIST scheduler from working properly.

Because KIST can flush 100 cells from the circuit queue to the outbuf, puts the channel in "waiting for cells" state and finally attempts to write those 100 cells which the connection subsystem will only do 16 (bucket limit), it will let 84 cells in the outbuf for a unknown period of time... Leading to really bad performance.

Thankfully, we have this libevent saving our asses... In other words, we should be VERY careful about changing anything here to force the KIST scheduler to not use libevent at all.

comment:2 Changed 4 months ago by dgoulet

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

comment:3 Changed 8 weeks ago by nickm

Keywords: 034-triage-20180328 added

comment:4 Changed 8 weeks ago by nickm

Keywords: 034-removed-20180328 added

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

comment:5 Changed 6 weeks 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.

Note: See TracTickets for help on using tickets.