Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: buf: Simplify fragment handling #52760

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

carlescufi
Copy link
Member

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 [email protected]

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]>
jfischer-no
jfischer-no previously approved these changes Dec 2, 2022
jori-nordic
jori-nordic previously approved these changes Dec 2, 2022
@carlescufi carlescufi added the DNM This PR should not be merged (Do Not Merge) label Dec 2, 2022
@carlescufi carlescufi removed the request for review from pfalcon December 2, 2022 17:13
desowin
desowin previously approved these changes Dec 5, 2022
Copy link

@desowin desowin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, the put and get operations are now O(1) instead of O(N) where N is the number of linked in fragments.

@carlescufi
Copy link
Member Author

In other words, the put and get operations are now O(1) instead of O(N) where N is the number of linked in fragments.

Exactly

tbursztyka
tbursztyka previously approved these changes Dec 5, 2022
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok as an intermediate change.

Shouldn't we remove frags pointer altogether at some point btw? To get back to smaller size, but also it would force code to use slist API to traverse net_buf etc... will be cleaner.

rlubos
rlubos previously approved these changes Dec 5, 2022
jukkar
jukkar previously approved these changes Dec 5, 2022
nordicjm
nordicjm previously approved these changes Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify handling of fragments in net_buf