Opened 7 months ago

Closed 5 months ago

#25373 closed enhancement (implemented)

Avoid needless wakeups for token bucket refills.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-roadmap-subtask, 034-triage-20180328, 034-included-20180328
Cc: Actual Points:
Parent ID: #25500 Points:
Reviewer: dgoulet Sponsor: Sponsor8

Description

In 0.2.3.5-alpha, we increased our default token bucket refill interval from 1 time per second to 10 times per second. (See #3630 and proposal 183.)

AFAICT, this is now the most frequently running timer causing wakeups on an idle Tor instance. It even causes wakeups on Tor instances when DisableNetwork is set. We can do better!

Two key insights:

  • First, it is not necessary to actually refill these buckets periodically. Instead, we can store the last time at which we refilled each one, and calculate its current size at immediately before we read or write. The only thing we'll need to use a timer for is to wake up connections on which we've stopped reading or writing.
  • Second, we only need to have this timer active when at least one connection is blocked on bandwidth.

Child Tickets

TicketTypeStatusOwnerSummary
#25760defectclosednickmRemove TestingEnableTbEmptyEvent if it is no longer used
#25766defectclosednickmRefactor token buckets to meet current coding standards
#25793defectclosednickmRefactor refactored token bucket API to be more generic
#25828defectclosednickmOur "has our write bucket been empty recently" check is dependent on TokenBucketRefillInterval

Change History (15)

comment:1 Changed 7 months ago by nickm

Sponsor: Sponsor8

comment:2 Changed 6 months ago by dgoulet

Parent ID: #25500

Setting master ticket parent ID.

comment:3 Changed 6 months ago by arma

I'm a fan.

In fact, if we're doing just-in-time refilling, we might even be able to get more fine-grained than once-every-100-ms. That is, we could look at how far through the second we are, and refill proportional to that fraction.

(It is an open question whether we still have strange bumps in our bandwidth flows now that we moved to 10-refills per second. We definitely had the strange bumps when it was 1-refill-per-second. Also, refill rates interact in subtle ways with the scheduler, so it might be good to check with pastly/rob/dgoulet to see if they have any guidance here.)

comment:4 Changed 6 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 6 months ago by nickm

Keywords: 034-roadmap-subtask added

comment:6 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:7 Changed 6 months ago by nickm

Keywords: 034-included-20180328 added

comment:8 Changed 6 months ago by nickm

I've started work on the refactoring part of this logic in a branch called token_bucket_refactor on top of remove_tb_empty.

comment:9 Changed 5 months ago by nickm

Okay, there's now a branch on top of master. See lazy_bucket_refill, with a pull request here at https://github.com/torproject/tor/pull/53 . It also contains a fix for #25828.

comment:10 Changed 5 months ago by nickm

Reviewer: dgoulet
Status: acceptedneeds_review

comment:11 Changed 5 months ago by dgoulet

Status: needs_reviewneeds_revision

Commented on the PR!

comment:12 Changed 5 months ago by nickm

Status: needs_revisionneeds_review

replied on the PR!

comment:13 Changed 5 months ago by nickm

(I forget where we left this in our IRC conversation -- are we merge_ready here, or should I do some more?)

comment:14 Changed 5 months ago by dgoulet

Status: needs_reviewmerge_ready

Ah yes this is lgtm. We are in merge_ready.

comment:15 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merging!

Note: See TracTickets for help on using tickets.