Opened 10 months ago

Last modified 4 months ago

#31483 accepted defect

token_bucket_ctr_adjust() does not convert rate to step

Reported by: teor Owned by: nickm
Priority: Very High Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: 043-deferred
Cc: gaba, nickm Actual Points:
Parent ID: Points: 0.5
Reviewer: asn Sponsor:


In #30687, we created a single-counter token bucket token_bucket_ctr.

token_bucket_rw_adjust() calls rate_per_sec_to_rate_per_step(rate), but token_bucket_ctr_adjust() does not.

I suggest we fix this bug by moving rate_per_sec_to_rate_per_step(rate) into token_bucket_cfg_init(). And we should add some documentation that explains the difference between rate and burst.

Gaba, this is sponsor 31-must, because it is a bug fix on sponsor 31 code that has already been merged.

Child Tickets

Change History (9)

comment:1 Changed 9 months ago by nickm

Priority: MediumVery High

Make all 042-must objects "Very High" priority.

comment:2 Changed 9 months ago by nickm

Owner: changed from dgoulet to nickm
Status: assignedaccepted

comment:3 Changed 9 months ago by nickm

Reviewer: asn

comment:4 Changed 9 months ago by nickm

So, I don't think we have a real misbehavior here, but we do have a small piece of ugliness.

The reason that I think we don't have a bug is that token_bucket_ctr_refill() doesn't use TICKS_PER_STEP, so it isn't necessary to convert the rate in "units per tick" into "units per step" when using a tocken_bucket_ctr_t. So that computation is correct.

To make matters more complicated, it's not clear to me that we want to harmonize both systems to use the same units. Right now, both "token_bucket_rw_t" and "token_bucket_ctr_t" are built to take their time in abstract ticks, and to track a resource measured in abstract units. For token_bucket_rw_t as it is used, the units are 1 byte, and the ticks are about 1 ms. To avoid rounding errors, we consider time as in "steps" of 16 ticks.

But for token_bucket_ctr_t as it is used for onion service DOS defenses, the units are 1 introduction cell, and the ticks are 1 second. 25 cells per second is the default rate. There's no reason to group these ticks into a 16-tick step. On the other hand, if we moved to a tick size that was smaller than 1 second, we'd have rounding errors with the cells-per-second calculation.

I think that if we want to harmonize these two types, we should do it by having them both use the same step/tick logic, and by fixing ticks around 1 ms. We should also say that units have to be chosen so that they have a meaningful resolution with respect to the tick size: instead of introduce cells per tick, we would have to use units something like "hundredths of an introduce cell per tick".

How much of this should we do in 0.4.2?

comment:5 Changed 9 months ago by teor

I feel like the answer is "none of it".

If the code works, but it's just ugly, that's an 0.4.3 issue.

comment:6 Changed 9 months ago by nickm

Keywords: 042-must removed
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final
Sponsor: Sponsor31-mustSponsor31-can

comment:7 Changed 4 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:8 Changed 4 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

comment:9 Changed 4 months ago by gaba

Sponsor: Sponsor31-can

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

Note: See TracTickets for help on using tickets.