michaelh/zephyr
1
0
Fork 0
Code Releases Activity

net: buf: Simplify fragment handling

Browse source 
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>
This commit is contained in:
Carles Cufi 2022-12-02 14:13:07 +01:00 • committed by Johan Hedberg
commit 3d306c181f
2 changed files with 9 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

45
subsys/net/buf.c
View file

@ -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)