From b1d56fc5890fb6d594e70520c09d040e9b2e1544 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 13 Nov 2015 13:38:01 -0500 Subject: [PATCH 1/3] Decouple ..attach_circuit() from most of its callers. Long ago we used to call connection_ap_handshake_attach_circuit() only in a few places, since connection_ap_attach_pending() attaches all the pending connections, and does so regularly. But this turned out to have a performance problem: it would introduce a delay to launching or connecting a stream. We couldn't just call connection_ap_attach_pending() every time we make a new connection, since it walks the whole connection list. So we started calling connection_ap_attach_pending all over, instead! But that's kind of ugly and messes up our callgraph. So instead, we now have connection_ap_attach_pending() use a list only of the pending connections, so we can call it much more frequently. We have a separate function to scan the whole connection array to see if we missed adding anything, and log a warning if so. Closes ticket #17590 --- changes/decouple_conn_attach | 6 ++ src/or/circuituse.c | 2 +- src/or/connection_edge.c | 143 ++++++++++++++++++++++++++++++----- src/or/connection_edge.h | 2 + src/or/main.c | 5 ++ src/or/rendclient.c | 7 +- 6 files changed, 137 insertions(+), 28 deletions(-) create mode 100644 changes/decouple_conn_attach diff --git a/changes/decouple_conn_attach b/changes/decouple_conn_attach new file mode 100644 index 0000000000..6167b4e932 --- /dev/null +++ b/changes/decouple_conn_attach @@ -0,0 +1,6 @@ + o Code simplification and refactorings: + - Decouple the list of streams needing to be attached to circuits + from the overall connection list. This change makes it possible to + attach streams quickly while both simplifying Tor's callgraph and + avoiding O(N) scans of the entire connection list. Closes ticket + 17590. diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 00340fd689..3a1d154178 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1123,7 +1123,7 @@ circuit_build_needed_circs(time_t now) * don't require an exit circuit, review in #13814. * This allows HSs to function in a consensus without exits. */ if (router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) - connection_ap_attach_pending(); + connection_ap_rescan_and_attach_pending(); /* make sure any hidden services have enough intro points * HS intro point streams only require an internal circuit */ diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 729ef8a4c7..ce6bacefd3 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -503,6 +503,15 @@ connection_edge_finished_connecting(edge_connection_t *edge_conn) return connection_edge_process_inbuf(edge_conn, 1); } +/** A list of all the entry_connection_t * objects that are not marked + * for close, and are in AP_CONN_STATE_CIRCUIT_WAIT. + * + * (Right now, we check in several places to make sure that this list is + * correct. When it's incorrect, we'll fix it, and log a BUG message.) + */ +/* XXXXX Free this list on exit. */ +static smartlist_t *pending_entry_connections = NULL; + /** Common code to connection_(ap|exit)_about_to_close. */ static void connection_edge_about_to_close(edge_connection_t *edge_conn) @@ -514,6 +523,27 @@ connection_edge_about_to_close(edge_connection_t *edge_conn) conn->marked_for_close_file, conn->marked_for_close); tor_fragile_assert(); } + + if (TO_CONN(edge_conn)->type != CONN_TYPE_AP || + PREDICT_UNLIKELY(NULL == pending_entry_connections)) + return; + + entry_connection_t *entry_conn = EDGE_TO_ENTRY_CONN(edge_conn); + + if (TO_CONN(edge_conn)->state == AP_CONN_STATE_CIRCUIT_WAIT) { + smartlist_remove(pending_entry_connections, entry_conn); + } + +#if 1 + /* Check to make sure that this isn't in pending_entry_connections if it + * didn't actually belong there. */ + if (TO_CONN(edge_conn)->type == CONN_TYPE_AP && + smartlist_contains(pending_entry_connections, entry_conn)) { + log_warn(LD_BUG, "What was %p doing in pending_entry_connections???", + entry_conn); + smartlist_remove(pending_entry_connections, entry_conn); + } +#endif } /** Called when we're about to finally unlink and free an AP (client) @@ -711,26 +741,104 @@ connection_ap_expire_beginning(void) } SMARTLIST_FOREACH_END(base_conn); } -/** Tell any AP streams that are waiting for a new circuit to try again, - * either attaching to an available circ or launching a new one. +/** + * As connection_ap_attach_pending, but first scans the entire connection + * array to see if any elements are missing. */ void -connection_ap_attach_pending(void) +connection_ap_rescan_and_attach_pending(void) { entry_connection_t *entry_conn; smartlist_t *conns = get_connection_array(); + + if (PREDICT_UNLIKELY(NULL == pending_entry_connections)) + pending_entry_connections = smartlist_new(); + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) { if (conn->marked_for_close || conn->type != CONN_TYPE_AP || conn->state != AP_CONN_STATE_CIRCUIT_WAIT) continue; + entry_conn = TO_ENTRY_CONN(conn); + if (! smartlist_contains(pending_entry_connections, entry_conn)) { + log_warn(LD_BUG, "Found a connection %p that was supposed to be " + "in pending_entry_connections, but wasn't. No worries; " + "adding it.", + pending_entry_connections); + smartlist_add(pending_entry_connections, entry_conn); + } + + } SMARTLIST_FOREACH_END(conn); + + connection_ap_attach_pending(); +} + +/** Tell any AP streams that are listed as waiting for a new circuit to try + * again, either attaching to an available circ or launching a new one. + */ +void +connection_ap_attach_pending(void) +{ + if (PREDICT_UNLIKELY(!pending_entry_connections)) { + return; + } + + SMARTLIST_FOREACH_BEGIN(pending_entry_connections, + entry_connection_t *, entry_conn) { + connection_t *conn = ENTRY_TO_CONN(entry_conn); + if (conn->marked_for_close) { + SMARTLIST_DEL_CURRENT(pending_entry_connections, entry_conn); + continue; + } + if (conn->state != AP_CONN_STATE_CIRCUIT_WAIT) { + log_warn(LD_BUG, "%p is no longer in circuit_wait. Why is it on " + "pending_entry_connections?", entry_conn); + SMARTLIST_DEL_CURRENT(pending_entry_connections, entry_conn); + continue; + } + if (connection_ap_handshake_attach_circuit(entry_conn) < 0) { if (!conn->marked_for_close) connection_mark_unattached_ap(entry_conn, END_STREAM_REASON_CANT_ATTACH); } - } SMARTLIST_FOREACH_END(conn); + + if (conn->marked_for_close || + conn->type != CONN_TYPE_AP || + conn->state != AP_CONN_STATE_CIRCUIT_WAIT) { + SMARTLIST_DEL_CURRENT(pending_entry_connections, entry_conn); + } + + } SMARTLIST_FOREACH_END(entry_conn); +} + +/** Mark entry_conn as needing to get attached to a circuit. + * + * And entry_conn must be in AP_CONN_STATE_CIRCUIT_WAIT, + * should not already be pending a circuit. The circuit will get + * launched or the connection will get attached the next time we + * call connection_ap_attach_pending(). + */ +void +connection_ap_mark_as_pending_circuit(entry_connection_t *entry_conn) +{ + connection_t *conn = ENTRY_TO_CONN(entry_conn); + tor_assert(conn->state == AP_CONN_STATE_CIRCUIT_WAIT); + if (conn->marked_for_close) + return; + + if (PREDICT_UNLIKELY(NULL == pending_entry_connections)) + pending_entry_connections = smartlist_new(); + + if (PREDICT_UNLIKELY(smartlist_contains(pending_entry_connections, + entry_conn))) { + log_warn(LD_BUG, "What?? pending_entry_connections already contains %p!", + entry_conn); + return; + } + + smartlist_add(pending_entry_connections, entry_conn); } /** Tell any AP streams that are waiting for a one-hop tunnel to @@ -851,12 +959,12 @@ connection_ap_detach_retriable(entry_connection_t *conn, * a tunneled directory connection, then just attach it. */ ENTRY_TO_CONN(conn)->state = AP_CONN_STATE_CIRCUIT_WAIT; circuit_detach_stream(TO_CIRCUIT(circ),ENTRY_TO_EDGE_CONN(conn)); - return connection_ap_handshake_attach_circuit(conn); + connection_ap_mark_as_pending_circuit(conn); } else { ENTRY_TO_CONN(conn)->state = AP_CONN_STATE_CONTROLLER_WAIT; circuit_detach_stream(TO_CIRCUIT(circ),ENTRY_TO_EDGE_CONN(conn)); - return 0; } + return 0; } /** Check if conn is using a dangerous port. Then warn and/or @@ -1454,10 +1562,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn, /* If we were given a circuit to attach to, try to attach. Otherwise, * try to find a good one and attach to that. */ int rv; - if (circ) - rv = connection_ap_handshake_attach_chosen_circuit(conn, circ, cpath); - else - rv = connection_ap_handshake_attach_circuit(conn); + if (circ) { + rv = connection_ap_handshake_attach_chosen_circuit(conn, circ, cpath); + } else { + connection_ap_mark_as_pending_circuit(conn); + rv = 0; + } /* If the above function returned 0 then we're waiting for a circuit. * if it returned 1, we're attached. Both are okay. But if it returned @@ -1564,11 +1674,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn, /* We have the descriptor so launch a connection to the HS. */ base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT; log_info(LD_REND, "Descriptor is here. Great."); - if (connection_ap_handshake_attach_circuit(conn) < 0) { - if (!base_conn->marked_for_close) - connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH); - return -1; - } + connection_ap_mark_as_pending_circuit(conn); return 0; } @@ -2324,12 +2430,7 @@ connection_ap_make_link(connection_t *partner, control_event_stream_status(conn, STREAM_EVENT_NEW, 0); /* attaching to a dirty circuit is fine */ - if (connection_ap_handshake_attach_circuit(conn) < 0) { - if (!base_conn->marked_for_close) - connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH); - return NULL; - } - + connection_ap_mark_as_pending_circuit(conn); log_info(LD_APP,"... application connection created and linked."); return conn; } diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h index 7c0b9c0767..203099976f 100644 --- a/src/or/connection_edge.h +++ b/src/or/connection_edge.h @@ -64,7 +64,9 @@ int connection_edge_is_rendezvous_stream(edge_connection_t *conn); int connection_ap_can_use_exit(const entry_connection_t *conn, const node_t *exit); void connection_ap_expire_beginning(void); +void connection_ap_rescan_and_attach_pending(void); void connection_ap_attach_pending(void); +void connection_ap_mark_as_pending_circuit(entry_connection_t *entry_conn); void connection_ap_fail_onehop(const char *failed_digest, cpath_build_state_t *build_state); void circuit_discard_optional_exit_enclaves(extend_info_t *info); diff --git a/src/or/main.c b/src/or/main.c index afcb313574..bfa41b964c 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2505,6 +2505,11 @@ run_main_loop_once(void) } } + /* This should be pretty fast if nothing is pending. BUT... watch out; + * we need to make sure it doesn't show up in the profiles. five times a + * second would be enough, for instance. */ + connection_ap_attach_pending(); + return 1; } diff --git a/src/or/rendclient.c b/src/or/rendclient.c index a39e518e99..3846ef0c32 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -1226,12 +1226,7 @@ rend_client_desc_trynow(const char *query) base_conn->timestamp_lastread = now; base_conn->timestamp_lastwritten = now; - if (connection_ap_handshake_attach_circuit(conn) < 0) { - /* it will never work */ - log_warn(LD_REND,"Rendezvous attempt failed. Closing."); - if (!base_conn->marked_for_close) - connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH); - } + connection_ap_mark_as_pending_circuit(conn); } else { /* 404, or fetch didn't get that far */ log_notice(LD_REND,"Closing stream for '%s.onion': hidden service is " "unavailable (try again later).", From 84b3350c83a995a7668c16cb06ae069664dc0633 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Nov 2015 08:49:30 -0500 Subject: [PATCH 2/3] Be more conservative in scanning the list of pending streams Now we only re-scan the list in the cases we did before: when we have a new circuit that we should try attaching to, or when we have added a new stream that we haven't tried to attach yet. This is part of 17590. --- src/or/circuituse.c | 6 +++--- src/or/connection_edge.c | 16 ++++++++++++++-- src/or/connection_edge.h | 2 +- src/or/main.c | 8 ++++---- src/or/rendclient.c | 4 ++-- 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 3a1d154178..5b24425877 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1475,7 +1475,7 @@ circuit_has_opened(origin_circuit_t *circ) case CIRCUIT_PURPOSE_C_ESTABLISH_REND: rend_client_rendcirc_has_opened(circ); /* Start building an intro circ if we don't have one yet. */ - connection_ap_attach_pending(); + connection_ap_attach_pending(1); /* This isn't a call to circuit_try_attaching_streams because a * circuit in _C_ESTABLISH_REND state isn't connected to its * hidden service yet, thus we can't attach streams to it yet, @@ -1537,14 +1537,14 @@ void circuit_try_attaching_streams(origin_circuit_t *circ) { /* Attach streams to this circuit if we can. */ - connection_ap_attach_pending(); + connection_ap_attach_pending(1); /* The call to circuit_try_clearing_isolation_state here will do * nothing and return 0 if we didn't attach any streams to circ * above. */ if (circuit_try_clearing_isolation_state(circ)) { /* Maybe *now* we can attach some streams to this circuit. */ - connection_ap_attach_pending(); + connection_ap_attach_pending(1); } } diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index ce6bacefd3..20a27dc90c 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -512,6 +512,8 @@ connection_edge_finished_connecting(edge_connection_t *edge_conn) /* XXXXX Free this list on exit. */ static smartlist_t *pending_entry_connections = NULL; +static int untried_pending_connections = 0; + /** Common code to connection_(ap|exit)_about_to_close. */ static void connection_edge_about_to_close(edge_connection_t *edge_conn) @@ -766,24 +768,31 @@ connection_ap_rescan_and_attach_pending(void) "in pending_entry_connections, but wasn't. No worries; " "adding it.", pending_entry_connections); + untried_pending_connections = 1; smartlist_add(pending_entry_connections, entry_conn); } } SMARTLIST_FOREACH_END(conn); - connection_ap_attach_pending(); + connection_ap_attach_pending(1); } /** Tell any AP streams that are listed as waiting for a new circuit to try * again, either attaching to an available circ or launching a new one. + * + * If retry is false, only check the list if it contains at least one + * streams that we have not yet tried to attach to a circuit. */ void -connection_ap_attach_pending(void) +connection_ap_attach_pending(int retry) { if (PREDICT_UNLIKELY(!pending_entry_connections)) { return; } + if (untried_pending_connections == 0 && !retry) + return; + SMARTLIST_FOREACH_BEGIN(pending_entry_connections, entry_connection_t *, entry_conn) { connection_t *conn = ENTRY_TO_CONN(entry_conn); @@ -811,6 +820,8 @@ connection_ap_attach_pending(void) } } SMARTLIST_FOREACH_END(entry_conn); + + untried_pending_connections = 0; } /** Mark entry_conn as needing to get attached to a circuit. @@ -838,6 +849,7 @@ connection_ap_mark_as_pending_circuit(entry_connection_t *entry_conn) return; } + untried_pending_connections = 1; smartlist_add(pending_entry_connections, entry_conn); } diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h index 203099976f..86f3fe990c 100644 --- a/src/or/connection_edge.h +++ b/src/or/connection_edge.h @@ -65,7 +65,7 @@ int connection_ap_can_use_exit(const entry_connection_t *conn, const node_t *exit); void connection_ap_expire_beginning(void); void connection_ap_rescan_and_attach_pending(void); -void connection_ap_attach_pending(void); +void connection_ap_attach_pending(int retry); void connection_ap_mark_as_pending_circuit(entry_connection_t *entry_conn); void connection_ap_fail_onehop(const char *failed_digest, cpath_build_state_t *build_state); diff --git a/src/or/main.c b/src/or/main.c index bfa41b964c..0fe818d74f 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2505,10 +2505,10 @@ run_main_loop_once(void) } } - /* This should be pretty fast if nothing is pending. BUT... watch out; - * we need to make sure it doesn't show up in the profiles. five times a - * second would be enough, for instance. */ - connection_ap_attach_pending(); + /* This will be pretty fast if nothing new is pending. Note that this gets + * called once per libevent loop, which will make it happen once per group + * of events that fire, or once per second. */ + connection_ap_attach_pending(0); return 1; } diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 3846ef0c32..b8a4b2ab9b 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -52,7 +52,7 @@ rend_client_introcirc_has_opened(origin_circuit_t *circ) tor_assert(circ->cpath); log_info(LD_REND,"introcirc is open"); - connection_ap_attach_pending(); + connection_ap_attach_pending(1); } /** Send the establish-rendezvous cell along a rendezvous circuit. if @@ -1107,7 +1107,7 @@ rend_client_rendezvous_acked(origin_circuit_t *circ, const uint8_t *request, * than trying to attach them all. See comments bug 743. */ /* If we already have the introduction circuit built, make sure we send * the INTRODUCE cell _now_ */ - connection_ap_attach_pending(); + connection_ap_attach_pending(1); return 0; } From 58edf92678019c0e69c043976b2af06551d83ed3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Nov 2015 09:06:47 -0500 Subject: [PATCH 3/3] Free pending_entry_connections on shutdown. --- src/or/connection_edge.c | 9 ++++++++- src/or/connection_edge.h | 2 ++ src/or/main.c | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 20a27dc90c..67e594a6a9 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -509,7 +509,6 @@ connection_edge_finished_connecting(edge_connection_t *edge_conn) * (Right now, we check in several places to make sure that this list is * correct. When it's incorrect, we'll fix it, and log a BUG message.) */ -/* XXXXX Free this list on exit. */ static smartlist_t *pending_entry_connections = NULL; static int untried_pending_connections = 0; @@ -3591,3 +3590,11 @@ circuit_clear_isolation(origin_circuit_t *circ) circ->socks_username_len = circ->socks_password_len = 0; } +/** Free all storage held in module-scoped variables for connection_edge.c */ +void +connection_edge_free_all(void) +{ + untried_pending_connections = 0; + smartlist_free(pending_entry_connections); + pending_entry_connections = NULL; +} diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h index 86f3fe990c..521e759aed 100644 --- a/src/or/connection_edge.h +++ b/src/or/connection_edge.h @@ -102,6 +102,8 @@ int connection_edge_update_circuit_isolation(const entry_connection_t *conn, void circuit_clear_isolation(origin_circuit_t *circ); streamid_t get_unique_stream_id_by_circ(origin_circuit_t *circ); +void connection_edge_free_all(void); + /** @name Begin-cell flags * * These flags are used in RELAY_BEGIN cells to change the default behavior diff --git a/src/or/main.c b/src/or/main.c index 0fe818d74f..b5c1b8ba69 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -3084,6 +3084,7 @@ tor_free_all(int postfork) channel_tls_free_all(); channel_free_all(); connection_free_all(); + connection_edge_free_all(); scheduler_free_all(); memarea_clear_freelist(); nodelist_free_all();