Lazy bucket refill #53
Conversation
Right now, this patch just introduces and exposes some new functions. Later, these functions will get a little more complexity.
There was nothing round_robinish about this function.
Also document its actual behavior
We used to do this 10x per second in connection_buckets_refill(); instead, we now do it when the bucket becomes empty. This change is part of the work of making connection_buckets_refill() obsolete. Closes ticket 25828; bugfix on 0.2.3.5-alpha.
(This patch does not yet eliminate the global refill callback; fortunately, bucket refilling is idempotent.)
Previously this was done as part of the refill callback, but there's no real reason to do it like that. Since we're trying to remove the refill callback completely, we can do this work as part of record_num_bytes_transferred_impl(), which already does quite a lot of this.
Now that we update our buckets on demand before reading or writing, we no longer need to update them all every TokenBucketRefillInterval msec. When a connection runs out of bandwidth, we do need a way to reenable it, however. We do this by scheduling a timer to reenable all blocked connections for TokenBucketRefillInterval msec after a connection becomes blocked. (If we were using PerConnBWRate more, it might make sense to have a per-connection timer, rather than a single timeout. But since PerConnBWRate is currently (mostly) unused, I'm going to go for the simpler approach here, since usually whenever one connection has become blocked on bandwidth, most connections are blocked on bandwidth.) Implements ticket 25373.
Pull Request Test Coverage Report for Build 504
|
src/or/connection.c
Outdated
| @@ -138,6 +138,8 @@ static const char *proxy_type_to_string(int proxy_type); | |||
| static int get_proxy_type(void); | |||
| const tor_addr_t *conn_get_outbound_address(sa_family_t family, | |||
| const or_options_t *options, unsigned int conn_type); | |||
| static void blocked_connection_reenable_init(const or_options_t *options); | |||
| static void schedule_blocked_connection_reenable(void); | |||
A bit of nitpick. I would love to have those into the same namespace like:
blocked_connection_reenable_init()
blocked_connection_reenable_schedule()
Or maybe more meaningful like as those seems to reenable blocked connections? Which would also match the libevent event naming.
reenable_blocked_connection_init()
reenable_blocked_connection_schedule()
bd3f826 fixes this
| (void)ev; | ||
| (void)arg; | ||
| SMARTLIST_FOREACH_BEGIN(get_connection_array(), connection_t *, conn) { | ||
| if (conn->read_blocked_on_bw == 1) { |
Do we really need those checks? Can't we just set 0 to everyone there? I'm asking because every bits and pieces from that loop that we can gain in CPU is a win in my opinion because this list can be quite large. And this callback can be called often.
So, not every connection was necessarily enabled before -- some might have been off because they had nothing to write, or for some other reason. Re-enabling everything would waste more CPU than it would save.
(Also note that this loop uses less CPU than the old loop it replaces.)
Oh damn, I missed the start_reading() part ... so yes agree.
src/or/connection.c
Outdated
| last_refilled_global_buckets_ts = now_ts; | ||
| } | ||
|
|
||
| if (connection_speaks_cells(conn) && conn->state == OR_CONN_STATE_OPEN) { |
So hmmm, we don't count bw when negotiating TLS (state != OPEN)?
(That's right, apparently.)
(At least, I don't think we did before, did we?)
Hmm, yeah I think we do not looking at connection_buf_read_from_socket()
src/or/connection.c
Outdated
| @@ -3117,7 +3121,8 @@ connection_consider_empty_read_buckets(connection_t *conn) | |||
| if (!connection_is_rate_limited(conn)) | |||
| return; /* Always okay. */ | |||
|
|
|||
| bool is_global = true; | |||
| int is_global = 1; | |||
|
|
|||
What is going here? Actually, do we need this is_global at all? It is still ignored in the *_bw_exhausted() functions.
I'd like to leave it in, since we will likely want it in the future if we ever have special handling for the per-connection buckets.
Ok but then why the removal of the bool ?
No description provided.
The text was updated successfully, but these errors were encountered: