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 <carles.cufi@nordicsemi.no>
diff --git a/include/zephyr/net/buf.h b/include/zephyr/net/buf.h
index f869dfa..c39f3dd 100644
--- a/include/zephyr/net/buf.h
+++ b/include/zephyr/net/buf.h
@@ -885,15 +885,6 @@
}
/**
- * 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
* pointed data will not need to be deallocated. This never needs to be
@@ -902,7 +893,7 @@
* 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 @@
* 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 887e75e..dc6f5dc 100644
--- a/subsys/net/buf.c
+++ b/subsys/net/buf.c
@@ -412,7 +412,7 @@
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 @@
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 @@
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 @@
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 @@
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)
