Merge remote-tracking branch 'public/decouple_conn_attach_2'

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.

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 */
@@ -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);
   }
 }
 

src/or/connection_edge.c

@@ -503,6 +503,16 @@ 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.)
+ */
+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)
@@ -514,6 +524,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 +742,114 @@ 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);
+      untried_pending_connections = 1;
+      smartlist_add(pending_entry_connections, entry_conn);
+    }
+
+  } SMARTLIST_FOREACH_END(conn);
+
+  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 <b>retry</b> 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(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);
+    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);
+
+  untried_pending_connections = 0;
+}
+
+/** Mark <b>entry_conn</b> as needing to get attached to a circuit.
+ *
+ * And <b>entry_conn</b> 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;
+  }
+
+  untried_pending_connections = 1;
+  smartlist_add(pending_entry_connections, entry_conn);
 }
 
 /** Tell any AP streams that are waiting for a one-hop tunnel to
@@ -851,12 +970,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 <b>conn</b> is using a dangerous port. Then warn and/or
@@ -1454,10 +1573,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 +1685,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 +2441,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;
 }
@@ -3478,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;
+}

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_attach_pending(void);
+void connection_ap_rescan_and_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);
 void circuit_discard_optional_exit_enclaves(extend_info_t *info);
@@ -100,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

src/or/main.c

@@ -2516,6 +2516,11 @@ run_main_loop_once(void)
     }
   }
 
+  /* 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;
 }
 
@@ -3090,6 +3095,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();

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;
 }
 
@@ -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).",