From: Oto Šťáva Date: Tue, 28 May 2024 14:54:29 +0000 (+0200) Subject: amend! daemon/session2: support multiple short-lived buffers X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fenvironments%2Fdocs-develop-nits-2dntdj%2Fdeployments%2F4198;p=thirdparty%2Fknot-resolver.git amend! daemon/session2: support multiple short-lived buffers 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. --- diff --git a/daemon/session2.c b/daemon/session2.c index ca06e47be..e269844c8 100644 --- a/daemon/session2.c +++ b/daemon/session2.c @@ -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); diff --git a/daemon/session2.h b/daemon/session2.h index eac8da3d6..ada57af25 100644 --- a/daemon/session2.h +++ b/daemon/session2.h @@ -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. */ diff --git a/daemon/worker.c b/daemon/worker.c index a73420458..5473c8b89 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -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,