#25793 closed defect (implemented)

Refactor refactored token bucket API to be more generic

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
Cc: Actual Points:
Parent ID: #25373 Points:
Reviewer: dgoulet Sponsor: Sponsor8-can

Description

The #25766 work makes us have a clean token bucket API. But that API isn't reusable yet. Let's fix it so we can use it for all the other places in our code that we want a token bucket.

The two main things to do here are:

  • decouple "read bucket" from "write bucket".
  • decouple "parameters" from "bucket value".
  • make time generic.

Child Tickets

Change History (8)

comment:1 Changed 16 months ago by nickm

Reviewer: dgoulet
Status: assignedneeds_review

https://github.com/torproject/tor/pull/47 is a github pull request here; the name is token_bucket_once_again

comment:2 Changed 16 months ago by dgoulet

Status: needs_reviewneeds_revision

Review on the PR.

I like it very much. I'm mostly concerned about the 32 bit "timestamp" which might not be ideal for Unix timestamp use case.

comment:3 Changed 16 months ago by nickm

Status: needs_revisionneeds_review

Thanks! I've answered on the PR, fixing one issue and discussing another.

Note that the 32-bit timestamp only remains in the token_bucket_rw_t layer: everything else is abstracted to not use it now. The only thing in the "raw" layer that uses time is the refill_steps() function, which takes a number of elapsed time units as an input.

comment:4 Changed 16 months ago by dgoulet

So hmmm, for anyone to implement a token bucket, they would need to create a specialized version? The token_bucket_raw_reset() API function has the token_bucket_timestamp_t as a param which means any specialized token object will kind of need to use that timestamp struct? Or not use the raw reset?

Where token_bucket_raw_refill_steps() only takes "steps" which means that if one would use the token bucket API without creating a new object, it would need to compute that steps value on its side instead of within a token_bucket_t API?

In other words, the implementation using the token bucket will always need to keep the last refilled time or create a specialized object?

comment:5 in reply to:  4 Changed 16 months ago by nickm

Replying to dgoulet:

So hmmm, for anyone to implement a token bucket, they would need to create a specialized version? The token_bucket_raw_reset() API function has the token_bucket_timestamp_t as a param which means any specialized token object will kind of need to use that timestamp struct? Or not use the raw reset?

This changed in d0769156a67873e98ff08f6ab5e7d37e4b4866a6 -- it doesn't take a timestamp any more.

Where token_bucket_raw_refill_steps() only takes "steps" which means that if one would use the token bucket API without creating a new object, it would need to compute that steps value on its side instead of within a token_bucket_t API?

Right. It needs to pass the time elapsed.

In other words, the implementation using the token bucket will always need to keep the last refilled time or create a specialized object?

Yes, that's right.

comment:6 Changed 16 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm with latest branch on GH.

comment:7 Changed 16 months ago by nickm

squashed and merged to master. Now the real #25373 fun can begin. :)

comment:8 Changed 16 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.