From 3d306c181f0d75c5765f80fdfbee614e1e3f191d Mon Sep 17 00:00:00 2001 From: Carles Cufi Date: Fri, 2 Dec 2022 14:13:07 +0100 Subject: [PATCH] net: buf: Simplify fragment handling This patch reworks how fragments are handled in the net_buf infrastructure. In particular, it removes the union around the node and frags members in the main net_buf structure. This is done so that both can be used at the same time, at a cost of 4 bytes per net_buf instance. This implies that the layout of net_buf instances changes whenever being inserted into a queue (fifo or lifo) or a linked list (slist). Until now, this is what happened when enqueueing a net_buf with frags in a queue or linked list: 1.1 Before enqueueing: +--------+ +--------+ +--------+ |#1 node|\ |#2 node|\ |#3 node|\ | | \ | | \ | | \ | frags |------| frags |------| frags |------NULL +--------+ +--------+ +--------+ net_buf #1 has 2 fragments, net_bufs #2 and #3. Both the node and frags pointers (they are the same, since they are unioned) point to the next fragment. 1.2 After enqueueing: +--------+ +--------+ +--------+ +--------+ +--------+ |q/slist |------|#1 node|------|#2 node|------|#3 node|------|q/slist | |node | | *flag | / | *flag | / | | / |node | | | | frags |/ | frags |/ | frags |/ | | +--------+ +--------+ +--------+ +--------+ +--------+ When enqueing a net_buf (in this case #1) that contains fragments, the current net_buf implementation actually enqueues all the fragments (in this case #2 and #3) as actual queue/slist items, since node and frags are one and the same in memory. This makes the enqueuing operation expensive and it makes it impossible to atomically dequeue. The `*flag` notation here means that the `flags` member has been set to `NET_BUF_FRAGS` in order to be able to reconstruct the frags pointers when dequeuing. After this patch, the layout changes considerably: 2.1 Before enqueueing: +--------+ +--------+ +--------+ |#1 node|--NULL |#2 node|--NULL |#3 node|--NULL | | | | | | | frags |-------| frags |-------| frags |------NULL +--------+ +--------+ +--------+ This is very similar to 1.1, except that now node and frags are different pointers, so node is just set to NULL. 2.2 After enqueueing: +--------+ +--------+ +--------+ |q/slist |-------|#1 node|-------|q/slist | |node | | | |node | | | | frags | | | +--------+ +--------+ +--------+ | +--------+ +--------+ | |#2 node|--NULL |#3 node|--NULL | | | | | +------------| frags |-------| frags |------NULL +--------+ +--------+ When enqueuing net_buf #1, now we only enqueue that very item, instead of enqueing the frags as well, since now node and frags are separate pointers. This simplifies the operation and makes it atomic. Resolves #52718. Signed-off-by: Carles Cufi --- include/zephyr/net/buf.h | 21 +++++-------------- subsys/net/buf.c | 45 ++++------------------------------------ 2 files changed, 9 insertions(+), 57 deletions(-) diff --git a/include/zephyr/net/buf.h b/include/zephyr/net/buf.h index f869dfab5a90e7..c39f3dd561c62d 100644 --- a/include/zephyr/net/buf.h +++ b/include/zephyr/net/buf.h @@ -884,15 +884,6 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf, buf->len = state->len; } -/** - * Flag indicating that the buffer has associated fragments. Only used - * internally by the buffer handling code while the buffer is inside a - * FIFO, meaning this never needs to be explicitly set or unset by the - * net_buf API user. As long as the buffer is outside of a FIFO, i.e. - * in practice always for the user for this API, the buf->frags pointer - * should be used instead. - */ -#define NET_BUF_FRAGS BIT(0) /** * Flag indicating that the buffer's associated data pointer, points to * externally allocated memory. Therefore once ref goes down to zero, the @@ -902,7 +893,7 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf, * Reference count mechanism however will behave the same way, and ref * count going to 0 will free the net_buf but no the data pointer in it. */ -#define NET_BUF_EXTERNAL_DATA BIT(1) +#define NET_BUF_EXTERNAL_DATA BIT(0) /** * @brief Network buffer representation. @@ -912,13 +903,11 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf, * using the net_buf_alloc() API. */ struct net_buf { - union { - /** Allow placing the buffer into sys_slist_t */ - sys_snode_t node; + /** Allow placing the buffer into sys_slist_t */ + sys_snode_t node; - /** Fragments associated with this buffer. */ - struct net_buf *frags; - }; + /** Fragments associated with this buffer. */ + struct net_buf *frags; /** Reference count. */ uint8_t ref; diff --git a/subsys/net/buf.c b/subsys/net/buf.c index 887e75e4be80d9..dc6f5dc8e0d278 100644 --- a/subsys/net/buf.c +++ b/subsys/net/buf.c @@ -412,7 +412,7 @@ struct net_buf *net_buf_get_debug(struct k_fifo *fifo, k_timeout_t timeout, struct net_buf *net_buf_get(struct k_fifo *fifo, k_timeout_t timeout) #endif { - struct net_buf *buf, *frag; + struct net_buf *buf; NET_BUF_DBG("%s():%d: fifo %p", func, line, fifo); @@ -423,18 +423,6 @@ struct net_buf *net_buf_get(struct k_fifo *fifo, k_timeout_t timeout) NET_BUF_DBG("%s():%d: buf %p fifo %p", func, line, buf, fifo); - /* Get any fragments belonging to this buffer */ - for (frag = buf; (frag->flags & NET_BUF_FRAGS); frag = frag->frags) { - frag->frags = k_fifo_get(fifo, K_NO_WAIT); - __ASSERT_NO_MSG(frag->frags); - - /* The fragments flag is only for FIFO-internal usage */ - frag->flags &= ~NET_BUF_FRAGS; - } - - /* Mark the end of the fragment list */ - frag->frags = NULL; - return buf; } @@ -460,24 +448,19 @@ static struct k_spinlock net_buf_slist_lock; void net_buf_slist_put(sys_slist_t *list, struct net_buf *buf) { - struct net_buf *tail; k_spinlock_key_t key; __ASSERT_NO_MSG(list); __ASSERT_NO_MSG(buf); - for (tail = buf; tail->frags; tail = tail->frags) { - tail->flags |= NET_BUF_FRAGS; - } - key = k_spin_lock(&net_buf_slist_lock); - sys_slist_append_list(list, &buf->node, &tail->node); + sys_slist_append(list, &buf->node); k_spin_unlock(&net_buf_slist_lock, key); } struct net_buf *net_buf_slist_get(sys_slist_t *list) { - struct net_buf *buf, *frag; + struct net_buf *buf; k_spinlock_key_t key; __ASSERT_NO_MSG(list); @@ -486,20 +469,6 @@ struct net_buf *net_buf_slist_get(sys_slist_t *list) buf = (void *)sys_slist_get(list); - if (buf) { - /* Get any fragments belonging to this buffer */ - for (frag = buf; (frag->flags & NET_BUF_FRAGS); frag = frag->frags) { - frag->frags = (void *)sys_slist_get(list); - __ASSERT_NO_MSG(frag->frags); - - /* The fragments flag is only for list-internal usage */ - frag->flags &= ~NET_BUF_FRAGS; - } - - /* Mark the end of the fragment list */ - frag->frags = NULL; - } - k_spin_unlock(&net_buf_slist_lock, key); return buf; @@ -507,16 +476,10 @@ struct net_buf *net_buf_slist_get(sys_slist_t *list) void net_buf_put(struct k_fifo *fifo, struct net_buf *buf) { - struct net_buf *tail; - __ASSERT_NO_MSG(fifo); __ASSERT_NO_MSG(buf); - for (tail = buf; tail->frags; tail = tail->frags) { - tail->flags |= NET_BUF_FRAGS; - } - - k_fifo_put_list(fifo, buf, tail); + k_fifo_put(fifo, buf); } #if defined(CONFIG_NET_BUF_LOG)