From 04d7f11086b97fa8bda2377ffe694089e18e45e7 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 26 Jun 2023 20:26:59 +0000 Subject: [PATCH] Bug 40566: Remove unused BDP estimators --- src/core/or/congestion_control_common.c | 239 ++---------------------- src/core/or/congestion_control_common.h | 6 +- src/core/or/congestion_control_st.h | 10 +- src/core/or/congestion_control_vegas.c | 7 +- src/core/or/congestion_control_vegas.h | 3 +- src/core/or/lttng_cc.inc | 8 +- src/core/or/sendme.c | 2 +- src/test/test_conflux_pool.c | 2 - src/test/test_congestion_control.c | 2 +- 9 files changed, 24 insertions(+), 255 deletions(-) diff --git a/src/core/or/congestion_control_common.c b/src/core/or/congestion_control_common.c index 2039354c1f..7dc9dbafdd 100644 --- a/src/core/or/congestion_control_common.c +++ b/src/core/or/congestion_control_common.c @@ -86,8 +86,7 @@ static bool congestion_control_update_circuit_bdp(congestion_control_t *, const circuit_t *, - const crypt_path_t *, - uint64_t, uint64_t); + uint64_t); /* Number of times the RTT value was reset. For MetricsPort. */ static uint64_t num_rtt_reset; @@ -321,12 +320,6 @@ congestion_control_init_params(congestion_control_t *cc, cc->cc_alg = cc_alg; } - cc->bdp_alg = - networkstatus_get_param(NULL, "cc_bdp_alg", - VEGAS_BDP_MIX_ALG, - 0, - NUM_BDP_ALGS-1); - /* Algorithm-specific parameters */ if (cc->cc_alg == CC_ALG_VEGAS) { congestion_control_vegas_set_params(cc, path); @@ -407,7 +400,6 @@ congestion_control_init(congestion_control_t *cc, cc_path_t path) { cc->sendme_pending_timestamps = smartlist_new(); - cc->sendme_arrival_timestamps = smartlist_new(); cc->in_slow_start = 1; congestion_control_init_params(cc, params, path); @@ -438,9 +430,7 @@ congestion_control_free_(congestion_control_t *cc) return; SMARTLIST_FOREACH(cc->sendme_pending_timestamps, uint64_t *, t, tor_free(t)); - SMARTLIST_FOREACH(cc->sendme_arrival_timestamps, uint64_t *, t, tor_free(t)); smartlist_free(cc->sendme_pending_timestamps); - smartlist_free(cc->sendme_arrival_timestamps); tor_free(cc); } @@ -457,22 +447,6 @@ enqueue_timestamp(smartlist_t *timestamps_u64, uint64_t timestamp_usec) smartlist_add(timestamps_u64, timestamp_ptr); } -/** - * Peek at the head of a smartlist queue of u64 timestamps. - */ -static inline uint64_t -peek_timestamp(const smartlist_t *timestamps_u64_usecs) -{ - uint64_t *timestamp_ptr = smartlist_get(timestamps_u64_usecs, 0); - - if (BUG(!timestamp_ptr)) { - log_err(LD_CIRC, "Congestion control timestamp list became empty!"); - return 0; - } - - return *timestamp_ptr; -} - /** * Dequeue a u64 monotime usec timestamp from the front of a * smartlist of pointers to 64. @@ -676,61 +650,6 @@ congestion_control_note_cell_sent(congestion_control_t *cc, monotime_absolute_usec()); } -/** - * Returns true if any edge connections are active. - * - * We need to know this so that we can stop computing BDP if the - * edges are not sending on the circuit. - */ -static int -circuit_has_active_streams(const circuit_t *circ, - const crypt_path_t *layer_hint) -{ - const edge_connection_t *streams; - - if (CIRCUIT_IS_ORIGIN(circ)) { - streams = CONST_TO_ORIGIN_CIRCUIT(circ)->p_streams; - } else { - streams = CONST_TO_OR_CIRCUIT(circ)->n_streams; - } - - /* Check linked list of streams */ - for (const edge_connection_t *conn = streams; conn != NULL; - conn = conn->next_stream) { - if (conn->base_.marked_for_close) - continue; - - if (edge_uses_cpath(conn, layer_hint)) { - if (connection_get_inbuf_len(TO_CONN(conn)) > 0) { - log_info(LD_CIRC, "CC: More in edge inbuf..."); - return 1; - } - - /* If we did not reach EOF on this read, there's more */ - if (!TO_CONN(conn)->inbuf_reached_eof) { - log_info(LD_CIRC, "CC: More on edge conn..."); - return 1; - } - - if (TO_CONN(conn)->linked_conn) { - if (connection_get_inbuf_len(TO_CONN(conn)->linked_conn) > 0) { - log_info(LD_CIRC, "CC: More in linked inbuf..."); - return 1; - } - - /* If there is a linked conn, and *it* did not each EOF, - * there's more */ - if (!TO_CONN(conn)->linked_conn->inbuf_reached_eof) { - log_info(LD_CIRC, "CC: More on linked conn..."); - return 1; - } - } - } - } - - return 0; -} - /** * Upon receipt of a SENDME, pop the oldest timestamp off the timestamp * list, and use this to update RTT. @@ -740,15 +659,13 @@ circuit_has_active_streams(const circuit_t *circ, */ bool congestion_control_update_circuit_estimates(congestion_control_t *cc, - const circuit_t *circ, - const crypt_path_t *layer_hint) + const circuit_t *circ) { uint64_t now_usec = monotime_absolute_usec(); /* Update RTT first, then BDP. BDP needs fresh RTT */ uint64_t curr_rtt_usec = congestion_control_update_circuit_rtt(cc, now_usec); - return congestion_control_update_circuit_bdp(cc, circ, layer_hint, now_usec, - curr_rtt_usec); + return congestion_control_update_circuit_bdp(cc, circ, curr_rtt_usec); } /** @@ -764,13 +681,6 @@ time_delta_should_use_heuristics(const congestion_control_t *cc) return true; } - /* If we managed to get enough acks to estimate a SENDME BDP, then - * we have enough to estimate clock jumps relative to a baseline, - * too. (This is at least 'cc_bwe_min' acks). */ - if (cc->bdp[BDP_ALG_SENDME_RATE]) { - return true; - } - /* Not enough data to estimate clock jumps */ return false; } @@ -937,14 +847,10 @@ congestion_control_update_circuit_rtt(congestion_control_t *cc, static bool congestion_control_update_circuit_bdp(congestion_control_t *cc, const circuit_t *circ, - const crypt_path_t *layer_hint, - uint64_t now_usec, uint64_t curr_rtt_usec) { int chan_q = 0; unsigned int blocked_on_chan = 0; - uint64_t timestamp_usec; - uint64_t sendme_rate_bdp = 0; tor_assert(cc); @@ -982,10 +888,7 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, cc->blocked_chan = 0; } - cc->bdp[BDP_ALG_CWND_RTT] = cwnd; - cc->bdp[BDP_ALG_INFLIGHT_RTT] = cwnd; - cc->bdp[BDP_ALG_SENDME_RATE] = cwnd; - cc->bdp[BDP_ALG_PIECEWISE] = cwnd; + cc->bdp = cwnd; static ratelim_t dec_notice_limit = RATELIM_INIT(300); log_fn_ratelim(&dec_notice_limit, LOG_NOTICE, LD_CIRC, @@ -1004,84 +907,7 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, * close to ewma RTT. Since all fields are u64, there is plenty of * room here to multiply first. */ - cc->bdp[BDP_ALG_CWND_RTT] = cc->cwnd*cc->min_rtt_usec/cc->ewma_rtt_usec; - - /* - * If we have no pending streams, we do not have enough data to fill - * the BDP, so preserve our old estimates but do not make any more. - */ - if (!blocked_on_chan && !circuit_has_active_streams(circ, layer_hint)) { - log_info(LD_CIRC, - "CC: Streams drained. Spare package window: %"PRIu64 - ", no BDP update", cc->cwnd - cc->inflight); - - /* Clear SENDME timestamps; they will be wrong with intermittent data */ - SMARTLIST_FOREACH(cc->sendme_arrival_timestamps, uint64_t *, t, - tor_free(t)); - smartlist_clear(cc->sendme_arrival_timestamps); - } else if (curr_rtt_usec && is_monotime_clock_reliable()) { - /* Sendme-based BDP will quickly measure BDP in much less than - * a cwnd worth of data when in use (in 2-10 SENDMEs). - * - * But if the link goes idle, it will be vastly lower than true BDP. Hence - * we only compute it if we have either pending stream data, or streams - * are still blocked on the channel queued data. - * - * We also do not compute it if we do not have a current RTT passed in, - * because that means that monotime is currently stalled or just jumped. - */ - enqueue_timestamp(cc->sendme_arrival_timestamps, now_usec); - - if (smartlist_len(cc->sendme_arrival_timestamps) >= bwe_sendme_min) { - /* If we have more sendmes than fit in a cwnd, trim the list. - * Those are not acurrately measuring throughput, if cwnd is - * currently smaller than BDP */ - while (smartlist_len(cc->sendme_arrival_timestamps) > - bwe_sendme_min && - (uint64_t)smartlist_len(cc->sendme_arrival_timestamps) > - n_ewma_count(cc)) { - (void)dequeue_timestamp(cc->sendme_arrival_timestamps); - } - int sendme_cnt = smartlist_len(cc->sendme_arrival_timestamps); - - /* Calculate SENDME_BWE_COUNT pure average */ - timestamp_usec = peek_timestamp(cc->sendme_arrival_timestamps); - uint64_t delta = now_usec - timestamp_usec; - - /* In Shadow, the time delta between acks can be 0 if there is no - * network activity between them. Only update BDP if the delta is - * non-zero. */ - if (delta > 0) { - /* The acked data is in sendme_cnt-1 chunks, because we are counting - * the data that is processed by the other endpoint *between* all of - * these sendmes. There's one less gap between the sendmes than the - * number of sendmes. */ - uint64_t cells = (sendme_cnt-1)*cc->sendme_inc; - - /* The bandwidth estimate is cells/delta, which when multiplied - * by min RTT obtains the BDP. However, we multiply first to - * avoid precision issues with the RTT being close to delta in size. */ - sendme_rate_bdp = cells*cc->min_rtt_usec/delta; - - /* Calculate BDP_EWMA_COUNT N-EWMA */ - cc->bdp[BDP_ALG_SENDME_RATE] = - n_count_ewma(sendme_rate_bdp, cc->bdp[BDP_ALG_SENDME_RATE], - n_ewma_count(cc)); - } - } - - /* In-flight BDP will cause the cwnd to drift down when underutilized. - * It is most useful when the local OR conn is blocked, so we only - * compute it if we're utilized. */ - cc->bdp[BDP_ALG_INFLIGHT_RTT] = - (cc->inflight - chan_q)*cc->min_rtt_usec/ - MAX(cc->ewma_rtt_usec, curr_rtt_usec); - } else { - /* We can still update inflight with just an EWMA RTT, but only - * if there is data flowing */ - cc->bdp[BDP_ALG_INFLIGHT_RTT] = - (cc->inflight - chan_q)*cc->min_rtt_usec/cc->ewma_rtt_usec; - } + cc->bdp = cc->cwnd*cc->min_rtt_usec/cc->ewma_rtt_usec; /* The orconn is blocked; use smaller of inflight vs SENDME */ if (blocked_on_chan) { @@ -1094,13 +920,6 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, cc->next_cc_event = 0; cc->blocked_chan = 1; } - - if (cc->bdp[BDP_ALG_SENDME_RATE]) { - cc->bdp[BDP_ALG_PIECEWISE] = MIN(cc->bdp[BDP_ALG_INFLIGHT_RTT], - cc->bdp[BDP_ALG_SENDME_RATE]); - } else { - cc->bdp[BDP_ALG_PIECEWISE] = cc->bdp[BDP_ALG_INFLIGHT_RTT]; - } } else { /* If we were previously blocked, emit a new congestion event * now that we are unblocked, to re-evaluate cwnd */ @@ -1110,19 +929,6 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, log_info(LD_CIRC, "CC: Streams un-blocked on circ channel. Chanq: %d", chan_q); } - - cc->bdp[BDP_ALG_PIECEWISE] = MAX(cc->bdp[BDP_ALG_SENDME_RATE], - cc->bdp[BDP_ALG_CWND_RTT]); - } - - /* We can end up with no piecewise value if we didn't have either - * a SENDME estimate or enough data for an inflight estimate. - * It also happens on the very first sendme, since we need two - * to get a BDP. In these cases, use the cwnd method. */ - if (!cc->bdp[BDP_ALG_PIECEWISE]) { - cc->bdp[BDP_ALG_PIECEWISE] = cc->bdp[BDP_ALG_CWND_RTT]; - log_info(LD_CIRC, "CC: No piecewise BDP. Using %"PRIu64, - cc->bdp[BDP_ALG_PIECEWISE]); } if (cc->next_cc_event == 0) { @@ -1130,44 +936,25 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, log_info(LD_CIRC, "CC: Circuit %d " "SENDME RTT: %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64", " - "BDP estimates: " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64". ", + "BDP estimate: %"PRIu64, CONST_TO_ORIGIN_CIRCUIT(circ)->global_identifier, cc->min_rtt_usec/1000, curr_rtt_usec/1000, cc->ewma_rtt_usec/1000, cc->max_rtt_usec/1000, - cc->bdp[BDP_ALG_INFLIGHT_RTT], - cc->bdp[BDP_ALG_CWND_RTT], - sendme_rate_bdp, - cc->bdp[BDP_ALG_SENDME_RATE], - cc->bdp[BDP_ALG_PIECEWISE] - ); + cc->bdp); } else { log_info(LD_CIRC, "CC: Circuit %"PRIu64":%d " "SENDME RTT: %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64", " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64", " - "%"PRIu64". ", + "%"PRIu64, CONST_TO_OR_CIRCUIT(circ)->p_chan->global_identifier, CONST_TO_OR_CIRCUIT(circ)->p_circ_id, cc->min_rtt_usec/1000, curr_rtt_usec/1000, cc->ewma_rtt_usec/1000, cc->max_rtt_usec/1000, - cc->bdp[BDP_ALG_INFLIGHT_RTT], - cc->bdp[BDP_ALG_CWND_RTT], - sendme_rate_bdp, - cc->bdp[BDP_ALG_SENDME_RATE], - cc->bdp[BDP_ALG_PIECEWISE] - ); + cc->bdp); } } @@ -1175,8 +962,7 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, * the curr_rtt_usec was not 0. */ bool ret = (blocked_on_chan || curr_rtt_usec != 0); if (ret) { - tor_trace(TR_SUBSYS(cc), TR_EV(bdp_update), circ, cc, curr_rtt_usec, - sendme_rate_bdp); + tor_trace(TR_SUBSYS(cc), TR_EV(bdp_update), circ, cc, curr_rtt_usec); } return ret; } @@ -1186,13 +972,12 @@ congestion_control_update_circuit_bdp(congestion_control_t *cc, */ int congestion_control_dispatch_cc_alg(congestion_control_t *cc, - circuit_t *circ, - const crypt_path_t *layer_hint) + circuit_t *circ) { int ret = -END_CIRC_REASON_INTERNAL; tor_assert_nonfatal_once(cc->cc_alg == CC_ALG_VEGAS); - ret = congestion_control_vegas_process_sendme(cc, circ, layer_hint); + ret = congestion_control_vegas_process_sendme(cc, circ); if (cc->cwnd > cwnd_max) { static ratelim_t cwnd_limit = RATELIM_INIT(60); diff --git a/src/core/or/congestion_control_common.h b/src/core/or/congestion_control_common.h index 92c78c5ebd..e185a5d29e 100644 --- a/src/core/or/congestion_control_common.h +++ b/src/core/or/congestion_control_common.h @@ -46,16 +46,14 @@ congestion_control_t *congestion_control_new( cc_path_t path); int congestion_control_dispatch_cc_alg(congestion_control_t *cc, - circuit_t *circ, - const crypt_path_t *layer_hint); + circuit_t *circ); void congestion_control_note_cell_sent(congestion_control_t *cc, const circuit_t *circ, const crypt_path_t *cpath); bool congestion_control_update_circuit_estimates(congestion_control_t *, - const circuit_t *, - const crypt_path_t *); + const circuit_t *); int congestion_control_get_package_window(const circuit_t *, const crypt_path_t *); diff --git a/src/core/or/congestion_control_st.h b/src/core/or/congestion_control_st.h index ba12e93242..b87d230c2f 100644 --- a/src/core/or/congestion_control_st.h +++ b/src/core/or/congestion_control_st.h @@ -106,19 +106,13 @@ struct congestion_control_t { * sendme_last_digests. */ smartlist_t *sendme_pending_timestamps; - /** - * Smartlist of uint64_t monotime timestamp of when sendme's arrived. - * FIFO queue that is managed similar to sendme_last_digests. - * Used to estimate circuitbandwidth and BDP. */ - smartlist_t *sendme_arrival_timestamps; - /** RTT time data for congestion control. */ uint64_t ewma_rtt_usec; uint64_t min_rtt_usec; uint64_t max_rtt_usec; - /* BDP estimates by algorithm */ - uint64_t bdp[NUM_BDP_ALGS]; + /* Vegas BDP estimate */ + uint64_t bdp; /** Congestion window */ uint64_t cwnd; diff --git a/src/core/or/congestion_control_vegas.c b/src/core/or/congestion_control_vegas.c index b611fc50c4..97f1dc0a0e 100644 --- a/src/core/or/congestion_control_vegas.c +++ b/src/core/or/congestion_control_vegas.c @@ -98,7 +98,7 @@ uint64_t cc_stats_vegas_circ_exited_ss = 0; static inline uint64_t vegas_bdp(const congestion_control_t *cc) { - return cc->bdp[BDP_ALG_CWND_RTT]; + return cc->bdp; } /** @@ -405,8 +405,7 @@ cwnd_full_reset(const congestion_control_t *cc) */ int congestion_control_vegas_process_sendme(congestion_control_t *cc, - const circuit_t *circ, - const crypt_path_t *layer_hint) + const circuit_t *circ) { uint64_t queue_use; @@ -422,7 +421,7 @@ congestion_control_vegas_process_sendme(congestion_control_t *cc, cc->next_cwnd_event--; /* Compute BDP and RTT. If we did not update, don't run the alg */ - if (!congestion_control_update_circuit_estimates(cc, circ, layer_hint)) { + if (!congestion_control_update_circuit_estimates(cc, circ)) { cc->inflight = cc->inflight - cc->sendme_inc; return 0; } diff --git a/src/core/or/congestion_control_vegas.h b/src/core/or/congestion_control_vegas.h index 84070664c8..24af16822c 100644 --- a/src/core/or/congestion_control_vegas.h +++ b/src/core/or/congestion_control_vegas.h @@ -35,8 +35,7 @@ extern uint64_t cc_stats_vegas_circ_exited_ss; /* Processing SENDME cell. */ int congestion_control_vegas_process_sendme(struct congestion_control_t *cc, - const circuit_t *circ, - const crypt_path_t *layer_hint); + const circuit_t *circ); void congestion_control_vegas_set_params(struct congestion_control_t *cc, cc_path_t path); diff --git a/src/core/or/lttng_cc.inc b/src/core/or/lttng_cc.inc index de06fa026f..d315be91f7 100644 --- a/src/core/or/lttng_cc.inc +++ b/src/core/or/lttng_cc.inc @@ -142,7 +142,7 @@ TRACEPOINT_EVENT(tor_cc, flow_decide_xoff_sending, /* Emitted when the BDP value has been updated. */ TRACEPOINT_EVENT(tor_cc, bdp_update, TP_ARGS(const circuit_t *, circ, const congestion_control_t *, cc, - uint64_t, curr_rtt_usec, uint64_t, sendme_rate_bdp), + uint64_t, curr_rtt_usec), TP_FIELDS( ctf_integer(uint64_t, circuit_ptr, circ) ctf_integer(uint32_t, n_circ_id, circ->n_circ_id) @@ -150,11 +150,7 @@ TRACEPOINT_EVENT(tor_cc, bdp_update, ctf_integer(uint64_t, curr_rtt_usec, curr_rtt_usec) ctf_integer(uint64_t, ewma_rtt_usec, cc->ewma_rtt_usec) ctf_integer(uint64_t, max_rtt_usec, cc->max_rtt_usec) - ctf_integer(uint64_t, bdp_inflight_rtt, cc->bdp[BDP_ALG_INFLIGHT_RTT]) - ctf_integer(uint64_t, bdp_cwnd_rtt, cc->bdp[BDP_ALG_CWND_RTT]) - ctf_integer(uint64_t, bdp_sendme_rate, cc->bdp[BDP_ALG_SENDME_RATE]) - ctf_integer(uint64_t, bdp_piecewise, cc->bdp[BDP_ALG_PIECEWISE]) - ctf_integer(uint64_t, sendme_rate_bdp, sendme_rate_bdp) + ctf_integer(uint64_t, bdp_cwnd_rtt, cc->bdp) ) ) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 4bb5c268b0..03af990484 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -491,7 +491,7 @@ sendme_process_circuit_level(crypt_path_t *layer_hint, return sendme_process_circuit_level_impl(layer_hint, circ); } - return congestion_control_dispatch_cc_alg(cc, circ, layer_hint); + return congestion_control_dispatch_cc_alg(cc, circ); } /** diff --git a/src/test/test_conflux_pool.c b/src/test/test_conflux_pool.c index ad78283fe3..fc30677377 100644 --- a/src/test/test_conflux_pool.c +++ b/src/test/test_conflux_pool.c @@ -128,7 +128,6 @@ circuit_establish_circuit_conflux_mock(const uint8_t *conflux_nonce, simulate_single_hop_extend(circ, 0); simulate_single_hop_extend(circ, 1); circ->cpath->prev->ccontrol = tor_malloc_zero(sizeof(congestion_control_t)); - circ->cpath->prev->ccontrol->sendme_arrival_timestamps = smartlist_new(); circ->cpath->prev->ccontrol->sendme_pending_timestamps = smartlist_new(); circ->cpath->prev->ccontrol->sendme_inc = 31; @@ -499,7 +498,6 @@ simulate_circuit_build(circuit_t *client_circ) relay_side->purpose = CIRCUIT_PURPOSE_OR; relay_side->n_chan = NULL; // No next hop relay_side->ccontrol = tor_malloc_zero(sizeof(congestion_control_t)); - relay_side->ccontrol->sendme_arrival_timestamps = smartlist_new(); relay_side->ccontrol->sendme_pending_timestamps = smartlist_new(); relay_side->ccontrol->sendme_inc = 31; smartlist_add(exit_circs, relay_side); diff --git a/src/test/test_congestion_control.c b/src/test/test_congestion_control.c index f494515e99..26095d310a 100644 --- a/src/test/test_congestion_control.c +++ b/src/test/test_congestion_control.c @@ -216,7 +216,7 @@ run_vegas_cwnd_test_vec(congestion_control_t *cc, circ->circuit_blocked_on_p_chan = vec[i].or_conn_blocked_in; cc->inflight = vec[i].inflight_in; - congestion_control_vegas_process_sendme(cc, circ, NULL); + congestion_control_vegas_process_sendme(cc, circ); /* If the or conn was blocked, ensure we updated our * CC state */