From 6b1dba214db3058b143bbb4d4c4bdfee32d100f1 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 13:45:21 -0500 Subject: [PATCH 1/7] cmux: Make EWMA policy mandatory To achieve this, a default value for the CircuitPriorityHalflife option was needed. We still look in the options and then the consensus but in case no value can be found, the default CircuitPriorityHalflifeMsec=30000 is used. It it the value we've been using since 0.2.4.4-alpha. This means that EWMA, our only policy, can not be disabled anymore fallbacking to the round robin algorithm. Unneeded code to control that is removed in this commit. Part of #25268 Signed-off-by: David Goulet --- src/or/channel.c | 15 ------ src/or/channel.h | 3 -- src/or/channeltls.c | 5 +- src/or/circuitmux_ewma.c | 105 ++++++++++++++++++++++++--------------- src/or/circuitmux_ewma.h | 1 - src/or/config.c | 9 ---- src/or/networkstatus.c | 10 ---- src/test/test_channel.c | 2 +- 8 files changed, 68 insertions(+), 82 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index ff1cfde2a..a9483ee02 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2108,21 +2108,6 @@ channel_listener_dumpstats(int severity) } } -/** - * Set the cmux policy on all active channels. - */ -void -channel_set_cmux_policy_everywhere(circuitmux_policy_t *pol) -{ - if (!active_channels) return; - - SMARTLIST_FOREACH_BEGIN(active_channels, channel_t *, curr) { - if (curr->cmux) { - circuitmux_set_policy(curr->cmux, pol); - } - } SMARTLIST_FOREACH_END(curr); -} - /** * Clean up channels. * diff --git a/src/or/channel.h b/src/or/channel.h index 0af5aed41..6cf8cd7f7 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -422,9 +422,6 @@ void channel_free_all(void); void channel_dumpstats(int severity); void channel_listener_dumpstats(int severity); -/* Set the cmux policy on all active channels */ -void channel_set_cmux_policy_everywhere(circuitmux_policy_t *pol); - #ifdef TOR_CHANNEL_INTERNAL_ #ifdef CHANNEL_PRIVATE_ diff --git a/src/or/channeltls.c b/src/or/channeltls.c index 9000703b0..54d94f610 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -160,9 +160,8 @@ channel_tls_common_init(channel_tls_t *tlschan) chan->write_var_cell = channel_tls_write_var_cell_method; chan->cmux = circuitmux_alloc(); - if (cell_ewma_enabled()) { - circuitmux_set_policy(chan->cmux, &ewma_policy); - } + /* We only have one policy for now so always set it to EWMA. */ + circuitmux_set_policy(chan->cmux, &ewma_policy); } /** diff --git a/src/or/circuitmux_ewma.c b/src/or/circuitmux_ewma.c index fde2d22a8..d9ee8d3ef 100644 --- a/src/or/circuitmux_ewma.c +++ b/src/or/circuitmux_ewma.c @@ -223,8 +223,6 @@ ewma_cmp_cmux(circuitmux_t *cmux_1, circuitmux_policy_data_t *pol_data_1, * has value ewma_scale_factor ** N.) */ static double ewma_scale_factor = 0.1; -/* DOCDOC ewma_enabled */ -static int ewma_enabled = 0; /*** EWMA circuitmux_policy_t method table ***/ @@ -612,13 +610,6 @@ cell_ewma_tick_from_timeval(const struct timeval *now, return res; } -/** Tell the caller whether ewma_enabled is set */ -int -cell_ewma_enabled(void) -{ - return ewma_enabled; -} - /** Compute and return the current cell_ewma tick. */ unsigned int cell_ewma_get_tick(void) @@ -626,45 +617,79 @@ cell_ewma_get_tick(void) return ((unsigned)approx_time() / EWMA_TICK_LEN); } +/* Default value for the CircuitPriorityHalflifeMsec consensus parameter in + * msec. */ +#define CMUX_PRIORITY_HALFLIFE_MSEC_DEFAULT 30000 +/* Minimum and maximum value for the CircuitPriorityHalflifeMsec consensus + * parameter. */ +#define CMUX_PRIORITY_HALFLIFE_MSEC_MIN 1 +#define CMUX_PRIORITY_HALFLIFE_MSEC_MAX INT32_MAX + +/* Return the value of the circuit priority halflife from the options if + * available or else from the consensus (in that order). If none can be found, + * a default value is returned. + * + * The source_msg points to a string describing from where the value was + * picked so it can be used for logging. */ +static double +get_circuit_priority_halflife(const or_options_t *options, + const networkstatus_t *consensus, + const char **source_msg) +{ + int32_t halflife_ms; + double halflife; + /* Compute the default value now. We might need it. */ + double halflife_default = + ((double) CMUX_PRIORITY_HALFLIFE_MSEC_DEFAULT) / 1000.0; + + /* Try to get it from configuration file first. */ + if (options && options->CircuitPriorityHalflife < EPSILON) { + halflife = options->CircuitPriorityHalflife; + *source_msg = "CircuitPriorityHalflife in configuration"; + goto end; + } + + /* Try to get the msec value from the consensus. */ + halflife_ms = networkstatus_get_param(consensus, + "CircuitPriorityHalflifeMsec", + CMUX_PRIORITY_HALFLIFE_MSEC_DEFAULT, + CMUX_PRIORITY_HALFLIFE_MSEC_MIN, + CMUX_PRIORITY_HALFLIFE_MSEC_MAX); + halflife = ((double) halflife_ms) / 1000.0; + *source_msg = "CircuitPriorityHalflifeMsec in consensus"; + + end: + /* We should never go below the EPSILON else we would consider it disabled + * and we can't have that. */ + if (halflife < EPSILON) { + log_warn(LD_CONFIG, "CircuitPriorityHalflife is too small (%f). " + "Adjusting to the smallest value allowed: %f.", + halflife, halflife_default); + halflife = halflife_default; + } + return halflife; +} + /** Adjust the global cell scale factor based on options */ void cell_ewma_set_scale_factor(const or_options_t *options, const networkstatus_t *consensus) { - int32_t halflife_ms; double halflife; const char *source; - if (options && options->CircuitPriorityHalflife >= -EPSILON) { - halflife = options->CircuitPriorityHalflife; - source = "CircuitPriorityHalflife in configuration"; - } else if (consensus && (halflife_ms = networkstatus_get_param( - consensus, "CircuitPriorityHalflifeMsec", - -1, -1, INT32_MAX)) >= 0) { - halflife = ((double)halflife_ms)/1000.0; - source = "CircuitPriorityHalflifeMsec in consensus"; - } else { - halflife = EWMA_DEFAULT_HALFLIFE; - source = "Default value"; - } - if (halflife <= EPSILON) { - /* The cell EWMA algorithm is disabled. */ - ewma_scale_factor = 0.1; - ewma_enabled = 0; - log_info(LD_OR, - "Disabled cell_ewma algorithm because of value in %s", - source); - } else { - /* convert halflife into halflife-per-tick. */ - halflife /= EWMA_TICK_LEN; - /* compute per-tick scale factor. */ - ewma_scale_factor = exp( LOG_ONEHALF / halflife ); - ewma_enabled = 1; - log_info(LD_OR, - "Enabled cell_ewma algorithm because of value in %s; " - "scale factor is %f per %d seconds", - source, ewma_scale_factor, EWMA_TICK_LEN); - } + /* Both options and consensus can be NULL. This assures us to either get a + * valid configured value or the default one. */ + halflife = get_circuit_priority_halflife(options, consensus, &source); + + /* convert halflife into halflife-per-tick. */ + halflife /= EWMA_TICK_LEN; + /* compute per-tick scale factor. */ + ewma_scale_factor = exp( LOG_ONEHALF / halflife ); + log_info(LD_OR, + "Enabled cell_ewma algorithm because of value in %s; " + "scale factor is %f per %d seconds", + source, ewma_scale_factor, EWMA_TICK_LEN); } /** Return the multiplier necessary to convert the value of a cell sent in diff --git a/src/or/circuitmux_ewma.h b/src/or/circuitmux_ewma.h index 8f4e57865..4e9e512df 100644 --- a/src/or/circuitmux_ewma.h +++ b/src/or/circuitmux_ewma.h @@ -15,7 +15,6 @@ extern circuitmux_policy_t ewma_policy; /* Externally visible EWMA functions */ -int cell_ewma_enabled(void); unsigned int cell_ewma_get_tick(void); void cell_ewma_set_scale_factor(const or_options_t *options, const networkstatus_t *consensus); diff --git a/src/or/config.c b/src/or/config.c index c246bd00e..0ac777921 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1799,7 +1799,6 @@ options_act(const or_options_t *old_options) char *msg=NULL; const int transition_affects_workers = old_options && options_transition_affects_workers(old_options, options); - int old_ewma_enabled; const int transition_affects_guards = old_options && options_transition_affects_guards(old_options, options); @@ -2073,16 +2072,8 @@ options_act(const or_options_t *old_options) if (accounting_is_enabled(options)) configure_accounting(time(NULL)); - old_ewma_enabled = cell_ewma_enabled(); /* Change the cell EWMA settings */ cell_ewma_set_scale_factor(options, networkstatus_get_latest_consensus()); - /* If we just enabled ewma, set the cmux policy on all active channels */ - if (cell_ewma_enabled() && !old_ewma_enabled) { - channel_set_cmux_policy_everywhere(&ewma_policy); - } else if (!cell_ewma_enabled() && old_ewma_enabled) { - /* Turn it off everywhere */ - channel_set_cmux_policy_everywhere(NULL); - } /* Update the BridgePassword's hashed version as needed. We store this as a * digest so that we can do side-channel-proof comparisons on it. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 31ecb2098..80cdc9e5b 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1779,7 +1779,6 @@ networkstatus_set_current_consensus(const char *consensus, consensus_waiting_for_certs_t *waiting = NULL; time_t current_valid_after = 0; int free_consensus = 1; /* Free 'c' at the end of the function */ - int old_ewma_enabled; int checked_protocols_already = 0; if (flav < 0) { @@ -2003,17 +2002,8 @@ networkstatus_set_current_consensus(const char *consensus, /* XXXXNM Microdescs: needs a non-ns variant. ???? NM*/ update_consensus_networkstatus_fetch_time(now); - /* Update ewma and adjust policy if needed; first cache the old value */ - old_ewma_enabled = cell_ewma_enabled(); /* Change the cell EWMA settings */ cell_ewma_set_scale_factor(options, c); - /* If we just enabled ewma, set the cmux policy on all active channels */ - if (cell_ewma_enabled() && !old_ewma_enabled) { - channel_set_cmux_policy_everywhere(&ewma_policy); - } else if (!cell_ewma_enabled() && old_ewma_enabled) { - /* Turn it off everywhere */ - channel_set_cmux_policy_everywhere(NULL); - } /* XXXX this call might be unnecessary here: can changing the * current consensus really alter our view of any OR's rate limits? */ diff --git a/src/test/test_channel.c b/src/test/test_channel.c index bdc9d32f7..392759768 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -575,7 +575,7 @@ test_channel_outbound_cell(void *arg) channel_register(chan); tt_int_op(chan->registered, OP_EQ, 1); /* Set EWMA policy so we can pick it when flushing. */ - channel_set_cmux_policy_everywhere(&ewma_policy); + circuitmux_set_policy(chan->cmux, &ewma_policy); tt_ptr_op(circuitmux_get_policy(chan->cmux), OP_EQ, &ewma_policy); /* Register circuit to the channel circid map which will attach the circuit From 9af5b625e8be64ed63ad5b19ae6c4b450e977ce0 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 13:51:34 -0500 Subject: [PATCH 2/7] cmux: Rename cell_ewma_set_scale_factor() It is rename to something more meaningful that explains what it does exactly which is sets the EWMA options (currently only one exists). The new name is cmux_ewma_set_options(). Also, remove a public function from circuitmux_ewma.h that is only used in the C file. Make it static inline as well. Signed-off-by: David Goulet --- src/or/circuitmux_ewma.c | 18 +++++++++--------- src/or/circuitmux_ewma.h | 6 +++--- src/or/config.c | 2 +- src/or/networkstatus.c | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/or/circuitmux_ewma.c b/src/or/circuitmux_ewma.c index d9ee8d3ef..b2ace8a9f 100644 --- a/src/or/circuitmux_ewma.c +++ b/src/or/circuitmux_ewma.c @@ -241,6 +241,13 @@ circuitmux_policy_t ewma_policy = { /*** EWMA method implementations using the below EWMA helper functions ***/ +/** Compute and return the current cell_ewma tick. */ +static inline unsigned int +cell_ewma_get_tick(void) +{ + return ((unsigned)approx_time() / EWMA_TICK_LEN); +} + /** * Allocate an ewma_policy_data_t and upcast it to a circuitmux_policy_data_t; * this is called when setting the policy on a circuitmux_t to ewma_policy. @@ -610,13 +617,6 @@ cell_ewma_tick_from_timeval(const struct timeval *now, return res; } -/** Compute and return the current cell_ewma tick. */ -unsigned int -cell_ewma_get_tick(void) -{ - return ((unsigned)approx_time() / EWMA_TICK_LEN); -} - /* Default value for the CircuitPriorityHalflifeMsec consensus parameter in * msec. */ #define CMUX_PRIORITY_HALFLIFE_MSEC_DEFAULT 30000 @@ -672,8 +672,8 @@ get_circuit_priority_halflife(const or_options_t *options, /** Adjust the global cell scale factor based on options */ void -cell_ewma_set_scale_factor(const or_options_t *options, - const networkstatus_t *consensus) +cmux_ewma_set_options(const or_options_t *options, + const networkstatus_t *consensus) { double halflife; const char *source; diff --git a/src/or/circuitmux_ewma.h b/src/or/circuitmux_ewma.h index 4e9e512df..2ef8c2586 100644 --- a/src/or/circuitmux_ewma.h +++ b/src/or/circuitmux_ewma.h @@ -12,12 +12,12 @@ #include "or.h" #include "circuitmux.h" +/* The public EWMA policy callbacks object. */ extern circuitmux_policy_t ewma_policy; /* Externally visible EWMA functions */ -unsigned int cell_ewma_get_tick(void); -void cell_ewma_set_scale_factor(const or_options_t *options, - const networkstatus_t *consensus); +void cmux_ewma_set_options(const or_options_t *options, + const networkstatus_t *consensus); #endif /* !defined(TOR_CIRCUITMUX_EWMA_H) */ diff --git a/src/or/config.c b/src/or/config.c index 0ac777921..107f9ac5f 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -2073,7 +2073,7 @@ options_act(const or_options_t *old_options) configure_accounting(time(NULL)); /* Change the cell EWMA settings */ - cell_ewma_set_scale_factor(options, networkstatus_get_latest_consensus()); + cmux_ewma_set_options(options, networkstatus_get_latest_consensus()); /* Update the BridgePassword's hashed version as needed. We store this as a * digest so that we can do side-channel-proof comparisons on it. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 80cdc9e5b..3455a7bb6 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -2003,7 +2003,7 @@ networkstatus_set_current_consensus(const char *consensus, update_consensus_networkstatus_fetch_time(now); /* Change the cell EWMA settings */ - cell_ewma_set_scale_factor(options, c); + cmux_ewma_set_options(options, c); /* XXXX this call might be unnecessary here: can changing the * current consensus really alter our view of any OR's rate limits? */ From 9d68647ba3fc0774653be7ac994f8a6307b146f9 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 13:59:51 -0500 Subject: [PATCH 3/7] cmux: Remove PARANOIA assert functions The reason to do so is because these functions haven't been used in years so since 0.2.4, every callsite is NOP. In future commits, we'll remove the round robin circuit policy which is mostly validated within those function. This simplifies the code greatly and remove dead code for which we never had a configure option in the first place nor an easy way to use them in production. Part of #25268 Signed-off-by: David Goulet --- src/or/circuitmux.c | 330 -------------------------------------------- src/or/relay.c | 25 ---- src/or/relay.h | 1 - 3 files changed, 356 deletions(-) diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index fe3d8f133..6ff03ab79 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -176,17 +176,6 @@ struct chanid_circid_muxinfo_t { circuit_muxinfo_t muxinfo; }; -/* - * Internal-use #defines - */ - -#ifdef CMUX_PARANOIA -#define circuitmux_assert_okay_paranoid(cmux) \ - circuitmux_assert_okay(cmux) -#else -#define circuitmux_assert_okay_paranoid(cmux) -#endif /* defined(CMUX_PARANOIA) */ - /* * Static function declarations */ @@ -211,9 +200,6 @@ static inline circuit_t ** circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ); static inline circuit_t ** circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ); -static void circuitmux_assert_okay_pass_one(circuitmux_t *cmux); -static void circuitmux_assert_okay_pass_two(circuitmux_t *cmux); -static void circuitmux_assert_okay_pass_three(circuitmux_t *cmux); /* Static global variables */ @@ -243,8 +229,6 @@ circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, tor_assert(cmux); tor_assert(circ); - circuitmux_assert_okay_paranoid(cmux); - /* Figure out our next_p and prev_p for this cmux/direction */ if (direction) { if (direction == CELL_DIRECTION_OUT) { @@ -305,8 +289,6 @@ circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, } /* Set the tail to this circuit */ cmux->active_circuits_tail = circ; - - circuitmux_assert_okay_paranoid(cmux); } static inline circuit_t ** @@ -406,11 +388,6 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) circuit_t *circ = NULL; tor_assert(cmux); - /* - * Don't circuitmux_assert_okay_paranoid() here; this gets called when - * channels are being freed and have already been unregistered, so - * the channel ID lookups it does will fail. - */ i = HT_START(chanid_circid_muxinfo_map, cmux->chanid_circid_map); while (i) { @@ -944,7 +921,6 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, tor_assert(circ); tor_assert(direction == CELL_DIRECTION_IN || direction == CELL_DIRECTION_OUT); - circuitmux_assert_okay_paranoid(cmux); /* * Figure out which channel we're using, and get the circuit's current @@ -1070,8 +1046,6 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, } cmux->n_cells += cell_count; } - - circuitmux_assert_okay_paranoid(cmux); } /** @@ -1095,7 +1069,6 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) tor_assert(cmux); tor_assert(cmux->chanid_circid_map); tor_assert(circ); - circuitmux_assert_okay_paranoid(cmux); /* See if we have it for n_chan/n_circ_id */ if (circ->n_chan) { @@ -1162,8 +1135,6 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) /* Free the hash entry */ tor_free(hashent); } - - circuitmux_assert_okay_paranoid(cmux); } /** @@ -1186,11 +1157,6 @@ circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, tor_assert(circ); tor_assert(direction == CELL_DIRECTION_OUT || direction == CELL_DIRECTION_IN); - /* - * Don't circuitmux_assert_okay_paranoid(cmux) here because the cell count - * already got changed and we have to update the list for it to be consistent - * again. - */ /* Get the right set of active list links for this direction */ if (direction == CELL_DIRECTION_OUT) { @@ -1258,8 +1224,6 @@ circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, cmux->policy->notify_circ_active(cmux, cmux->policy_data, circ, hashent->muxinfo.policy_data); } - - circuitmux_assert_okay_paranoid(cmux); } /** @@ -1283,11 +1247,6 @@ circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, tor_assert(circ); tor_assert(direction == CELL_DIRECTION_OUT || direction == CELL_DIRECTION_IN); - /* - * Don't circuitmux_assert_okay_paranoid(cmux) here because the cell count - * already got changed and we have to update the list for it to be consistent - * again. - */ /* Get the right set of active list links for this direction */ if (direction == CELL_DIRECTION_OUT) { @@ -1372,8 +1331,6 @@ circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, cmux->policy->notify_circ_inactive(cmux, cmux->policy_data, circ, hashent->muxinfo.policy_data); } - - circuitmux_assert_okay_paranoid(cmux); } /** @@ -1400,8 +1357,6 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, tor_assert(cmux); tor_assert(circ); - circuitmux_assert_okay_paranoid(cmux); - /* Search for this circuit's entry */ hashent = circuitmux_find_map_entry(cmux, circ); /* Assert that we found one */ @@ -1435,14 +1390,8 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, hashent->muxinfo.cell_count = n_cells; circuitmux_make_circuit_active(cmux, circ, hashent->muxinfo.direction); } else { - /* - * Update the entry cell count like this so we can put a - * circuitmux_assert_okay_paranoid inside make_circuit_(in)active() too. - */ hashent->muxinfo.cell_count = n_cells; } - - circuitmux_assert_okay_paranoid(cmux); } /* @@ -1517,7 +1466,6 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, tor_assert(cmux); tor_assert(circ); - circuitmux_assert_okay_paranoid(cmux); if (n_cells == 0) return; @@ -1568,8 +1516,6 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, --(cmux->n_active_circuits); circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction); } - - circuitmux_assert_okay_paranoid(cmux); } /** @@ -1592,282 +1538,6 @@ circuitmux_notify_xmit_destroy(circuitmux_t *cmux) I64_PRINTF_ARG(global_destroy_ctr)); } -/* - * Circuitmux consistency checking assertions - */ - -/** - * Check that circuitmux data structures are consistent and fail with an - * assert if not. - */ - -void -circuitmux_assert_okay(circuitmux_t *cmux) -{ - tor_assert(cmux); - - /* - * Pass 1: iterate the hash table; for each entry: - * a) Check that the circuit has this cmux for n_mux or p_mux - * b) If the cell_count is > 0, set the mark bit; otherwise clear it - * c) Also check activeness (cell_count > 0 should be active) - * d) Count the number of circuits, active circuits and queued cells - * and at the end check that they match the counters in the cmux. - * - * Pass 2: iterate the active circuits list; for each entry, - * make sure the circuit is attached to this mux and appears - * in the hash table. Make sure the mark bit is 1, and clear - * it in the hash table entry. Consistency-check the linked - * list pointers. - * - * Pass 3: iterate the hash table again; assert if any active circuits - * (mark bit set to 1) are discovered that weren't cleared in pass 2 - * (don't appear in the linked list). - */ - - circuitmux_assert_okay_pass_one(cmux); - circuitmux_assert_okay_pass_two(cmux); - circuitmux_assert_okay_pass_three(cmux); -} - -/** - * Do the first pass of circuitmux_assert_okay(); see the comment in that - * function. - */ - -static void -circuitmux_assert_okay_pass_one(circuitmux_t *cmux) -{ - chanid_circid_muxinfo_t **i = NULL; - uint64_t chan_id; - channel_t *chan; - circid_t circ_id; - circuit_t *circ; - or_circuit_t *or_circ; - circuit_t **next_p, **prev_p; - unsigned int n_circuits, n_active_circuits, n_cells; - - tor_assert(cmux); - tor_assert(cmux->chanid_circid_map); - - /* Reset the counters */ - n_circuits = n_active_circuits = n_cells = 0; - /* Start iterating the hash table */ - i = HT_START(chanid_circid_muxinfo_map, cmux->chanid_circid_map); - while (i) { - /* Assert that the hash table entry isn't null */ - tor_assert(*i); - - /* Get the channel and circuit id */ - chan_id = (*i)->chan_id; - circ_id = (*i)->circ_id; - - /* Find the channel and circuit, assert that they exist */ - chan = channel_find_by_global_id(chan_id); - tor_assert(chan); - circ = circuit_get_by_circid_channel_even_if_marked(circ_id, chan); - tor_assert(circ); - - /* Assert that we know which direction this is going */ - tor_assert((*i)->muxinfo.direction == CELL_DIRECTION_OUT || - (*i)->muxinfo.direction == CELL_DIRECTION_IN); - - if ((*i)->muxinfo.direction == CELL_DIRECTION_OUT) { - /* We should be n_mux on this circuit */ - tor_assert(cmux == circ->n_mux); - tor_assert(chan == circ->n_chan); - /* Get next and prev for next test */ - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - /* This should be an or_circuit_t and we should be p_mux */ - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(cmux == or_circ->p_mux); - tor_assert(chan == or_circ->p_chan); - /* Get next and prev for next test */ - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } - - /* - * Should this circuit be active? I.e., does the mux know about > 0 - * cells on it? - */ - const int circ_is_active = ((*i)->muxinfo.cell_count > 0); - - /* It should be in the linked list iff it's active */ - if (circ_is_active) { - /* Either we have a next link or we are the tail */ - tor_assert(*next_p || (circ == cmux->active_circuits_tail)); - /* Either we have a prev link or we are the head */ - tor_assert(*prev_p || (circ == cmux->active_circuits_head)); - /* Increment the active circuits counter */ - ++n_active_circuits; - } else { - /* Shouldn't be in list, so no next or prev link */ - tor_assert(!(*next_p)); - tor_assert(!(*prev_p)); - /* And can't be head or tail */ - tor_assert(circ != cmux->active_circuits_head); - tor_assert(circ != cmux->active_circuits_tail); - } - - /* Increment the circuits counter */ - ++n_circuits; - /* Adjust the cell counter */ - n_cells += (*i)->muxinfo.cell_count; - - /* Set the mark bit to circ_is_active */ - (*i)->muxinfo.mark = circ_is_active; - - /* Advance to the next entry */ - i = HT_NEXT(chanid_circid_muxinfo_map, cmux->chanid_circid_map, i); - } - - /* Now check the counters */ - tor_assert(n_cells == cmux->n_cells); - tor_assert(n_circuits == cmux->n_circuits); - tor_assert(n_active_circuits == cmux->n_active_circuits); -} - -/** - * Do the second pass of circuitmux_assert_okay(); see the comment in that - * function. - */ - -static void -circuitmux_assert_okay_pass_two(circuitmux_t *cmux) -{ - circuit_t *curr_circ, *prev_circ = NULL, *next_circ; - or_circuit_t *curr_or_circ; - uint64_t curr_chan_id; - circid_t curr_circ_id; - circuit_t **next_p, **prev_p; - channel_t *chan; - unsigned int n_active_circuits = 0; - chanid_circid_muxinfo_t search, *hashent = NULL; - - tor_assert(cmux); - tor_assert(cmux->chanid_circid_map); - - /* - * Walk the linked list of active circuits in cmux; keep track of the - * previous circuit seen for consistency checking purposes. Count them - * to make sure the number in the linked list matches - * cmux->n_active_circuits. - */ - curr_circ = cmux->active_circuits_head; - while (curr_circ) { - /* Reset some things */ - chan = NULL; - curr_or_circ = NULL; - next_circ = NULL; - next_p = prev_p = NULL; - cell_direction_t direction; - - /* Figure out if this is n_mux or p_mux */ - if (cmux == curr_circ->n_mux) { - /* Get next_p and prev_p */ - next_p = &(curr_circ->next_active_on_n_chan); - prev_p = &(curr_circ->prev_active_on_n_chan); - /* Get the channel */ - chan = curr_circ->n_chan; - /* Get the circuit id */ - curr_circ_id = curr_circ->n_circ_id; - /* Remember the direction */ - direction = CELL_DIRECTION_OUT; - } else { - /* We must be p_mux and this must be an or_circuit_t */ - curr_or_circ = TO_OR_CIRCUIT(curr_circ); - tor_assert(cmux == curr_or_circ->p_mux); - /* Get next_p and prev_p */ - next_p = &(curr_or_circ->next_active_on_p_chan); - prev_p = &(curr_or_circ->prev_active_on_p_chan); - /* Get the channel */ - chan = curr_or_circ->p_chan; - /* Get the circuit id */ - curr_circ_id = curr_or_circ->p_circ_id; - /* Remember the direction */ - direction = CELL_DIRECTION_IN; - } - - /* Assert that we got a channel and get the channel ID */ - tor_assert(chan); - curr_chan_id = chan->global_identifier; - - /* Assert that prev_p points to last circuit we saw */ - tor_assert(*prev_p == prev_circ); - /* If that's NULL, assert that we are the head */ - if (!(*prev_p)) tor_assert(curr_circ == cmux->active_circuits_head); - - /* Get the next circuit */ - next_circ = *next_p; - /* If it's NULL, assert that we are the tail */ - if (!(*next_p)) tor_assert(curr_circ == cmux->active_circuits_tail); - - /* Now find the hash table entry for this circuit */ - search.chan_id = curr_chan_id; - search.circ_id = curr_circ_id; - hashent = HT_FIND(chanid_circid_muxinfo_map, cmux->chanid_circid_map, - &search); - - /* Assert that we have one */ - tor_assert(hashent); - - /* Assert that the direction matches */ - tor_assert(direction == hashent->muxinfo.direction); - - /* Assert that the hash entry got marked in pass one */ - tor_assert(hashent->muxinfo.mark); - - /* Clear the mark */ - hashent->muxinfo.mark = 0; - - /* Increment the counter */ - ++n_active_circuits; - - /* Advance to the next active circuit and update prev_circ */ - prev_circ = curr_circ; - curr_circ = next_circ; - } - - /* Assert that the counter matches the cmux */ - tor_assert(n_active_circuits == cmux->n_active_circuits); -} - -/** - * Do the third pass of circuitmux_assert_okay(); see the comment in that - * function. - */ - -static void -circuitmux_assert_okay_pass_three(circuitmux_t *cmux) -{ - chanid_circid_muxinfo_t **i = NULL; - - tor_assert(cmux); - tor_assert(cmux->chanid_circid_map); - - /* Start iterating the hash table */ - i = HT_START(chanid_circid_muxinfo_map, cmux->chanid_circid_map); - - /* Advance through each entry */ - while (i) { - /* Assert that it isn't null */ - tor_assert(*i); - - /* - * Assert that this entry is not marked - i.e., that either we didn't - * think it should be active in pass one or we saw it in the active - * circuits linked list. - */ - tor_assert(!((*i)->muxinfo.mark)); - - /* Advance to the next entry */ - i = HT_NEXT(chanid_circid_muxinfo_map, cmux->chanid_circid_map, i); - } -} - /*DOCDOC */ void circuitmux_append_destroy_cell(channel_t *chan, diff --git a/src/or/relay.c b/src/or/relay.c index 506b7eccb..caf031f7f 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2397,13 +2397,6 @@ circuit_consider_sending_sendme(circuit_t *circ, crypt_path_t *layer_hint) } } -#ifdef ACTIVE_CIRCUITS_PARANOIA -#define assert_cmux_ok_paranoid(chan) \ - assert_circuit_mux_okay(chan) -#else -#define assert_cmux_ok_paranoid(chan) -#endif /* defined(ACTIVE_CIRCUITS_PARANOIA) */ - /** The total number of cells we have allocated. */ static size_t total_cells_allocated = 0; @@ -2691,16 +2684,12 @@ update_circuit_on_cmux_(circuit_t *circ, cell_direction_t direction, } tor_assert(circuitmux_attached_circuit_direction(cmux, circ) == direction); - assert_cmux_ok_paranoid(chan); - /* Update the number of cells we have for the circuit mux */ if (direction == CELL_DIRECTION_OUT) { circuitmux_set_num_cells(cmux, circ, circ->n_chan_cells.n); } else { circuitmux_set_num_cells(cmux, circ, or_circ->p_chan_cells.n); } - - assert_cmux_ok_paranoid(chan); } /** Remove all circuits from the cmux on chan. @@ -2845,7 +2834,6 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) } /* If it returns NULL, no cells left to send */ if (!circ) break; - assert_cmux_ok_paranoid(chan); if (circ->n_chan == chan) { queue = &circ->n_chan_cells; @@ -2949,8 +2937,6 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) } /* Okay, we're done sending now */ - assert_cmux_ok_paranoid(chan); - return n_flushed; } @@ -3101,17 +3087,6 @@ circuit_clear_cell_queue(circuit_t *circ, channel_t *chan) update_circuit_on_cmux(circ, direction); } -/** Fail with an assert if the circuit mux on chan is corrupt - */ -void -assert_circuit_mux_okay(channel_t *chan) -{ - tor_assert(chan); - tor_assert(chan->cmux); - - circuitmux_assert_okay(chan->cmux); -} - /** Return 1 if we shouldn't restart reading on this circuit, even if * we get a SENDME. Else return 0. */ diff --git a/src/or/relay.h b/src/or/relay.h index f0fa7e987..ecc67e0b3 100644 --- a/src/or/relay.h +++ b/src/or/relay.h @@ -76,7 +76,6 @@ void destroy_cell_queue_append(destroy_cell_queue_t *queue, void channel_unlink_all_circuits(channel_t *chan, smartlist_t *detached_out); MOCK_DECL(int, channel_flush_from_first_active_circuit, (channel_t *chan, int max)); -void assert_circuit_mux_okay(channel_t *chan); void update_circuit_on_cmux_(circuit_t *circ, cell_direction_t direction, const char *file, int lineno); #define update_circuit_on_cmux(circ, direction) \ From c235c32bbcd33351d0d720fe2fb3dbac6369d12c Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 14:23:36 -0500 Subject: [PATCH 4/7] cmux: Remove round-robin circuit policy Since 0.2.4, tor uses EWMA circuit policy to prioritize. The previous algorithm, round-robin, hasn't been used since then but was still used as a fallback. Now that EWMA is mandatory, remove that code entirely and enforce a cmux policy to be set. This is part of a circuitmux cleanup to improve performance and reduce complexity in the code. We'll be able to address future optimization with this work. Closes #25268 Signed-off-by: David Goulet --- src/or/circuitlist.c | 5 - src/or/circuitmux.c | 338 +++---------------------------------- src/or/or.h | 17 -- src/test/test_channel.c | 3 +- src/test/test_circuitmux.c | 2 + 5 files changed, 26 insertions(+), 339 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 29ad9a8ee..00726ca98 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -403,9 +403,6 @@ circuit_set_p_circid_chan(or_circuit_t *or_circ, circid_t id, circuit_set_circid_chan_helper(circ, CELL_DIRECTION_IN, id, chan); if (chan) { - tor_assert(bool_eq(or_circ->p_chan_cells.n, - or_circ->next_active_on_p_chan)); - chan->timestamp_last_had_circuits = approx_time(); } @@ -428,8 +425,6 @@ circuit_set_n_circid_chan(circuit_t *circ, circid_t id, circuit_set_circid_chan_helper(circ, CELL_DIRECTION_OUT, id, chan); if (chan) { - tor_assert(bool_eq(circ->n_chan_cells.n, circ->next_active_on_n_chan)); - chan->timestamp_last_had_circuits = approx_time(); } diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 6ff03ab79..e601ffa55 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -114,13 +114,6 @@ struct circuitmux_s { */ chanid_circid_muxinfo_map_t *chanid_circid_map; - /* - * Double-linked ring of circuits with queued cells waiting for room to - * free up on this connection's outbuf. Every time we pull cells from - * a circuit, we advance this pointer to the next circuit in the ring. - */ - struct circuit_t *active_circuits_head, *active_circuits_tail; - /** List of queued destroy cells */ destroy_cell_queue_t destroy_cell_queue; /** Boolean: True iff the last cell to circuitmux_get_first_active_circuit @@ -188,18 +181,9 @@ chanid_circid_entry_hash(chanid_circid_muxinfo_t *a); static chanid_circid_muxinfo_t * circuitmux_find_map_entry(circuitmux_t *cmux, circuit_t *circ); static void -circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); +circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ); static void -circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); -static inline void -circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); -static inline circuit_t ** -circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ); -static inline circuit_t ** -circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ); +circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ); /* Static global variables */ @@ -208,115 +192,6 @@ static int64_t global_destroy_ctr = 0; /* Function definitions */ -/** - * Linked list helpers - */ - -/** - * Move an active circuit to the tail of the cmux's active circuits list; - * used by circuitmux_notify_xmit_cells(). - */ - -static inline void -circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) -{ - circuit_t **next_p = NULL, **prev_p = NULL; - circuit_t **next_prev = NULL, **prev_next = NULL; - circuit_t **tail_next = NULL; - or_circuit_t *or_circ = NULL; - - tor_assert(cmux); - tor_assert(circ); - - /* Figure out our next_p and prev_p for this cmux/direction */ - if (direction) { - if (direction == CELL_DIRECTION_OUT) { - tor_assert(circ->n_mux == cmux); - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(or_circ->p_mux == cmux); - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } - } else { - if (circ->n_mux == cmux) { - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(or_circ->p_mux == cmux); - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } - } - tor_assert(next_p); - tor_assert(prev_p); - - /* Check if this really is an active circuit */ - if ((*next_p == NULL && *prev_p == NULL) && - !(circ == cmux->active_circuits_head || - circ == cmux->active_circuits_tail)) { - /* Not active, no-op */ - return; - } - - /* Check if this is already the tail */ - if (circ == cmux->active_circuits_tail) return; - - /* Okay, we have to move it; figure out next_prev and prev_next */ - if (*next_p) next_prev = circuitmux_prev_active_circ_p(cmux, *next_p); - if (*prev_p) prev_next = circuitmux_next_active_circ_p(cmux, *prev_p); - /* Adjust the previous node's next pointer, if any */ - if (prev_next) *prev_next = *next_p; - /* Otherwise, we were the head */ - else cmux->active_circuits_head = *next_p; - /* Adjust the next node's previous pointer, if any */ - if (next_prev) *next_prev = *prev_p; - /* We're out of the list; now re-attach at the tail */ - /* Adjust our next and prev pointers */ - *next_p = NULL; - *prev_p = cmux->active_circuits_tail; - /* Set the next pointer of the tail, or the head if none */ - if (cmux->active_circuits_tail) { - tail_next = circuitmux_next_active_circ_p(cmux, - cmux->active_circuits_tail); - *tail_next = circ; - } else { - cmux->active_circuits_head = circ; - } - /* Set the tail to this circuit */ - cmux->active_circuits_tail = circ; -} - -static inline circuit_t ** -circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ) -{ - tor_assert(cmux); - tor_assert(circ); - - if (circ->n_mux == cmux) return &(circ->next_active_on_n_chan); - else { - tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux); - return &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - } -} - -static inline circuit_t ** -circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ) -{ - tor_assert(cmux); - tor_assert(circ); - - if (circ->n_mux == cmux) return &(circ->prev_active_on_n_chan); - else { - tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux); - return &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - } -} - /** * Helper for chanid_circid_cell_count_map_t hash table: compare the channel * ID and circuit ID for a and b, and return less than, equal to, or greater @@ -412,7 +287,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) */ if (to_remove->muxinfo.cell_count > 0) { - circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_OUT); + circuitmux_make_circuit_inactive(cmux, circ); } /* Clear n_mux */ @@ -427,7 +302,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) */ if (to_remove->muxinfo.cell_count > 0) { - circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_IN); + circuitmux_make_circuit_inactive(cmux, circ); } /* @@ -978,10 +853,10 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, */ if (hashent->muxinfo.cell_count > 0 && cell_count == 0) { --(cmux->n_active_circuits); - circuitmux_make_circuit_inactive(cmux, circ, direction); + circuitmux_make_circuit_inactive(cmux, circ); } else if (hashent->muxinfo.cell_count == 0 && cell_count > 0) { ++(cmux->n_active_circuits); - circuitmux_make_circuit_active(cmux, circ, direction); + circuitmux_make_circuit_active(cmux, circ); } cmux->n_cells -= hashent->muxinfo.cell_count; cmux->n_cells += cell_count; @@ -1029,20 +904,11 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, if (direction == CELL_DIRECTION_OUT) circ->n_mux = cmux; else TO_OR_CIRCUIT(circ)->p_mux = cmux; - /* Make sure the next/prev pointers are NULL */ - if (direction == CELL_DIRECTION_OUT) { - circ->next_active_on_n_chan = NULL; - circ->prev_active_on_n_chan = NULL; - } else { - TO_OR_CIRCUIT(circ)->next_active_on_p_chan = NULL; - TO_OR_CIRCUIT(circ)->prev_active_on_p_chan = NULL; - } - /* Update counters */ ++(cmux->n_circuits); if (cell_count > 0) { ++(cmux->n_active_circuits); - circuitmux_make_circuit_active(cmux, circ, direction); + circuitmux_make_circuit_active(cmux, circ); } cmux->n_cells += cell_count; } @@ -1106,7 +972,7 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) if (hashent->muxinfo.cell_count > 0) { --(cmux->n_active_circuits); /* This does policy notifies, so comes before freeing policy data */ - circuitmux_make_circuit_inactive(cmux, circ, last_searched_direction); + circuitmux_make_circuit_inactive(cmux, circ); } cmux->n_cells -= hashent->muxinfo.cell_count; @@ -1143,81 +1009,16 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) */ static void -circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) +circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ) { - circuit_t **next_active = NULL, **prev_active = NULL, **next_prev = NULL; - circuitmux_t *circuit_cmux = NULL; - chanid_circid_muxinfo_t *hashent = NULL; - channel_t *chan = NULL; - circid_t circ_id; - int already_active; - tor_assert(cmux); + tor_assert(cmux->policy); tor_assert(circ); - tor_assert(direction == CELL_DIRECTION_OUT || - direction == CELL_DIRECTION_IN); - - /* Get the right set of active list links for this direction */ - if (direction == CELL_DIRECTION_OUT) { - next_active = &(circ->next_active_on_n_chan); - prev_active = &(circ->prev_active_on_n_chan); - circuit_cmux = circ->n_mux; - chan = circ->n_chan; - circ_id = circ->n_circ_id; - } else { - next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux; - chan = TO_OR_CIRCUIT(circ)->p_chan; - circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; - } - - /* Assert that it is attached to this mux and a channel */ - tor_assert(cmux == circuit_cmux); - tor_assert(chan != NULL); - - /* - * Check if the circuit really was inactive; if it's active, at least one - * of the next_active and prev_active pointers will not be NULL, or this - * circuit will be either the head or tail of the list for this cmux. - */ - already_active = (*prev_active != NULL || *next_active != NULL || - cmux->active_circuits_head == circ || - cmux->active_circuits_tail == circ); - - /* If we're already active, log a warning and finish */ - if (already_active) { - log_warn(LD_CIRC, - "Circuit %u on channel " U64_FORMAT " was already active", - (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier)); - return; - } - - /* - * This is going at the head of the list; if the old head is not NULL, - * then its prev pointer should point to this. - */ - *next_active = cmux->active_circuits_head; /* Next is old head */ - *prev_active = NULL; /* Prev is NULL (this will be the head) */ - if (cmux->active_circuits_head) { - /* The list had an old head; update its prev pointer */ - next_prev = - circuitmux_prev_active_circ_p(cmux, cmux->active_circuits_head); - tor_assert(next_prev); - *next_prev = circ; - } else { - /* The list was empty; this becomes the tail as well */ - cmux->active_circuits_tail = circ; - } - /* This becomes the new head of the list */ - cmux->active_circuits_head = circ; /* Policy-specific notification */ - if (cmux->policy && - cmux->policy->notify_circ_active) { + if (cmux->policy->notify_circ_active) { /* Okay, we need to check the circuit for policy data now */ - hashent = circuitmux_find_map_entry(cmux, circ); + chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); /* We should have found something */ tor_assert(hashent); /* Notify */ @@ -1232,99 +1033,16 @@ circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, */ static void -circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) +circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ) { - circuit_t **next_active = NULL, **prev_active = NULL; - circuit_t **next_prev = NULL, **prev_next = NULL; - circuitmux_t *circuit_cmux = NULL; - chanid_circid_muxinfo_t *hashent = NULL; - channel_t *chan = NULL; - circid_t circ_id; - int already_inactive; - tor_assert(cmux); + tor_assert(cmux->policy); tor_assert(circ); - tor_assert(direction == CELL_DIRECTION_OUT || - direction == CELL_DIRECTION_IN); - - /* Get the right set of active list links for this direction */ - if (direction == CELL_DIRECTION_OUT) { - next_active = &(circ->next_active_on_n_chan); - prev_active = &(circ->prev_active_on_n_chan); - circuit_cmux = circ->n_mux; - chan = circ->n_chan; - circ_id = circ->n_circ_id; - } else { - next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux; - chan = TO_OR_CIRCUIT(circ)->p_chan; - circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; - } - - /* Assert that it is attached to this mux and a channel */ - tor_assert(cmux == circuit_cmux); - tor_assert(chan != NULL); - - /* - * Check if the circuit really was active; if it's inactive, the - * next_active and prev_active pointers will be NULL and this circuit - * will not be the head or tail of the list for this cmux. - */ - already_inactive = (*prev_active == NULL && *next_active == NULL && - cmux->active_circuits_head != circ && - cmux->active_circuits_tail != circ); - - /* If we're already inactive, log a warning and finish */ - if (already_inactive) { - log_warn(LD_CIRC, - "Circuit %d on channel " U64_FORMAT " was already inactive", - (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier)); - return; - } - - /* Remove from the list; first get next_prev and prev_next */ - if (*next_active) { - /* - * If there's a next circuit, its previous circuit becomes this - * circuit's previous circuit. - */ - next_prev = circuitmux_prev_active_circ_p(cmux, *next_active); - } else { - /* Else, the tail becomes this circuit's previous circuit */ - next_prev = &(cmux->active_circuits_tail); - } - - /* Got next_prev, now prev_next */ - if (*prev_active) { - /* - * If there's a previous circuit, its next circuit becomes this circuit's - * next circuit. - */ - prev_next = circuitmux_next_active_circ_p(cmux, *prev_active); - } else { - /* Else, the head becomes this circuit's next circuit */ - prev_next = &(cmux->active_circuits_head); - } - - /* Assert that we got sensible values for the next/prev pointers */ - tor_assert(next_prev != NULL); - tor_assert(prev_next != NULL); - - /* Update the next/prev pointers - this removes circ from the list */ - *next_prev = *prev_active; - *prev_next = *next_active; - - /* Now null out prev_active/next_active */ - *prev_active = NULL; - *next_active = NULL; /* Policy-specific notification */ - if (cmux->policy && - cmux->policy->notify_circ_inactive) { + if (cmux->policy->notify_circ_inactive) { /* Okay, we need to check the circuit for policy data now */ - hashent = circuitmux_find_map_entry(cmux, circ); + chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); /* We should have found something */ tor_assert(hashent); /* Notify */ @@ -1383,12 +1101,12 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, if (hashent->muxinfo.cell_count > 0 && n_cells == 0) { --(cmux->n_active_circuits); hashent->muxinfo.cell_count = n_cells; - circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_inactive(cmux, circ); /* Is the old cell count == 0 and the new cell count > 0 ? */ } else if (hashent->muxinfo.cell_count == 0 && n_cells > 0) { ++(cmux->n_active_circuits); hashent->muxinfo.cell_count = n_cells; - circuitmux_make_circuit_active(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_active(cmux, circ); } else { hashent->muxinfo.cell_count = n_cells; } @@ -1417,6 +1135,9 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux, circuit_t *circ = NULL; tor_assert(cmux); + tor_assert(cmux->policy); + /* This callback is mandatory. */ + tor_assert(cmux->policy->pick_active_circuit); tor_assert(destroy_queue_out); *destroy_queue_out = NULL; @@ -1435,14 +1156,7 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux, /* We also must have a cell available for this to be the case */ tor_assert(cmux->n_cells > 0); /* Do we have a policy-provided circuit selector? */ - if (cmux->policy && cmux->policy->pick_active_circuit) { - circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data); - } - /* Fall back on the head of the active circuits list */ - if (!circ) { - tor_assert(cmux->active_circuits_head); - circ = cmux->active_circuits_head; - } + circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data); cmux->last_cell_was_destroy = 0; } else { tor_assert(cmux->n_cells == 0); @@ -1492,12 +1206,6 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, /* Adjust the mux cell counter */ cmux->n_cells -= n_cells; - /* If we aren't making it inactive later, move it to the tail of the list */ - if (!becomes_inactive) { - circuitmux_move_active_circ_to_tail(cmux, circ, - hashent->muxinfo.direction); - } - /* * We call notify_xmit_cells() before making the circuit inactive if needed, * so the policy can always count on this coming in on an active circuit. @@ -1514,7 +1222,7 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, */ if (becomes_inactive) { --(cmux->n_active_circuits); - circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_inactive(cmux, circ); } } diff --git a/src/or/or.h b/src/or/or.h index 03efdd1d8..4d3a11849 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3179,15 +3179,6 @@ typedef struct circuit_t { /** Index in smartlist of all circuits (global_circuitlist). */ int global_circuitlist_idx; - /** Next circuit in the doubly-linked ring of circuits waiting to add - * cells to n_conn. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *next_active_on_n_chan; - /** Previous circuit in the doubly-linked ring of circuits waiting to add - * cells to n_conn. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *prev_active_on_n_chan; - /** Various statistics about cells being added to or removed from this * circuit's queues; used only if CELL_STATS events are enabled and * cleared after being sent to control port. */ @@ -3467,14 +3458,6 @@ struct onion_queue_t; typedef struct or_circuit_t { circuit_t base_; - /** Next circuit in the doubly-linked ring of circuits waiting to add - * cells to p_chan. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *next_active_on_p_chan; - /** Previous circuit in the doubly-linked ring of circuits waiting to add - * cells to p_chan. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *prev_active_on_p_chan; /** Pointer to an entry on the onion queue, if this circuit is waiting for a * chance to give an onionskin to a cpuworker. Used only in onion.c */ struct onion_queue_t *onionqueue_entry; diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 392759768..812ec6c1a 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -281,6 +281,7 @@ new_fake_channel(void) chan->state = CHANNEL_STATE_OPEN; chan->cmux = circuitmux_alloc(); + circuitmux_set_policy(chan->cmux, &ewma_policy); return chan; } @@ -582,8 +583,6 @@ test_channel_outbound_cell(void *arg) * to the channel's cmux as well. */ circuit_set_n_circid_chan(TO_CIRCUIT(circ), 42, chan); tt_int_op(channel_num_circuits(chan), OP_EQ, 1); - tt_assert(!TO_CIRCUIT(circ)->next_active_on_n_chan); - tt_assert(!TO_CIRCUIT(circ)->prev_active_on_n_chan); /* Test the cmux state. */ tt_ptr_op(TO_CIRCUIT(circ)->n_mux, OP_EQ, chan->cmux); tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)), diff --git a/src/test/test_circuitmux.c b/src/test/test_circuitmux.c index 854f72505..75b7a0ea4 100644 --- a/src/test/test_circuitmux.c +++ b/src/test/test_circuitmux.c @@ -7,6 +7,7 @@ #include "or.h" #include "channel.h" #include "circuitmux.h" +#include "circuitmux_ewma.h" #include "relay.h" #include "scheduler.h" #include "test.h" @@ -45,6 +46,7 @@ test_cmux_destroy_cell_queue(void *arg) cmux = circuitmux_alloc(); tt_assert(cmux); ch = new_fake_channel(); + circuitmux_set_policy(cmux, &ewma_policy); ch->has_queued_writes = has_queued_writes; ch->wide_circ_ids = 1; From e19cd38f08a67ed6bfb6f9ab174c134c41885af4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 14:34:53 -0500 Subject: [PATCH 5/7] cmux: Always use the cmux policy Remove the checks on cmux->policy since it should always be set. Signed-off-by: David Goulet --- src/or/circuitmux.c | 10 ++++------ src/test/test_circuitlist.c | 1 + 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index e601ffa55..f9f5faa05 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -458,9 +458,7 @@ circuitmux_clear_policy(circuitmux_t *cmux) tor_assert(cmux); /* Internally, this is just setting policy to NULL */ - if (cmux->policy) { - circuitmux_set_policy(cmux, NULL); - } + circuitmux_set_policy(cmux, NULL); } /** @@ -884,7 +882,7 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, hashent->muxinfo.cell_count = cell_count; hashent->muxinfo.direction = direction; /* Allocate policy specific circuit data if we need it */ - if (cmux->policy && cmux->policy->alloc_circ_data) { + if (cmux->policy->alloc_circ_data) { /* Assert that we have the means to free policy-specific data */ tor_assert(cmux->policy->free_circ_data); /* Allocate it */ @@ -1085,7 +1083,7 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, cmux->n_cells += n_cells; /* Do we need to notify a cmux policy? */ - if (cmux->policy && cmux->policy->notify_set_n_cells) { + if (cmux->policy->notify_set_n_cells) { /* Call notify_set_n_cells */ cmux->policy->notify_set_n_cells(cmux, cmux->policy_data, @@ -1210,7 +1208,7 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, * We call notify_xmit_cells() before making the circuit inactive if needed, * so the policy can always count on this coming in on an active circuit. */ - if (cmux->policy && cmux->policy->notify_xmit_cells) { + if (cmux->policy->notify_xmit_cells) { cmux->policy->notify_xmit_cells(cmux, cmux->policy_data, circ, hashent->muxinfo.policy_data, n_cells); diff --git a/src/test/test_circuitlist.c b/src/test/test_circuitlist.c index d170009a9..3794ffc2c 100644 --- a/src/test/test_circuitlist.c +++ b/src/test/test_circuitlist.c @@ -9,6 +9,7 @@ #include "channel.h" #include "circuitbuild.h" #include "circuitlist.h" +#include "circuitmux_ewma.h" #include "hs_circuitmap.h" #include "test.h" #include "log_test_helpers.h" From 779eded6bb227c0f315d59921636185f20b1d3e6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 15 Feb 2018 14:53:10 -0500 Subject: [PATCH 6/7] man: Update the CircuitPriorityHalflife entry The behavior has changed slightly in the previous commits. Signed-off-by: David Goulet --- doc/tor.1.txt | 20 +++++++++----------- src/or/config.c | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 2c8135ff0..210ae3804 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -238,7 +238,7 @@ GENERAL OPTIONS [[RelayBandwidthBurst]] **RelayBandwidthBurst** __N__ **bytes**|**KBytes**|**MBytes**|**GBytes**|**TBytes**|**KBits**|**MBits**|**GBits**|**TBits**:: If not 0, limit the maximum token bucket size (also known as the burst) for \_relayed traffic_ to the given number of bytes in each direction. - They do not include directory fetches by the relay (from authority + They do not include directory fetches by the relay (from authority or other relays), because that is considered "client" activity. (Default: 0) [[PerConnBWRate]] **PerConnBWRate** __N__ **bytes**|**KBytes**|**MBytes**|**GBytes**|**TBytes**|**KBits**|**MBits**|**GBits**|**TBits**:: @@ -777,17 +777,15 @@ GENERAL OPTIONS This is useful when running on flash memory or other media that support only a limited number of writes. (Default: 0) -[[CircuitPriorityHalflife]] **CircuitPriorityHalflife** __NUM1__:: +[[CircuitPriorityHalflife]] **CircuitPriorityHalflife** __NUM__:: If this value is set, we override the default algorithm for choosing which - circuit's cell to deliver or relay next. When the value is 0, we - round-robin between the active circuits on a connection, delivering one - cell from each in turn. When the value is positive, we prefer delivering - cells from whichever connection has the lowest weighted cell count, where - cells are weighted exponentially according to the supplied - CircuitPriorityHalflife value (in seconds). If this option is not set at - all, we use the behavior recommended in the current consensus - networkstatus. This is an advanced option; you generally shouldn't have - to mess with it. (Default: not set) + circuit's cell to deliver or relay next. It is delivered first to the + circuit that has the lowest weighted cell count, where cells are weighted + exponentially according to this value (in seconds). If the value is -1, it + is taken from the consensus if possible else it will fallback to the + default value of 30. Minimum: 1, Maximum: 2147483647. This can be defined + as a float value. This is an advanced option; you generally shouldn't have + to mess with it. (Default: -1) [[CountPrivateBandwidth]] **CountPrivateBandwidth** **0**|**1**:: If this option is set, then Tor's rate-limiting applies not only to diff --git a/src/or/config.c b/src/or/config.c index 107f9ac5f..79b44b36e 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -263,7 +263,7 @@ static config_var_t option_vars_[] = { OBSOLETE("CircuitIdleTimeout"), V(CircuitsAvailableTimeout, INTERVAL, "0"), V(CircuitStreamTimeout, INTERVAL, "0"), - V(CircuitPriorityHalflife, DOUBLE, "-100.0"), /*negative:'Use default'*/ + V(CircuitPriorityHalflife, DOUBLE, "-1.0"), /*negative:'Use default'*/ V(ClientDNSRejectInternalAddresses, BOOL,"1"), V(ClientOnly, BOOL, "0"), V(ClientPreferIPv6ORPort, AUTOBOOL, "auto"), From 4449c9e8fe7b11ec816762c8b37b1ed70d873d4a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 19 Mar 2018 06:00:00 -0400 Subject: [PATCH 7/7] add a changes file for 25268 --- changes/ticket25268 | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/ticket25268 diff --git a/changes/ticket25268 b/changes/ticket25268 new file mode 100644 index 000000000..e444984dc --- /dev/null +++ b/changes/ticket25268 @@ -0,0 +1,7 @@ + o Removed features: + - The old "round-robin" circuit multiplexer (circuitmux) + implementation has been removed, along with a fairly large set of + code that existed to support it. It has not been the default + circuitmux since we introduced the "EWMA" circuitmux in 0.2.4.x, + but it still required an unreasonable amount of memory and CPU. + Closes ticket 25268.