]> git.ipfire.org Git - thirdparty/knot-resolver.git/commitdiff
amend! daemon/session2: support multiple short-lived buffers docs-develop-nits-2dntdj/deployments/4198
authorOto Šťáva <oto.stava@nic.cz>
Tue, 28 May 2024 14:54:29 +0000 (16:54 +0200)
committerOto Šťáva <oto.stava@nic.cz>
Tue, 28 May 2024 14:54:29 +0000 (16:54 +0200)
daemon/session2: optimize context-specific allocations

There were two problems:

1) Some payloads are short-lived (e.g. allocated on stack) and we need
   to make a copy of them if the iteration over protocol layers becomes
   asynchronous.
2) The `pl_dns_stream_wrap` function used a mempool belonging to its
   session-wide context. Some sessions may live for a long time, which
   could potentially lead to needlessly long-lived memory allocations.

Both of these problems are solved in this commit by using a new
`knot_mm_t pool` field in `struct protolayer_iter_ctx`, which lives only
for a single submit (and survives asynchronicity). The whole pool is
then freed all at once when the `struct protolayer_iter_ctx` is
finalized.

daemon/session2.c
daemon/session2.h
daemon/worker.c

index ca06e47be1fe8cbad03771f316b3366bd012cbe3..e269844c88bb7573dff66f5ebde95dae819318ab 100644 (file)
@@ -181,46 +181,6 @@ static size_t iovecs_copy(void *dest, const struct iovec *iov, int cnt,
        return copy_size;
 }
 
-void *protolayer_buffer_list_add(struct protolayer_buffer_list *list, size_t n)
-{
-       struct protolayer_buffer_list_entry *e =
-                       malloc(sizeof(struct protolayer_buffer_list_entry) + n);
-       if (!e)
-               return NULL;
-
-       if (!list->head) {
-               if (kr_fails_assert(!list->tail)) {
-                       free(e);
-                       return NULL;
-               }
-
-               list->head = list->tail = e;
-               return e->data;
-       }
-
-       if (kr_fails_assert(list->tail)) {
-               free(e);
-               return NULL;
-       }
-
-       list->tail->next = e;
-       list->tail = e;
-       return e->data;
-}
-
-void protolayer_buffer_list_deinit(struct protolayer_buffer_list *list)
-{
-       struct protolayer_buffer_list_entry *e = list->head;
-       struct protolayer_buffer_list_entry *next;
-       while (e) {
-               next = e->next;
-               free(e);
-               e = next;
-       }
-       list->head = NULL;
-       list->tail = NULL;
-}
-
 size_t protolayer_payload_size(const struct protolayer_payload *payload)
 {
        switch (payload->type) {
@@ -449,7 +409,7 @@ static int protolayer_iter_ctx_finish(struct protolayer_iter_ctx *ctx, int ret)
                ctx->finished_cb(ret, s, &ctx->comm,
                                ctx->finished_cb_baton);
 
-       protolayer_buffer_list_deinit(&ctx->async_buffer_list);
+       mm_ctx_delete(&ctx->pool);
        free(ctx);
 
        return ret;
@@ -503,7 +463,7 @@ static void protolayer_payload_ensure_long_lived(struct protolayer_iter_ctx *ctx
        if (kr_fails_assert(buf_len))
                return;
 
-       void *buf = protolayer_buffer_list_add(&ctx->async_buffer_list, buf_len);
+       void *buf = mm_alloc(&ctx->pool, buf_len);
        kr_require(buf);
        protolayer_payload_copy(buf, &ctx->payload, buf_len);
 
@@ -625,6 +585,7 @@ static int session2_submit(
                .finished_cb = cb,
                .finished_cb_baton = baton
        };
+       mm_ctx_init(&ctx->pool);
 
        const struct protolayer_grp *grp = &protolayer_grps[session->proto];
        for (size_t i = 0; i < grp->num_layers; i++) {
@@ -813,7 +774,6 @@ struct session2 *session2_new(enum session2_transport_type transport_type,
        };
 
        memcpy(&s->layer_data, offsets, sizeof(offsets));
-       mm_ctx_mempool(&s->pool, CPU_PAGE_SIZE);
        queue_init(s->waiting);
        int ret = wire_buf_init(&s->wire_buf, wire_buf_length);
        kr_require(!ret);
@@ -856,7 +816,6 @@ static void session2_free(struct session2 *s)
        }
 
        wire_buf_deinit(&s->wire_buf);
-       mm_ctx_delete(&s->pool);
        trie_free(s->tasks);
        queue_deinit(s->waiting);
        free(s);
index eac8da3d6368f3060ce7a451137d90014d9abd1e..ada57af25cbbd882bca92768107866dd0362cf71 100644 (file)
@@ -392,28 +392,6 @@ struct protolayer_payload {
        };
 };
 
-/** An entry in a linked list of buffers. The buffer data itself is allocated in
- * the same object as the header. */
-struct protolayer_buffer_list_entry {
-       struct protolayer_buffer_list_entry *next;
-       alignas(CPU_STRUCT_ALIGN) char data[];
-};
-
-/** A linked list of buffers. */
-struct protolayer_buffer_list {
-       struct protolayer_buffer_list_entry *head;
-       struct protolayer_buffer_list_entry *tail;
-};
-
-/** Uses `malloc()` to allocate a new buffer of size `n` and adds it to the
- * specified `list`. Returns a pointer to the buffer data (excl. the header) or
- * `NULL` if the allocation fails. */
-void *protolayer_buffer_list_add(struct protolayer_buffer_list *list, size_t n);
-
-/** Frees the specified buffer list's entries (but not the list's control
- * structure itself). */
-void protolayer_buffer_list_deinit(struct protolayer_buffer_list *list);
-
 /** Context for protocol layer iterations, containing payload data,
  * layer-specific data, and internal information for the protocol layer
  * manager. */
@@ -424,6 +402,10 @@ struct protolayer_iter_ctx {
        /** Communication information. Typically written into by one of the
         * first layers facilitating transport protocol processing. */
        struct comm_info comm;
+       /** Per-iter memory pool. Has no `free` procedure, gets freed as a whole
+        * when the context is being destroyed. Initialized and destroyed
+        * automatically - layers may use it to allocate memory. */
+       knot_mm_t pool;
 
 /* callback for when the layer iteration has ended - read-only for layers: */
        protolayer_finished_cb finished_cb;
@@ -442,9 +424,6 @@ struct protolayer_iter_ctx {
        /** Status passed to the finish callback. */
        int status;
        enum protolayer_iter_action action;
-       /** Points to a buffers where data has been copied from short-lived
-        * payloads. Automatically freed together with the context. */
-       struct protolayer_buffer_list async_buffer_list;
 
        /** Contains a sequence of variably-sized CPU-aligned layer-specific
         * structs. See `struct session2::layer_data` for details. */
@@ -783,7 +762,6 @@ struct session2 {
                };
        } transport;
 
-       knot_mm_t pool;
        uv_timer_t timer; /**< For session-wide timeout events. */
        enum protolayer_event_type timer_event; /**< The event fired on timeout. */
        trie_t *tasks; /**< List of tasks associated with given session. */
index a73420458dd5b4b35c4afcb4665f64e1adb3e658..5473c8b89dec05a9c3f3522b63f693c3b96466f6 100644 (file)
@@ -1822,14 +1822,6 @@ struct pl_dns_stream_sess_data {
        bool connected : 1; /**< True: The stream is connected */
 };
 
-struct pl_dns_stream_iter_data {
-       struct protolayer_data h;
-       struct {
-               knot_mm_t *pool;
-               void *mem;
-       } sent;
-};
-
 static int pl_dns_stream_sess_init(struct session2 *session,
                                    void *sess_data, void *param)
 {
@@ -1848,14 +1840,6 @@ static int pl_dns_single_stream_sess_init(struct session2 *session,
        return kr_ok();
 }
 
-static int pl_dns_stream_iter_deinit(struct protolayer_iter_ctx *ctx,
-                                     void *iter_data)
-{
-       struct pl_dns_stream_iter_data *stream = iter_data;
-       mm_free(stream->sent.pool, stream->sent.mem);
-       return kr_ok();
-}
-
 static enum protolayer_event_cb_result pl_dns_stream_resolution_timeout(
                struct session2 *s)
 {
@@ -2232,18 +2216,12 @@ struct sized_iovs {
 static enum protolayer_iter_cb_result pl_dns_stream_wrap(
                void *sess_data, void *iter_data, struct protolayer_iter_ctx *ctx)
 {
-       struct pl_dns_stream_iter_data *stream = iter_data;
-       struct session2 *s = ctx->session;
-
-       if (kr_fails_assert(!stream->sent.mem))
-               return protolayer_break(ctx, kr_error(EINVAL));
-
        if (ctx->payload.type == PROTOLAYER_PAYLOAD_BUFFER) {
                if (kr_fails_assert(ctx->payload.buffer.len <= UINT16_MAX))
                        return protolayer_break(ctx, kr_error(EMSGSIZE));
 
                const int iovcnt = 2;
-               struct sized_iovs *siov = mm_alloc(&s->pool,
+               struct sized_iovs *siov = mm_alloc(&ctx->pool,
                                sizeof(*siov) + iovcnt * sizeof(struct iovec));
                kr_require(siov);
                knot_wire_write_u16(siov->nlen, ctx->payload.buffer.len);
@@ -2256,14 +2234,11 @@ static enum protolayer_iter_cb_result pl_dns_stream_wrap(
                        .iov_len = ctx->payload.buffer.len
                };
 
-               stream->sent.mem = siov;
-               stream->sent.pool = &s->pool;
-
                ctx->payload = protolayer_payload_iovec(siov->iovs, iovcnt, false);
                return protolayer_continue(ctx);
        } else if (ctx->payload.type == PROTOLAYER_PAYLOAD_IOVEC) {
                const int iovcnt = 1 + ctx->payload.iovec.cnt;
-               struct sized_iovs *siov = mm_alloc(&s->pool,
+               struct sized_iovs *siov = mm_alloc(&ctx->pool,
                                sizeof(*siov) + iovcnt * sizeof(struct iovec));
                kr_require(siov);
 
@@ -2282,9 +2257,6 @@ static enum protolayer_iter_cb_result pl_dns_stream_wrap(
                        .iov_len = sizeof(siov->nlen)
                };
 
-               stream->sent.mem = siov;
-               stream->sent.pool = &s->pool;
-
                ctx->payload = protolayer_payload_iovec(siov->iovs, iovcnt, false);
                return protolayer_continue(ctx);
        } else {
@@ -2323,10 +2295,8 @@ int worker_init(void)
        };
        const struct protolayer_globals stream_common = {
                .sess_size = sizeof(struct pl_dns_stream_sess_data),
-               .iter_size = sizeof(struct pl_dns_stream_iter_data),
                .wire_buf_overhead = KNOT_WIRE_MAX_PKTSIZE,
                .sess_init = NULL, /* replaced in specific layers below */
-               .iter_deinit = pl_dns_stream_iter_deinit,
                .unwrap = pl_dns_stream_unwrap,
                .wrap = pl_dns_stream_wrap,
                .event_unwrap = pl_dns_stream_event_unwrap,