From 9a77743b7b2e657cb8d9fd413d53cb9e3b8e00ca Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 11:49:31 -0700 Subject: [PATCH 01/10] Fix non-live condition checks. Rechecking the timeout condition was foolish, because it is checked on the same codepath. It was also wrong, because we didn't round. Also, the liveness check itself should be <, and not <=, because we only have 1 second resolution. --- src/or/circuitbuild.c | 13 +++++-------- src/or/circuituse.c | 12 +++++------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index be435b950a..4f4d9c70d2 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -986,15 +986,8 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, /* * Check if this is a timeout that was for a circuit that spent its * entire existence during a time where we have had no network activity. - * - * Also double check that it is a valid timeout after we have possibly - * just recently reset cbt->close_ms. - * - * We use close_ms here because timeouts aren't actually counted as timeouts - * until close_ms elapses. */ - if (cbt->liveness.network_last_live <= start_time && - start_time <= (now - cbt->close_ms/1000.0)) { + if (cbt->liveness.network_last_live < start_time) { if (did_onehop) { char last_live_buf[ISO_TIME_LEN+1]; char start_time_buf[ISO_TIME_LEN+1]; @@ -1009,6 +1002,9 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, now_buf); } cbt->liveness.nonlive_timeouts++; + log_info(LD_CIRC, + "Got non-live timeout. Current count is: %d", + cbt->liveness.nonlive_timeouts); } } @@ -1038,6 +1034,7 @@ circuit_build_times_network_check_live(circuit_build_times_t *cbt) return 0; } else if (cbt->liveness.nonlive_timeouts >= CBT_NETWORK_NONLIVE_TIMEOUT_COUNT) { + // XXX: We won't ever conclude the network is flaky here for poor arma... if (cbt->timeout_ms < circuit_build_times_get_initial_timeout()) { log_notice(LD_CIRC, "Network is flaky. No activity for %ld seconds. " diff --git a/src/or/circuituse.c b/src/or/circuituse.c index ce03500f34..6ecb925230 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -34,8 +34,6 @@ extern circuit_t *global_circuitlist; /* from circuitlist.c */ static void circuit_expire_old_circuits_clientside(time_t now); static void circuit_increment_failure_count(void); -long int lround(double x); - /** Return 1 if circ could be returned by circuit_get_best(). * Else return 0. */ @@ -282,11 +280,11 @@ circuit_expire_building(time_t now) circuit_t *victim, *next_circ = global_circuitlist; /* circ_times.timeout is BUILD_TIMEOUT_INITIAL_VALUE if we haven't * decided on a customized one yet */ - time_t general_cutoff = now - lround(circ_times.timeout_ms/1000); - time_t begindir_cutoff = now - lround(circ_times.timeout_ms/2000); - time_t fourhop_cutoff = now - lround(4*circ_times.timeout_ms/3000); - time_t cannibalize_cutoff = now - lround(circ_times.timeout_ms/2000); - time_t close_cutoff = now - lround(circ_times.close_ms/1000); + time_t general_cutoff = now - tor_lround(circ_times.timeout_ms/1000); + time_t begindir_cutoff = now - tor_lround(circ_times.timeout_ms/2000); + time_t fourhop_cutoff = now - tor_lround(4*circ_times.timeout_ms/3000); + time_t cannibalize_cutoff = now - tor_lround(circ_times.timeout_ms/2000); + time_t close_cutoff = now - tor_lround(circ_times.close_ms/1000); time_t introcirc_cutoff = begindir_cutoff; cpath_build_state_t *build_state; From 0744a175afa559435bd0e3cdb53891282469e0ee Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 08:47:35 -0700 Subject: [PATCH 02/10] Fix state checks on liveness handling. If we really want all this complexity for these stages here, we need to handle it better for people with large timeouts. It should probably go away, though. --- src/or/circuitbuild.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 4f4d9c70d2..81b63fe64d 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1034,17 +1034,25 @@ circuit_build_times_network_check_live(circuit_build_times_t *cbt) return 0; } else if (cbt->liveness.nonlive_timeouts >= CBT_NETWORK_NONLIVE_TIMEOUT_COUNT) { - // XXX: We won't ever conclude the network is flaky here for poor arma... - if (cbt->timeout_ms < circuit_build_times_get_initial_timeout()) { + if (cbt->liveness.suspended_timeout <= 0) { + cbt->liveness.suspended_timeout = cbt->timeout_ms; + cbt->liveness.suspended_close_timeout = cbt->close_ms; + + if (cbt->timeout_ms < circuit_build_times_get_initial_timeout()) + cbt->timeout_ms = circuit_build_times_get_initial_timeout(); + else + cbt->timeout_ms *= 2; + + if (cbt->close_ms < circuit_build_times_get_initial_timeout()) + cbt->close_ms = circuit_build_times_get_initial_timeout(); + else + cbt->close_ms *= 2; + log_notice(LD_CIRC, "Network is flaky. No activity for %ld seconds. " "Temporarily raising timeout to %lds.", (long int)(now - cbt->liveness.network_last_live), - tor_lround(circuit_build_times_get_initial_timeout()/1000)); - cbt->liveness.suspended_timeout = cbt->timeout_ms; - cbt->liveness.suspended_close_timeout = cbt->close_ms; - cbt->close_ms = cbt->timeout_ms - = circuit_build_times_get_initial_timeout(); + tor_lround(cbt->timeout_ms/1000)); control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_SUSPENDED); } From 11910cf5b32edfd6b900386d37bb69c7592174c1 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 08:55:11 -0700 Subject: [PATCH 03/10] Do away with the complexity of the network liveness detection. We really should ignore any timeouts that have *no* network activity for their entire measured lifetime, now that we have the 95th percentile measurement changes. Usually this is up to a minute, even on fast connections. --- src/or/circuitbuild.c | 71 +++++++++++++------------------------------ src/or/or.h | 32 +------------------ src/test/test.c | 26 ++++------------ 3 files changed, 28 insertions(+), 101 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 81b63fe64d..234765bcaf 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -306,6 +306,7 @@ circuit_build_times_init(circuit_build_times_t *cbt) control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_RESET); } +#if 0 /** * Rewind our build time history by n positions. */ @@ -332,6 +333,7 @@ circuit_build_times_rewind_history(circuit_build_times_t *cbt, int n) "Rewound history by %d places. Current index: %d. " "Total: %d", n, cbt->build_times_idx, cbt->total_build_times); } +#endif /** * Add a new build time value time to the set of build times. Time @@ -941,8 +943,16 @@ circuit_build_times_needs_circuits_now(circuit_build_times_t *cbt) void circuit_build_times_network_is_live(circuit_build_times_t *cbt) { - cbt->liveness.network_last_live = approx_time(); - cbt->liveness.nonlive_discarded = 0; + time_t now = approx_time(); + if (cbt->liveness.nonlive_timeouts > 0) { + log_notice(LD_CIRC, + "Tor now sees network activity. Restoring circuit build " + "timeout recording. Network was down for %ld seconds " + "during %d circuit attempts.", + (long int)now - cbt->liveness.network_last_live, + cbt->liveness.nonlive_timeouts); + } + cbt->liveness.network_last_live = now; cbt->liveness.nonlive_timeouts = 0; } @@ -1002,9 +1012,16 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, now_buf); } cbt->liveness.nonlive_timeouts++; - log_info(LD_CIRC, + if (cbt->liveness.nonlive_timeouts == 1) { + log_notice(LD_CIRC, + "Tor has not observed any network activity for the past %ld " + "seconds. Disabling circuit build timeout code.", + (long int)now - cbt->liveness.network_last_live); + } else { + log_info(LD_CIRC, "Got non-live timeout. Current count is: %d", cbt->liveness.nonlive_timeouts); + } } } @@ -1018,54 +1035,8 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, int circuit_build_times_network_check_live(circuit_build_times_t *cbt) { - time_t now = approx_time(); - if (cbt->liveness.nonlive_timeouts >= CBT_NETWORK_NONLIVE_DISCARD_COUNT) { - if (!cbt->liveness.nonlive_discarded) { - cbt->liveness.nonlive_discarded = 1; - log_notice(LD_CIRC, "Network is no longer live (too many recent " - "circuit timeouts). Dead for %ld seconds.", - (long int)(now - cbt->liveness.network_last_live)); - /* Only discard NETWORK_NONLIVE_TIMEOUT_COUNT-1 because we stopped - * counting after that */ - circuit_build_times_rewind_history(cbt, - CBT_NETWORK_NONLIVE_TIMEOUT_COUNT-1); - control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_DISCARD); - } + if (cbt->liveness.nonlive_timeouts > 0) { return 0; - } else if (cbt->liveness.nonlive_timeouts >= - CBT_NETWORK_NONLIVE_TIMEOUT_COUNT) { - if (cbt->liveness.suspended_timeout <= 0) { - cbt->liveness.suspended_timeout = cbt->timeout_ms; - cbt->liveness.suspended_close_timeout = cbt->close_ms; - - if (cbt->timeout_ms < circuit_build_times_get_initial_timeout()) - cbt->timeout_ms = circuit_build_times_get_initial_timeout(); - else - cbt->timeout_ms *= 2; - - if (cbt->close_ms < circuit_build_times_get_initial_timeout()) - cbt->close_ms = circuit_build_times_get_initial_timeout(); - else - cbt->close_ms *= 2; - - log_notice(LD_CIRC, - "Network is flaky. No activity for %ld seconds. " - "Temporarily raising timeout to %lds.", - (long int)(now - cbt->liveness.network_last_live), - tor_lround(cbt->timeout_ms/1000)); - control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_SUSPENDED); - } - - return 0; - } else if (cbt->liveness.suspended_timeout > 0) { - log_notice(LD_CIRC, - "Network activity has resumed. " - "Resuming circuit timeout calculations."); - cbt->timeout_ms = cbt->liveness.suspended_timeout; - cbt->close_ms = cbt->liveness.suspended_close_timeout; - cbt->liveness.suspended_timeout = 0; - cbt->liveness.suspended_close_timeout = 0; - control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_RESUME); } return 1; diff --git a/src/or/or.h b/src/or/or.h index 6c398b7dcb..dc46684133 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2961,26 +2961,6 @@ typedef uint32_t build_time_t; /** Save state every 10 circuits */ #define CBT_SAVE_STATE_EVERY 10 -/* Circuit Build Timeout network liveness constants */ - -/** - * Have we received a cell in the last N circ attempts? - * - * This tells us when to temporarily switch back to - * BUILD_TIMEOUT_INITIAL_VALUE until we start getting cells, - * at which point we switch back to computing the timeout from - * our saved history. - */ -#define CBT_NETWORK_NONLIVE_TIMEOUT_COUNT 3 - -/** - * This tells us when to toss out the last streak of N timeouts. - * - * If instead we start getting cells, we switch back to computing the timeout - * from our saved history. - */ -#define CBT_NETWORK_NONLIVE_DISCARD_COUNT (CBT_NETWORK_NONLIVE_TIMEOUT_COUNT*2) - /* Circuit build times consensus parameters */ /** @@ -3021,9 +3001,7 @@ double circuit_build_times_quantile_cutoff(void); #define CBT_DEFAULT_TIMEOUT_INITIAL_VALUE (60*1000) int32_t circuit_build_times_initial_timeout(void); -#if CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT < 1 || \ - CBT_NETWORK_NONLIVE_DISCARD_COUNT < 1 || \ - CBT_NETWORK_NONLIVE_TIMEOUT_COUNT < 1 +#if CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT < 1 #error "RECENT_CIRCUITS is set too low." #endif @@ -3033,8 +3011,6 @@ typedef struct { time_t network_last_live; /** If the network is not live, how many timeouts has this caused? */ int nonlive_timeouts; - /** If the network is not live, have we yet discarded our history? */ - int nonlive_discarded; /** Circular array of circuits that have made it to the first hop. Slot is * 1 if circuit timed out, 0 if circuit succeeded */ int8_t *timeouts_after_firsthop; @@ -3042,12 +3018,6 @@ typedef struct { int num_recent_circs; /** Index into circular array. */ int after_firsthop_idx; - /** Timeout gathering is suspended if non-zero. The old timeout value - * is stored here in that case. */ - double suspended_timeout; - /** Timeout gathering is suspended if non-zero. The old close value - * is stored here in that case. */ - double suspended_close_timeout; } network_liveness_t; /** Structure for circuit build times history */ diff --git a/src/test/test.c b/src/test/test.c index d9528328b8..8d8c46fca2 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -499,28 +499,14 @@ test_circuit_timeout(void) build_times_idx = estimate.build_times_idx; total_build_times = estimate.total_build_times; - for (i = 0; i < CBT_NETWORK_NONLIVE_TIMEOUT_COUNT; i++) { - test_assert(circuit_build_times_network_check_live(&estimate)); - test_assert(circuit_build_times_network_check_live(&final)); - circuit_build_times_count_close(&estimate, 0, - (time_t)(approx_time()-estimate.close_ms/1000.0-1)); - circuit_build_times_count_close(&final, 0, - (time_t)(approx_time()-final.close_ms/1000.0-1)); - } + test_assert(circuit_build_times_network_check_live(&estimate)); + test_assert(circuit_build_times_network_check_live(&final)); - test_assert(!circuit_build_times_network_check_live(&estimate)); - test_assert(!circuit_build_times_network_check_live(&final)); - - for ( ; i < CBT_NETWORK_NONLIVE_DISCARD_COUNT; i++) { - circuit_build_times_count_close(&estimate, 0, - (time_t)(approx_time()-estimate.close_ms/1000.0-1)); - - if (i < CBT_NETWORK_NONLIVE_DISCARD_COUNT-1) { - circuit_build_times_count_close(&final, 0, - (time_t)(approx_time()-final.close_ms/1000.0-1)); - } - } + circuit_build_times_count_close(&estimate, 0, + (time_t)(approx_time()-estimate.close_ms/1000.0-1)); + circuit_build_times_count_close(&final, 0, + (time_t)(approx_time()-final.close_ms/1000.0-1)); test_assert(!circuit_build_times_network_check_live(&estimate)); test_assert(!circuit_build_times_network_check_live(&final)); From 4324bb1b213613b9fc304054ea31aecf50773ba3 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 10:06:31 -0700 Subject: [PATCH 04/10] Cap the circuit build timeout to the max time we've seen. Also, cap the measurement timeout to 2X the max we've seen. --- src/or/circuitbuild.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 234765bcaf..7a0a215768 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1204,6 +1204,7 @@ circuit_build_times_count_timeout(circuit_build_times_t *cbt, static int circuit_build_times_set_timeout_worker(circuit_build_times_t *cbt) { + build_time_t max_time; if (cbt->total_build_times < circuit_build_times_min_circs_to_observe()) { return 0; } @@ -1217,11 +1218,29 @@ circuit_build_times_set_timeout_worker(circuit_build_times_t *cbt) cbt->close_ms = circuit_build_times_calculate_timeout(cbt, circuit_build_times_close_quantile()); + max_time = circuit_build_times_max(cbt); + /* Sometimes really fast guard nodes give us such a steep curve * that this ends up being not that much greater than timeout_ms. * Make it be at least 1 min to handle this case. */ cbt->close_ms = MAX(cbt->close_ms, circuit_build_times_initial_timeout()); + if (cbt->timeout_ms > max_time) { + log_notice(LD_CIRC, + "Circuit build timeout of %dms is beyond the maximum build " + "time we have ever observed. Capping it to %dms.", + (int)cbt->timeout_ms, max_time); + cbt->timeout_ms = max_time; + } + + if (max_time < INT32_MAX/2 && cbt->close_ms > 2*max_time) { + log_notice(LD_CIRC, + "Circuit build measurement period of %dms is more than twice " + "the maximum build time we have ever observed. Capping it to " + "%dms.", (int)cbt->close_ms, 2*max_time); + cbt->close_ms = 2*max_time; + } + cbt->have_computed_timeout = 1; return 1; } From f1b0e4e4b4d238595b1694df201e995bee235960 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 09:15:08 -0700 Subject: [PATCH 05/10] Add changes file. --- changes/bug1772 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changes/bug1772 diff --git a/changes/bug1772 b/changes/bug1772 new file mode 100644 index 0000000000..9a4f809ee2 --- /dev/null +++ b/changes/bug1772 @@ -0,0 +1,11 @@ + o Minor bugfixes: + - Simplify the logic that causes us to decide if the network is unavailable + for purposes of recording circuit build times. If we receive no cells + whatsoever for the entire duration of a circuit's full measured lifetime, + the network is probably down. This should hopefully reduce some of the + cases where we see ridiculous circuit build timeouts for people with spotty + wireless connections. Fixes bug 1772; bugfix on 0.2.2.2-alpha. + - Prevent the circuit build timeout from becoming larger than the maximum + build time we have ever seen. Also, prevent the measurement time period + from becoming larger than twice that value. Fixes bug 1772; bugfix on + 0.2.2.2-alpha From 7f10707c42e69ef69395aecf7984d16107c78331 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 29 Sep 2010 18:01:22 -0400 Subject: [PATCH 06/10] refactor and recomment; no actual changes --- src/or/circuitbuild.c | 19 ++++++++++++------- src/or/circuitbuild.h | 1 + src/or/circuituse.c | 5 +++-- src/or/or.h | 5 +++-- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7a0a215768..5dfabcc97a 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -149,6 +149,14 @@ circuit_build_times_min_circs_to_observe(void) return num; } +/** Return true iff cbt has recorded enough build times that we + * want to start acting on the timeout it implies. */ +int +circuit_build_times_enough_to_compute(circuit_build_times_t *cbt) +{ + return cbt->total_build_times >= circuit_build_times_min_circs_to_observe(); +} + double circuit_build_times_quantile_cutoff(void) { @@ -292,8 +300,8 @@ circuit_build_times_reset(circuit_build_times_t *cbt) /** * Initialize the buildtimes structure for first use. * - * Sets the initial timeout value based to either the - * config setting or BUILD_TIMEOUT_INITIAL_VALUE. + * Sets the initial timeout values based on either the config setting, + * the consensus param, or the default (CBT_DEFAULT_TIMEOUT_INITIAL_VALUE). */ void circuit_build_times_init(circuit_build_times_t *cbt) @@ -918,9 +926,7 @@ int circuit_build_times_needs_circuits(circuit_build_times_t *cbt) { /* Return true if < MIN_CIRCUITS_TO_OBSERVE */ - if (cbt->total_build_times < circuit_build_times_min_circs_to_observe()) - return 1; - return 0; + return !circuit_build_times_enough_to_compute(cbt); } /** @@ -1205,9 +1211,8 @@ static int circuit_build_times_set_timeout_worker(circuit_build_times_t *cbt) { build_time_t max_time; - if (cbt->total_build_times < circuit_build_times_min_circs_to_observe()) { + if (!circuit_build_times_enough_to_compute(cbt)) return 0; - } if (!circuit_build_times_update_alpha(cbt)) return 0; diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 7cc5c2877a..9d666ecc76 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -79,6 +79,7 @@ void bridges_retry_all(void); void entry_guards_free_all(void); extern circuit_build_times_t circ_times; +int circuit_build_times_enough_to_compute(circuit_build_times_t *cbt); void circuit_build_times_update_state(circuit_build_times_t *cbt, or_state_t *state); int circuit_build_times_parse_state(circuit_build_times_t *cbt, diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 6ecb925230..66ee0c4353 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -278,8 +278,9 @@ void circuit_expire_building(time_t now) { circuit_t *victim, *next_circ = global_circuitlist; - /* circ_times.timeout is BUILD_TIMEOUT_INITIAL_VALUE if we haven't - * decided on a customized one yet */ + /* circ_times.timeout_ms and circ_times.close_ms are from + * circuit_build_times_get_initial_timeout() if we haven't computed + * custom timeouts yet */ time_t general_cutoff = now - tor_lround(circ_times.timeout_ms/1000); time_t begindir_cutoff = now - tor_lround(circ_times.timeout_ms/2000); time_t fourhop_cutoff = now - tor_lround(4*circ_times.timeout_ms/3000); diff --git a/src/or/or.h b/src/or/or.h index dc46684133..c90b0391a5 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2979,8 +2979,9 @@ typedef uint32_t build_time_t; * Maximum count of timeouts that finish the first hop in the past * RECENT_CIRCUITS before calculating a new timeout. * - * This tells us to abandon timeout history and set - * the timeout back to BUILD_TIMEOUT_INITIAL_VALUE. + * This tells us whether to abandon timeout history and set + * the timeout back to whatever circuit_build_times_get_initial_timeout() + * gives us. */ #define CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT (CBT_DEFAULT_RECENT_CIRCUITS*9/10) From ceb3d4d578f4ebb8d0d1247adf895ccbab7f72db Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 29 Sep 2010 18:05:10 -0400 Subject: [PATCH 07/10] no measurement circs if not enough build times In the first 100 circuits, our timeout_ms and close_ms are the same. So we shouldn't transition circuits to purpose CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT, since they will just timeout again next time we check. --- src/or/circuituse.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 66ee0c4353..18ef0c824b 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -384,7 +384,8 @@ circuit_expire_building(time_t now) continue; } - if (circuit_timeout_want_to_count_circ(TO_ORIGIN_CIRCUIT(victim))) { + if (circuit_timeout_want_to_count_circ(TO_ORIGIN_CIRCUIT(victim)) && + circuit_build_times_enough_to_compute(&circ_times)) { /* Circuits are allowed to last longer for measurement. * Switch their purpose and wait. */ if (victim->purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { From c8f731fabb7db3e785896cbc241fb4f4fb0b0ea9 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 19:35:40 -0700 Subject: [PATCH 08/10] Comment network liveness and change detection behavior. --- src/or/circuitbuild.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 5dfabcc97a..ec7093c345 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -941,7 +941,11 @@ circuit_build_times_needs_circuits_now(circuit_build_times_t *cbt) } /** - * Called to indicate that the network showed some signs of liveness. + * Called to indicate that the network showed some signs of liveness, + * which means that we recieved a cell. + * + * This is used by circuit_build_times_network_check_live() to decide + * if we should record the circuit build timeout or not. * * This function is called every time we receive a cell. Avoid * syscalls, events, and other high-intensity work. @@ -965,6 +969,10 @@ circuit_build_times_network_is_live(circuit_build_times_t *cbt) /** * Called to indicate that we completed a circuit. Because this circuit * succeeded, it doesn't count as a timeout-after-the-first-hop. + * + * This is used by circuit_build_times_network_check_changed() to determine + * if we had too many recent timeouts and need to reset our learned timeout + * to something higher. */ void circuit_build_times_network_circ_success(circuit_build_times_t *cbt) @@ -977,6 +985,10 @@ circuit_build_times_network_circ_success(circuit_build_times_t *cbt) /** * A circuit just timed out. If it failed after the first hop, record it * in our history for later deciding if the network speed has changed. + * + * This is used by circuit_build_times_network_check_changed() to determine + * if we had too many recent timeouts and need to reset our learned timeout + * to something higher. */ static void circuit_build_times_network_timeout(circuit_build_times_t *cbt, @@ -993,6 +1005,9 @@ circuit_build_times_network_timeout(circuit_build_times_t *cbt, * A circuit was just forcibly closed. If there has been no recent network * activity at all, but this circuit was launched back when we thought the * network was live, increment the number of "nonlive" circuit timeouts. + * + * This is used by circuit_build_times_network_check_live() to decide + * if we should record the circuit build timeout or not. */ static void circuit_build_times_network_close(circuit_build_times_t *cbt, @@ -1032,8 +1047,11 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, } /** - * Returns false if the network has not received a cell or tls handshake - * in the past NETWORK_NOTLIVE_TIMEOUT_COUNT circuits. + * When the network is not live, we do not record circuit build times. + * + * The network is considered not live if there has been at least one + * circuit build that began and ended (had its close_ms measurement + * period expire) since we last recieved a cell. * * Also has the side effect of rewinding the circuit time history * in the case of recent liveness changes. @@ -1051,7 +1069,8 @@ circuit_build_times_network_check_live(circuit_build_times_t *cbt) /** * Returns true if we have seen more than MAX_RECENT_TIMEOUT_COUNT of * the past RECENT_CIRCUITS time out after the first hop. Used to detect - * if the network connection has changed significantly. + * if the network connection has changed significantly, and if so, + * resets our circuit build timeout to the default. * * Also resets the entire timeout history in this case and causes us * to restart the process of building test circuits and estimating a From f2aa8f08cb6a5a258eadf05ca157ec43b975706a Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 29 Sep 2010 23:51:25 -0400 Subject: [PATCH 09/10] fix two casts --- src/or/circuitbuild.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index ec7093c345..542b82ac50 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -942,7 +942,7 @@ circuit_build_times_needs_circuits_now(circuit_build_times_t *cbt) /** * Called to indicate that the network showed some signs of liveness, - * which means that we recieved a cell. + * i.e. we received a cell. * * This is used by circuit_build_times_network_check_live() to decide * if we should record the circuit build timeout or not. @@ -957,9 +957,9 @@ circuit_build_times_network_is_live(circuit_build_times_t *cbt) if (cbt->liveness.nonlive_timeouts > 0) { log_notice(LD_CIRC, "Tor now sees network activity. Restoring circuit build " - "timeout recording. Network was down for %ld seconds " + "timeout recording. Network was down for %d seconds " "during %d circuit attempts.", - (long int)now - cbt->liveness.network_last_live, + (int)(now - cbt->liveness.network_last_live), cbt->liveness.nonlive_timeouts); } cbt->liveness.network_last_live = now; @@ -1035,9 +1035,9 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, cbt->liveness.nonlive_timeouts++; if (cbt->liveness.nonlive_timeouts == 1) { log_notice(LD_CIRC, - "Tor has not observed any network activity for the past %ld " + "Tor has not observed any network activity for the past %d " "seconds. Disabling circuit build timeout code.", - (long int)now - cbt->liveness.network_last_live); + (int)(now - cbt->liveness.network_last_live)); } else { log_info(LD_CIRC, "Got non-live timeout. Current count is: %d", @@ -1051,7 +1051,7 @@ circuit_build_times_network_close(circuit_build_times_t *cbt, * * The network is considered not live if there has been at least one * circuit build that began and ended (had its close_ms measurement - * period expire) since we last recieved a cell. + * period expire) since we last received a cell. * * Also has the side effect of rewinding the circuit time history * in the case of recent liveness changes. From 7eedd0f6bc6843a5fdd755dc68183296a2ad54bc Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 29 Sep 2010 20:53:50 -0700 Subject: [PATCH 10/10] Nominaly lower the minimum timeout value to 1500. This won't change any behavior, since it will still be rounded back up to 2seconds, but should reduce the chances of some extra warns. --- src/or/or.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/or.h b/src/or/or.h index c90b0391a5..6e1d903e66 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2996,7 +2996,7 @@ double circuit_build_times_quantile_cutoff(void); #define CBT_DEFAULT_TEST_FREQUENCY 60 /** Lowest allowable value for CircuitBuildTimeout in milliseconds */ -#define CBT_DEFAULT_TIMEOUT_MIN_VALUE (2*1000) +#define CBT_DEFAULT_TIMEOUT_MIN_VALUE (1500) /** Initial circuit build timeout in milliseconds */ #define CBT_DEFAULT_TIMEOUT_INITIAL_VALUE (60*1000)