commit 6b1dba214db3058b143bbb4d4c4bdfee32d100f1 Author: David Goulet dgoulet@torproject.org Date: Thu Feb 15 13:45:21 2018 -0500
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 dgoulet@torproject.org --- 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 @@ -2109,21 +2109,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. * * This gets called periodically from run_scheduled_events() in main.c; 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 <b>options</b> */ 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