improve handling of CID retirement

CMakeLists.txt

@@ -59,7 +59,6 @@ SET(QUICLY_LIBRARY_FILES
     lib/rate.c
     lib/recvstate.c
     lib/remote_cid.c
-    lib/retire_cid.c
     lib/sendstate.c
     lib/sentmap.c
     lib/streambuf.c
@@ -77,7 +76,6 @@ SET(UNITTEST_SOURCE_FILES
     t/ranges.c
     t/rate.c
     t/remote_cid.c
-    t/retire_cid.c
     t/sentmap.c
     t/simple.c
     t/stream-concurrency.c

include/quicly/remote_cid.h

@@ -84,6 +84,13 @@ typedef struct st_quicly_remote_cid_set_t {
      * we expect to receive CIDs with sequence number smaller than or equal to this number
      */
     uint64_t _largest_sequence_expected;
+    /**
+     * queue containing CID sequence numbers that should be sent using RETIRE_CONNECTION_ID frames
+     */
+    struct {
+        uint64_t cids[QUICLY_LOCAL_ACTIVE_CONNECTION_ID_LIMIT * 2];
+        size_t count;
+    } retired;
 } quicly_remote_cid_set_t;
 
 /**
@@ -93,16 +100,30 @@ typedef struct st_quicly_remote_cid_set_t {
  */
 void quicly_remote_cid_init_set(quicly_remote_cid_set_t *set, ptls_iovec_t *initial_cid, void (*random_bytes)(void *, size_t));
 /**
- * registers received connection ID
+ * registers received connection ID at the same time pushing CIDs to the retired queue, if any
  * returns 0 if successful (registered or ignored because of duplication/stale information), transport error code otherwise
  */
 int quicly_remote_cid_register(quicly_remote_cid_set_t *set, uint64_t sequence, const uint8_t *cid, size_t cid_len,
-                               const uint8_t srt[QUICLY_STATELESS_RESET_TOKEN_LEN], uint64_t retire_prior_to,
-                               uint64_t unregistered_seqs[QUICLY_LOCAL_ACTIVE_CONNECTION_ID_LIMIT], size_t *num_unregistered_seqs);
+                               const uint8_t srt[QUICLY_STATELESS_RESET_TOKEN_LEN], uint64_t retire_prior_to);
 /**
- * unregisters specified CID from the store
+ * unregisters specified CID from the store; the unregistered CID is pushed onto the retired queue
  */
-void quicly_remote_cid_unregister(quicly_remote_cid_set_t *set, uint64_t sequence);
+int quicly_remote_cid_unregister(quicly_remote_cid_set_t *set, uint64_t sequence);
+/**
+ * pushes a CID sequence number to the retired queue
+ */
+int quicly_remote_cid_push_retired(quicly_remote_cid_set_t *set, uint64_t sequence);
+/**
+ * flushes the retire queue
+ */
+static void quicly_remote_cid_clear_retired(quicly_remote_cid_set_t *set);
+
+/* inline definitions */
+
+inline void quicly_remote_cid_clear_retired(quicly_remote_cid_set_t *set)
+{
+    set->retired.count = 0;
+}
 
 #ifdef __cplusplus
 }

lib/quicly.c

@@ -42,7 +42,6 @@
 #if QUICLY_USE_DTRACE
 #include "quicly-probes.h"
 #endif
-#include "quicly/retire_cid.h"
 
 #define QUICLY_TLS_EXTENSION_TYPE_TRANSPORT_PARAMETERS_FINAL 0x39
 #define QUICLY_TLS_EXTENSION_TYPE_TRANSPORT_PARAMETERS_DRAFT 0xffa5
@@ -413,10 +412,6 @@ struct st_quicly_conn_t {
          *
          */
         uint8_t try_jumpstart : 1;
-        /**
-         * pending RETIRE_CONNECTION_ID frames to be sent
-         */
-        quicly_retire_cid_set_t retire_cid;
         /**
          * payload of DATAGRAM frames to be sent
          */
@@ -1014,27 +1009,17 @@ static size_t local_cid_size(const quicly_conn_t *conn)
 }
 
 /**
- * set up an internal record to send RETIRE_CONNECTION_ID frame later
+ * Resets CIDs associated to paths if they are being retired. To maximize the chance of having enough number of CIDs to run all
+ * paths when new CIDs are provided through multiple NCID frames possibly scattered over multiple packets, CIDs are reassigned to
+ * the paths lazily.
  */
-static void schedule_retire_connection_id_frame(quicly_conn_t *conn, uint64_t sequence)
-{
-    quicly_retire_cid_push(&conn->egress.retire_cid, sequence);
-    conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT;
-}
-
-static void retire_connection_id(quicly_conn_t *conn, uint64_t sequence)
+static void dissociate_cid(quicly_conn_t *conn, uint64_t sequence)
 {
-    /* Reset path CIDs that are being retired. To maximize the chance of having enough number of CIDs to run all paths when new CIDs
-     * are provided through multiple NCID frames possibly scattered over multiple packets, CIDs are reassigned to the paths lazily.
-     */
     for (size_t i = 0; i < PTLS_ELEMENTSOF(conn->paths); ++i) {
         struct st_quicly_conn_path_t *path = conn->paths[i];
         if (path != NULL && path->dcid == sequence)
             path->dcid = UINT64_MAX;
     }
-
-    quicly_remote_cid_unregister(&conn->super.remote.cid_set, sequence);
-    schedule_retire_connection_id_frame(conn, sequence);
 }
 
 static int write_crypto_data(quicly_conn_t *conn, ptls_buffer_t *tlsbuf, size_t epoch_offsets[5])
@@ -1854,14 +1839,25 @@ static int new_path(quicly_conn_t *conn, size_t path_index, struct sockaddr *rem
     return 0;
 }
 
-static void do_delete_path(quicly_conn_t *conn, struct st_quicly_conn_path_t *path)
+static int do_delete_path(quicly_conn_t *conn, struct st_quicly_conn_path_t *path)
 {
-    if (path->dcid != UINT64_MAX && conn->super.remote.cid_set.cids[0].cid.len != 0)
-        retire_connection_id(conn, path->dcid);
+    int ret;
+
+    if (path->dcid != UINT64_MAX && conn->super.remote.cid_set.cids[0].cid.len != 0) {
+        uint64_t cid = path->dcid;
+        dissociate_cid(conn, cid);
+        if ((ret = quicly_remote_cid_unregister(&conn->super.remote.cid_set, cid)) != 0)
+            return ret;
+        assert(conn->super.remote.cid_set.retired.count != 0);
+        conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT;
+    }
+
     free(path);
+
+    return 0;
 }
 
-static void delete_path(quicly_conn_t *conn, size_t path_index)
+static int delete_path(quicly_conn_t *conn, size_t path_index)
 {
     QUICLY_PROBE(DELETE_PATH, conn, conn->stash.now, path_index);
     QUICLY_LOG_CONN(delete_path, conn, { PTLS_LOG_ELEMENT_UNSIGNED(path_index, path_index); });
@@ -1871,20 +1867,21 @@ static void delete_path(quicly_conn_t *conn, size_t path_index)
     if (path->path_challenge.send_at != INT64_MAX)
         conn->super.stats.num_paths.validation_failed += 1;
 
-    do_delete_path(conn, path);
+    return do_delete_path(conn, path);
 }
 
 /**
  * paths[0] (the default path) is freed and the path specified by `path_index` is promoted
  */
 static int promote_path(quicly_conn_t *conn, size_t path_index)
 {
+    int ret;
+
     QUICLY_PROBE(PROMOTE_PATH, conn, conn->stash.now, path_index);
     QUICLY_LOG_CONN(promote_path, conn, { PTLS_LOG_ELEMENT_UNSIGNED(path_index, path_index); });
 
     { /* mark all packets as lost, as it is unlikely that packets sent on the old path wound be acknowledged */
         quicly_sentmap_iter_t iter;
-        int ret;
         if ((ret = quicly_loss_init_sentmap_iter(&conn->egress.loss, &iter, conn->stash.now,
                                                  conn->super.remote.transport_params.max_ack_delay, 0)) != 0)
             return ret;
@@ -1921,12 +1918,12 @@ static int promote_path(quicly_conn_t *conn, size_t path_index)
     conn->paths[path_index] = NULL;
     conn->super.stats.num_paths.promoted += 1;
 
-    do_delete_path(conn, path);
+    ret = do_delete_path(conn, path);
 
     /* rearm the loss timer, now that the RTT estimate has been changed */
     setup_next_send(conn);
 
-    return 0;
+    return ret;
 }
 
 static int open_path(quicly_conn_t *conn, size_t *path_index, struct sockaddr *remote_addr, struct sockaddr *local_addr)
@@ -1950,8 +1947,8 @@ static int open_path(quicly_conn_t *conn, size_t *path_index, struct sockaddr *r
         return QUICLY_ERROR_PACKET_IGNORED;
 
     /* free existing path info */
-    if (conn->paths[*path_index] != NULL)
-        delete_path(conn, *path_index);
+    if (conn->paths[*path_index] != NULL && (ret = delete_path(conn, *path_index)) != 0)
+        return ret;
 
     /* initialize new path info */
     if ((ret = new_path(conn, *path_index, remote_addr, local_addr)) != 0)
@@ -2602,7 +2599,6 @@ static quicly_conn_t *create_connection(quicly_context_t *ctx, uint32_t protocol
         quicly_pacer_reset(conn->egress.pacer);
     }
     conn->egress.ecn.state = conn->super.ctx->enable_ecn ? QUICLY_ECN_PROBING : QUICLY_ECN_OFF;
-    quicly_retire_cid_init(&conn->egress.retire_cid);
     quicly_linklist_init(&conn->egress.pending_streams.blocked.uni);
     quicly_linklist_init(&conn->egress.pending_streams.blocked.bidi);
     quicly_linklist_init(&conn->egress.pending_streams.control);
@@ -3367,9 +3363,13 @@ static int on_ack_retire_connection_id(quicly_sentmap_t *map, const quicly_sent_
 {
     quicly_conn_t *conn = (quicly_conn_t *)((char *)map - offsetof(quicly_conn_t, egress.loss.sentmap));
     uint64_t sequence = sent->data.retire_connection_id.sequence;
+    int ret;
 
-    if (!acked)
-        schedule_retire_connection_id_frame(conn, sequence);
+    if (!acked) {
+        if ((ret = quicly_remote_cid_push_retired(&conn->super.remote.cid_set, sequence)) != 0)
+            return ret;
+        conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT;
+    }
 
     return 0;
 }
@@ -5202,15 +5202,12 @@ static int send_other_control_frames(quicly_conn_t *conn, quicly_send_context_t
     }
 
     { /* RETIRE_CONNECTION_ID */
-        size_t i, size = quicly_retire_cid_get_num_pending(&conn->egress.retire_cid);
-        for (i = 0; i < size; i++) {
-            uint64_t sequence = conn->egress.retire_cid.sequences[i];
+        for (size_t i = 0; i < conn->super.remote.cid_set.retired.count; ++i) {
+            uint64_t sequence = conn->super.remote.cid_set.retired.cids[i];
             if ((ret = send_retire_connection_id(conn, s, sequence)) != 0)
-                break;
+                return ret;
         }
-        quicly_retire_cid_shift(&conn->egress.retire_cid, i);
-        if (ret != 0)
-            return ret;
+        quicly_remote_cid_clear_retired(&conn->super.remote.cid_set);
     }
 
     return 0;
@@ -6581,15 +6578,15 @@ static int handle_new_connection_id_frame(quicly_conn_t *conn, struct st_quicly_
         PTLS_LOG_ELEMENT_HEXDUMP(stateless_reset_token, frame.stateless_reset_token, QUICLY_STATELESS_RESET_TOKEN_LEN);
     });
 
-    uint64_t unregistered_seqs[QUICLY_LOCAL_ACTIVE_CONNECTION_ID_LIMIT];
-    size_t num_unregistered_seqs;
+    size_t orig_num_retired = conn->super.remote.cid_set.retired.count;
     if ((ret = quicly_remote_cid_register(&conn->super.remote.cid_set, frame.sequence, frame.cid.base, frame.cid.len,
-                                          frame.stateless_reset_token, frame.retire_prior_to, unregistered_seqs,
-                                          &num_unregistered_seqs)) != 0)
+                                          frame.stateless_reset_token, frame.retire_prior_to)) != 0)
         return ret;
-
-    for (size_t i = 0; i < num_unregistered_seqs; i++)
-        retire_connection_id(conn, unregistered_seqs[i]);
+    if (orig_num_retired != conn->super.remote.cid_set.retired.count) {
+        for (size_t i = orig_num_retired; i < conn->super.remote.cid_set.retired.count; ++i)
+            dissociate_cid(conn, conn->super.remote.cid_set.retired.cids[i]);
+        conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT;
+    }
 
     return 0;
 }