Token bucket refactor #46
Conversation
This option was used for shadow testing previously, but is no longer used for anything. It interferes with refactoring our token buckets.
This differs from our previous token bucket abstraction in a few
ways:
1) It is an abstraction, and not a collection of fields.
2) It is meant to be used with monotonic timestamps, which should
produce better results than calling gettimeofday over and over.
src/common/token_bucket.c
Outdated
| /* See LICENSE for licensing information */ | ||
|
|
||
| /** | ||
| * \file address.c |
tocket_bucket.c might be better. Also, while at it, maybe the copyright should only be "2018 TPO".
or even token_bucket.c . ;)
5ce7846 has a fix.
| token_bucket_init(token_bucket_t *bucket, | ||
| uint32_t rate, | ||
| uint32_t burst, | ||
| uint32_t now_ts) |
Within the DoS subsystem, I did something extremely similar (of course bucketting...) but I went for uint64_t on purposes so we could address just all use cases in the future. Turned out that we could just have uint32_t instead since we use consensus param for some values which are bound to be int32_t.
You think we could go to 64 bit there or memory footprint is the idea of 32 bit?
Hm. So, the existing code here assumes it's 32-bit.
Do we ever need to allow a burst of greater than 4GB? Nobody has actually asked for it so far. And since the API returns a size_t, it should be comparatively easy to make this bigger later on if we want it.
Ok, after seeing how it is used in the code and tightly coupled with torrc options that are int32_t, this is fine.
|
|
||
| /** Convert a rate in bytes per second to a rate in bytes per step */ | ||
| static uint32_t | ||
| rate_per_sec_to_rate_per_step(uint32_t rate) |
I think we should inline this for the small tiny bit of performance we get. Token bucket like that are in fast path often.
I think the compiler should be able to figure out that this function should be inlined on its own. (In my experiments, both gcc and clang inline it.)
Also, note that this function is only used when setting the rate -- not when actually using the buckets.
src/common/token_bucket.c
Outdated
| if (bucket->read_bucket > (int32_t)burst) | ||
| bucket->read_bucket = burst; | ||
| if (bucket->write_bucket > (int32_t)burst) | ||
| bucket->write_bucket = burst; |
Could we do a = MIN(bucket->read_bucket, burst) here instead? Would just make things a bit clearer in terms of code semantic I believe.
c752a23 has a fix here.
src/common/token_bucket.c
Outdated
| const int32_t burst, | ||
| const uint32_t elapsed_steps) | ||
| { | ||
| const int was_empty = *bucketptr <= 0; |
Huge nitpick but for my brain to parse, having (*bucketptr <= 0) would be great.
Done with 3deeec3
src/common/token_bucket.c
Outdated
| { | ||
| const int was_empty = *bucketptr <= 0; | ||
| /* The casts here prevent an underflow. */ | ||
| const size_t gap = ((size_t)burst) - ((size_t)*bucketptr); |
bucketptr = -1 and burst = INT32_MAX we wned up with gap = INT32_MAX + 1 which means we can go above the maximum burst value for the gap. It is fine since we normalize it to the "burst" value after but I think its worth commenting that this can happen and it is ok.
4fc779c is a comment rewrite for this.
| token_bucket_refill(token_bucket_t *bucket, | ||
| uint32_t now_ts) | ||
| { | ||
| const uint32_t elapsed_ticks = (now_ts - bucket->last_refilled_at_ts); |
As the commit messages mentions it, it is meant to be use with monotonic time only so then now_ts will never be smaller than last_refilled_at_ts. Can we make sure of this?
We had issues in the scheduler with Windows not really having monotonic clock so we had to adjust to this reality (see kist_scheduler_schedule().
I've done something similar here with 2c4eef1
|
|
||
| static int | ||
| decrement_single_bucket(int32_t *bucketptr, | ||
| ssize_t n) |
Why ssize_t and not int32_t which is what the counters uses?
So that the API is agnostic to the implementation details.
| uint32_t rate; | ||
| int32_t burst; | ||
| int32_t read_bucket; | ||
| int32_t write_bucket; |
Is more of a design questions.
I would love to use that in the DoS subsystem which would mean removing bunch of codes. However, this token_bucket_t interfaces kind of bakes the fact that you have "read" and "write" thus a connection.
If that struct had only one bucket, we could use it in some other places in a more generic way. Which would mean we would have token_bucket_t read_bucket and the write one in the or_connection_t. Is that a major inconvenience? Because as it is, I can use that in DoS code but not really semantically.
This is what I was asking you about earlier: if we make the change you suggest here, we have to include the rate, burst, and last_refilled_at fields in the or_connection_t twice.
I can try a refactoring to make this change, but I'd rather do it as a separate branch after this is merged, if that's okay with you.
There are also couple of issues with this implementation that make it a little tricky to reuse. Most notably, it assumes that it's working in units of bytes per second, but it converts them to "bytes per step". That conversion is fine for bandwidths, but the imprecesion might not be okay for all other units: if so, we'd have to configure it.
Right, I think I realized what you meant when you asked when seeing this.
Hmmm... and it makes sense also with our current options since we only have a single bw rate and not a read bw rate and write bw rate.
If we could make this whole piece of work in a generic way to have token bucket for other things than "bytes per second", it would be really great so we don't have multiple bucket implementation.
I'm not that concerned about having duplicate values that is 4 bytes (rate), 4 bytes (burst) and 4 bytes (timetamp) which means 16 bytes extra. And scaled to 100k connections let say, it is overhead of ~2MB which for a relay is not that much compared to a mobile client.
An option then is to configure a "bucket param" object statically and attach it to any bucket you create with it. The DoS subsystem does not copy rate/burst to the bucket object and uses the global value each time instead. If we had a way to do this with a "param" object, it would also work out pretty well. Which also could reduce by 8 bytes (rate + burst) the token_bucket_t object and make it only have a pointer to a static object (+ 4 bytes). Anyway, we aren't saving much space here either way but what I'm more wanting here is a more generic API so it can be used elsewhere. I know that we'll have to go to hidden service circuit creation rate limitation at some point which a bucket system would be great to have.
src/or/main.c
Outdated
|
|
||
| stats_n_bytes_read += bytes_read; | ||
| stats_n_bytes_written += bytes_written; | ||
| if (accounting_is_enabled(options) && milliseconds_elapsed >= 0) | ||
| accounting_add_bytes(bytes_read, bytes_written, seconds_rolled_over); | ||
|
|
||
| if (milliseconds_elapsed > 0) { | ||
| connection_bucket_refill(milliseconds_elapsed, (time_t)now.tv_sec, | ||
| connection_bucket_refill((time_t)now.tv_sec, |
refill_callback() doesn't use monotonic time unless we do adjustments in the previous commit I pointed it out.
Fix boilerplate on token_bucket.c header.
Use MIN() to cap the bucket sizes.
Add some parentheses.
Improve comment in refill_single_bucket() to better describe C underflow rules.
Actually, don't. Instead: Expose a function that computes stamp units from msec. (It turns out we can't just expose STAMP_TICKS_PER_SECOND, since Apple doesn't have that.)
Increase TICKS_PER_STEP, to reduce rounding errors.
Stop using STAMP_TICKS_PER_SECOND: Apple doesn't have it.
Fix a copy-paste error in an earlier fixup commit
You sure about this 300 * 1000 ?
Isn't 5 minutes backward would be UINT32_MAX - 300 ? Lets consider this example:
now_ts = 100
last_refilled_as_ts = 400
Considering that now_ts jumped backwards of 300 seconds.
elapsed_ticks = 4294966995 or in other words UINT32_MAX - 300.
Also, 48 days in seconds is 4147200 which is not really 300 * 1000 so I'm puzzled here?
Ticks are about 1ms long. (They're somewhere between 0.5 and 2.0 ms, technically.)
src/common/token_bucket.c
Outdated
| if (bucket->write_bucket > (int32_t)burst) | ||
| bucket->write_bucket = burst; | ||
| bucket->read_bucket = MIN(bucket->read_bucket, (int32_t)burst); | ||
| bucket->read_bucket = MIN(bucket->write_bucket, (int32_t)burst); | ||
| } |
write_bucket for the second.
|
This was squashed and merged with b152d62 |
No description provided.
The text was updated successfully, but these errors were encountered: