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 zephyrproject-rtos#52718.

Signed-off-by: Carles Cufi <[email protected]>
(cherry picked from commit 3d306c1)

Author: carlescufi

include/net/buf.h

@@ -889,15 +889,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
@@ -907,7 +898,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.
@@ -917,13 +908,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;

subsys/net/buf.c

@@ -405,7 +405,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);
 
@@ -416,18 +416,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;
 }
 
@@ -451,24 +439,19 @@ void net_buf_simple_reserve(struct net_buf_simple *buf, size_t reserve)
 
 void net_buf_slist_put(sys_slist_t *list, struct net_buf *buf)
 {
-	struct net_buf *tail;
 	unsigned int key;
 
 	__ASSERT_NO_MSG(list);
 	__ASSERT_NO_MSG(buf);
 
-	for (tail = buf; tail->frags; tail = tail->frags) {
-		tail->flags |= NET_BUF_FRAGS;
-	}
-
 	key = irq_lock();
-	sys_slist_append_list(list, &buf->node, &tail->node);
+	sys_slist_append(list, &buf->node);
 	irq_unlock(key);
 }
 
 struct net_buf *net_buf_slist_get(sys_slist_t *list)
 {
-	struct net_buf *buf, *frag;
+	struct net_buf *buf;
 	unsigned int key;
 
 	__ASSERT_NO_MSG(list);
@@ -477,40 +460,15 @@ struct net_buf *net_buf_slist_get(sys_slist_t *list)
 	buf = (void *)sys_slist_get(list);
 	irq_unlock(key);
 
-	if (!buf) {
-		return NULL;
-	}
-
-	/* Get any fragments belonging to this buffer */
-	for (frag = buf; (frag->flags & NET_BUF_FRAGS); frag = frag->frags) {
-		key = irq_lock();
-		frag->frags = (void *)sys_slist_get(list);
-		irq_unlock(key);
-
-		__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;
-
 	return buf;
 }
 
 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)