Opened 2 years ago

Closed 2 years ago

#25268 closed enhancement (implemented)

cmux: Remove round robin code and make EWMA mandatory

Reported by: dgoulet Owned by: dgoulet
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-cmux, tor-sched, tor-relay, tor-circuit, review-group-33, review-group-34
Cc: Actual Points:
Parent ID: #25328 Points:
Reviewer: isis Sponsor: SponsorM-can


Since 0.2.4.x stable, Tor has been using EWMA circuit policy which is enabled by the consensus param: CircuitPriorityHalflifeMsec=30000

The circuitmux.c code still does quite a bit to maintain the round robin circuit policy that was used prior to EWMA. It hasn't been used since 024 (except in Chutney, see #25265).

A lot of that code is called for every cell tor receives so it is part of our fast path. Removing this round robin code would help in performance and remove complexity from the code.

However, that round robin functionality is used as a fallback if the EWMA policy was either not set or couldn't pick an active circuit (I'm not sure that is possible if we have active circuits).

Thus, the side effect of removing this code is that we'll now enforce a cmux to have a policy set and make the pick_active_circuit() callback mandatory. That is OK because we've been enforcing that since 024.

It would also probably mean that we need to put in a default value in tor code for CircuitPriorityHalflifeMsec so that if the consensus doesn't specify it, we fallback to a usable value instead of being able to disable EWMA.

This piece of work will help out with improving performance of the cmux subsystem with #25152 and make sure we stick to only the required code for this fast path

Child Tickets

Change History (8)

comment:1 Changed 2 years ago by dgoulet

Status: assignedneeds_review

See branch: ticket25268_034_01

As I said, this mostly cleans up code but also make the EWMA policy mandatory. Unit test and chutney tests pass. Stats of the branch:

 16 files changed, 119 insertions(+), 805 deletions(-)

There could be a commit that would benefit an OnionGit MR, let me know if needed.

comment:2 Changed 2 years ago by nickm

Keywords: review-group-34 added

comment:3 Changed 2 years ago by nickm

Keywords: review-group-33 added; review-group-34 removed

comment:4 Changed 2 years ago by dgoulet

Parent ID: #25328

comment:5 Changed 2 years ago by isis

Reviewer: isis

comment:6 Changed 2 years ago by isis

Sponsor: SponsorM-can
Status: needs_reviewmerge_ready

Looks good! I also ran make check && make test-stem && make test-network, and ran it through Travis where it passed.

Stem was mad about some stuff, frequently erroring on [] != [''], but I'm pretty sure that's a Stem problem? Or my Stem is too old or something (v1.4.1). Chutney seemed fine though.

comment:7 Changed 2 years ago by nickm

Keywords: review-group-34 added

comment:8 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

I wrote a changes file for this, and merged in master. Thanks!

Note: See TracTickets for help on using tickets.