Bluetooth: L2CAP: simplify codepath for SDU TX #67212

jori-nordic · 2024-01-04T16:29:15Z

See commits.

TLDR:

always send SDUs from workqueue context
~~remove~~ fix support for net_buf_frags

jori-nordic · 2024-01-04T16:29:48Z

Stacked on #67205 . Will get out of draft when merged.

If $BASH_SOURCE is the empty string, then those two tests end up with the same simulation ID. That's not a good time. Signed-off-by: Jonathan Rico <[email protected]>

Thalley

A few minor comments, otherwise LGTM

Thalley · 2024-01-08T09:13:27Z

include/zephyr/bluetooth/l2cap.h

@@ -579,22 +579,20 @@ int bt_l2cap_chan_disconnect(struct bt_l2cap_chan *chan);
 *  size the buffers for the for the outgoing buffer pool.
 *
 *  When sending L2CAP data over an LE connection the application is sending
- *  L2CAP SDUs. The application can optionally reserve
+ *  L2CAP SDUs. The application should reserve


When providing suggestions (like should) I find it helpful to also explain why.

Why should the application reserve this? What happens if it does/doesn't?

The migration guide now also states that it is required, but that does not fit with "should".

@jori-nordic Should this be changed to a shall?

subsys/bluetooth/host/l2cap.c

Thalley · 2024-01-08T09:27:26Z

tests/bsim/bluetooth/host/l2cap/credits/src/main.c

@@ -23,7 +23,8 @@ CREATE_FLAG(flag_l2cap_connected);
 #define L2CAP_MTU       (2 * SDU_LEN)

 /* Only one SDU transmitted or received at a time */
-NET_BUF_POOL_DEFINE(sdu_pool, 1, L2CAP_MTU, CONFIG_BT_CONN_TX_USER_DATA_SIZE, NULL);
+NET_BUF_POOL_DEFINE(sdu_pool, 1, L2CAP_MTU + BT_L2CAP_SDU_HDR_SIZE,


Suggested change

NET_BUF_POOL_DEFINE(sdu_pool, 1, L2CAP_MTU + BT_L2CAP_SDU_HDR_SIZE,

NET_BUF_POOL_DEFINE(sdu_pool, 1, BT_L2CAP_SDU_BUF_SIZE(L2CAP_MTU),

not really, L2CAP_MTU here already has the normal L2 header included.

Does it?

CONFIG_BT_L2CAP_TX_MTU is usually used with BT_L2CAP_SDU_BUF_SIZE, so that is without the header, right?

L2CAP_MTU is a multiple of SDU_LEN which is a multiple of L2CAP_MPS which is defined as CONFIG_BT_L2CAP_TX_MTU

Given that they are multiples, and not using a constant addition to the size, I would assume that none of the above are including the header size (granted, due the multiples, they can certainly fit it, but none of them are adding BT_L2CAP_HDR_SIZE).

If the intention is that L2CAP_MTU contains the size of the header, then I feel the above macro definitions are not clear about that (at least not to me). That being said, it is also unclear to me why we are multiplying by the MTU by 4 (2 * 2) in the first place, so I might just be missing something here.

Yes, you're right my bad. MTU & MPS both don't include headers. Updated.

jori-nordic · 2024-01-10T09:49:06Z

@rlubos FYI.
Do you know if IPSP / Bluetooth L2 net uses buffer fragments?

rlubos · 2024-01-10T10:01:58Z

Do you know if IPSP / Bluetooth L2 net uses buffer fragments?

I might be a bit out of context here, what do you mean by "buffer fragments"?

jori-nordic · 2024-01-10T10:15:11Z

Do you know if IPSP / Bluetooth L2 net uses buffer fragments?

I might be a bit out of context here, what do you mean by "buffer fragments"?

this: https://github.com/jori-nordic/zephyr/blob/303b2492d69ed375e069eba09f4ad7f8588c382b/include/zephyr/net/buf.h#L2336

rlubos · 2024-01-10T11:05:01Z

Do you know if IPSP / Bluetooth L2 net uses buffer fragments?

I might be a bit out of context here, what do you mean by "buffer fragments"?

this: https://github.com/jori-nordic/zephyr/blob/303b2492d69ed375e069eba09f4ad7f8588c382b/include/zephyr/net/buf.h#L2336

Ah, so net_buf->frags is generally used by the IP stack to chain net bufs, and given that Bluetooth L2 MTU is 1280, then yes, data will likely be fragmented over several net bufs.

jori-nordic · 2024-01-10T11:10:59Z

@rlubos is there a way to test the IP L2 transport without involving a linux machine?
Ie I want to exercise those network fragments, but only involve zephyr devices.

rlubos · 2024-01-10T11:47:03Z

@rlubos is there a way to test the IP L2 transport without involving a linux machine?

If you meant IPSP L2, the only valid testing scenario I know involes Linux host (as described in IPSP sample docs). But I don't claim to be BLE expert, i. e. I don't know if it's possible to test device vs device.

jori-nordic · 2024-01-10T14:32:56Z

Since @rlubos told me we had at least one user of frags, I decided to support them properly with a test.

Thalley · 2024-01-11T10:14:29Z

include/zephyr/bluetooth/l2cap.h

@@ -579,22 +579,20 @@ int bt_l2cap_chan_disconnect(struct bt_l2cap_chan *chan);
 *  size the buffers for the for the outgoing buffer pool.
 *
 *  When sending L2CAP data over an LE connection the application is sending
- *  L2CAP SDUs. The application can optionally reserve
+ *  L2CAP SDUs. The application should reserve


@jori-nordic Should this be changed to a shall?

Thalley · 2024-01-11T10:16:29Z

tests/bsim/bluetooth/host/l2cap/frags/src/main.c

+CREATE_FLAG(is_connected);
+CREATE_FLAG(flag_l2cap_connected);
+
+#define SMOL_SIZE 10


Should this have been

Suggested change

#define SMOL_SIZE 10

#define SMALL_SIZE 10

I'll un-meme it for you :)

Hah, I was wondering if that was the case (especially with chonk) - And while that is indeed fun, I think a less meme-oriented naming convention is better here :D

tests/bsim/bluetooth/host/l2cap/frags/src/main.c

jori-nordic · 2024-01-11T10:31:42Z

@jori-nordic Should this be changed to a shall?

huh I thought I had made the change. another one lost in the sands of ~~time~~ git rebase.

The purpose of this change is to help reason about what happens when a application sends an SDU. After this patch, the SDU is always added the channel's TX queue. The queue is always processed from system workqueue context. This change also enforces the application to reserve `BT_L2CAP_SDU_CHAN_SEND_RESERVE` bytes in the buffer it passes to L2CAP. This is to avoid unnecessary fragmentation and code complexity. Signed-off-by: Jonathan Rico <[email protected]>

It seems like a nice idea at first, but leads to hard-to-debug situations for the application. The previous behavior can be implemented by the app by defining `alloc_seg` and allocating from the same pool as `buf`. Signed-off-by: Jonathan Rico <[email protected]>

Fix the handling of buffers with fragments. Add a test to regression too. The main user of this is IPSP. Signed-off-by: Jonathan Rico <[email protected]>

jori-nordic · 2024-01-11T10:34:59Z

@Thalley there were some net_buf_ref issues I had to debug this morning, should be good for review (if CI passes) now.

Thalley · 2024-01-11T14:59:24Z

@jori-nordic Should this be changed to a shall?

huh I thought I had made the change. another one lost in the sands of ~~time~~ git rebase.

Yeah, I feel like I saw it fixed earlier as well, and then somehow reverted :s

alwa-nordic

Notes from offline review

alwa-nordic · 2024-01-11T15:02:42Z

doc/releases/migration-guide-3.6.rst

@@ -212,6 +212,10 @@ Bluetooth
  Any pointer to a UUID must be prefixed with `const`, otherwise there will be a compilation warning.
  For example change ``struct bt_uuid *uuid = BT_UUID_DECLARE_16(xx)`` to
  ``const struct bt_uuid *uuid = BT_UUID_DECLARE_16(xx)``. (:github:`66136`)
+* The :c:func:`bt_l2cap_chan_send` API now doesn't return the number of bytes sent.


This should go in the change log. Also add that it never returned anything useful.

alwa-nordic · 2024-01-11T15:08:19Z

include/zephyr/bluetooth/l2cap.h

+ *  The application can use the BT_L2CAP_SDU_BUF_SIZE() helper to correctly size
+ *  the buffers for the outgoing buffer pool.


Suggested change

* The application can use the BT_L2CAP_SDU_BUF_SIZE() helper to correctly size

* the buffers for the outgoing buffer pool.

* The application can use the BT_L2CAP_SDU_BUF_SIZE() helper to correctly size

* the buffers to account for the reserved headroom.

alwa-nordic · 2024-01-11T15:10:14Z

include/zephyr/bluetooth/l2cap.h

 *
 *  @note Buffer ownership is transferred to the stack in case of success, in
 *  case of an error the caller retains the ownership of the buffer.
 *
- *  @return Bytes sent in case of success or negative value in case of error.
+ *  @return 0 in case of success or negative value in case of error. See below.
+ *  @return -EINVAL if `buf` or `chan` is NULL.


alwa-nordic · 2024-01-11T15:14:16Z

subsys/bluetooth/host/l2cap.c


 	/* Resume tx in case there are buffers in the queue */
-	while ((buf = l2cap_chan_le_get_tx_buf(ch))) {
-		int sent = l2cap_tx_meta_data(buf)->sent;
+	while ((buf = l2cap_chan_le_get_tx_buf(le_chan))) {


Separate commit

alwa-nordic · 2024-01-11T15:20:29Z

subsys/bluetooth/host/l2cap.c

 	if (ch->chan.ops->alloc_seg) {
+		/* Use the dedicated segment callback if registered */


"Use the above user-defined al-locator."

alwa-nordic · 2024-01-11T15:23:32Z

subsys/bluetooth/host/l2cap.c

-		goto segment;
+	if (!seg) {
+		/* Try to use original pool if possible */
+		seg = net_buf_alloc(pool, K_NO_WAIT);


/* Y O I N K */

alwa-nordic · 2024-01-11T15:31:47Z

subsys/bluetooth/host/l2cap.c

+			return -EAGAIN;
+		}
+
+		/* Don't send more that TX MPS */


alwa-nordic · 2024-01-11T15:39:01Z

subsys/bluetooth/host/l2cap.c

-		return -EMSGSIZE;
-	}
-
+	/* Those are _not_ HCI fragments nor L2CAP segments. They are the


"The frag here does not refer to"

jori-nordic · 2024-01-11T17:34:44Z

I will split some stuff out, open multiple PRs instead tomorrow.

jori-nordic force-pushed the l2cap-single-tx branch 3 times, most recently from b0bbab8 to 43355cc Compare January 5, 2024 09:23

jori-nordic requested a review from theob-pro January 5, 2024 09:31

jori-nordic force-pushed the l2cap-single-tx branch from 43355cc to 5238eeb Compare January 5, 2024 09:55

tests: Bluetooth: use unique IDs for l2cap bsim tests

a93a2c2

If $BASH_SOURCE is the empty string, then those two tests end up with the same simulation ID. That's not a good time. Signed-off-by: Jonathan Rico <[email protected]>

jori-nordic force-pushed the l2cap-single-tx branch from 5238eeb to 24b1a5e Compare January 5, 2024 15:52

jori-nordic marked this pull request as ready for review January 5, 2024 15:52

zephyrbot added area: Bluetooth Host Bluetooth Host (excluding BR/EDR) Release Notes To be mentioned in the release notes area: Bluetooth labels Jan 5, 2024

zephyrbot requested review from alwa-nordic, asbjornsabo, henrikbrixandersen, hermabe, jhedberg, kartben, kruithofa, MaureenHelm, sjanc, Thalley and Vudentz January 5, 2024 15:53

zephyrbot assigned jhedberg Jan 5, 2024

Thalley requested changes Jan 8, 2024

View reviewed changes

jori-nordic mentioned this pull request Jan 9, 2024

Bluetooth: L2CAP: remove metadata requirement #67386

Merged

jori-nordic force-pushed the l2cap-single-tx branch 2 times, most recently from 3e0dd12 to 1508fea Compare January 9, 2024 15:09

Thalley previously approved these changes Jan 9, 2024

View reviewed changes

jori-nordic dismissed Thalley’s stale review via 2cb25b7 January 10, 2024 09:45

jori-nordic force-pushed the l2cap-single-tx branch from 1508fea to 2cb25b7 Compare January 10, 2024 09:45

Thalley previously approved these changes Jan 10, 2024

View reviewed changes

jori-nordic dismissed Thalley’s stale review via 97dd185 January 10, 2024 14:30

jori-nordic force-pushed the l2cap-single-tx branch from 2cb25b7 to 97dd185 Compare January 10, 2024 14:30

jori-nordic force-pushed the l2cap-single-tx branch 3 times, most recently from 0ab4545 to 38557f8 Compare January 10, 2024 16:13

Thalley requested changes Jan 11, 2024

View reviewed changes

jori-nordic added 3 commits January 11, 2024 11:33

Bluetooth: L2CAP: fix net buf frags handling

68de060

Fix the handling of buffers with fragments. Add a test to regression too. The main user of this is IPSP. Signed-off-by: Jonathan Rico <[email protected]>

jori-nordic force-pushed the l2cap-single-tx branch from 38557f8 to 68de060 Compare January 11, 2024 10:34

Thalley approved these changes Jan 11, 2024

View reviewed changes

alwa-nordic requested changes Jan 11, 2024

View reviewed changes

jori-nordic closed this Jan 11, 2024

jori-nordic mentioned this pull request Jan 12, 2024

Bluetooth: L2CAP patches tracker #67511

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: L2CAP: simplify codepath for SDU TX #67212

Bluetooth: L2CAP: simplify codepath for SDU TX #67212

jori-nordic commented Jan 4, 2024 •

edited

Loading

jori-nordic commented Jan 4, 2024

Thalley left a comment

Thalley Jan 8, 2024

Thalley Jan 11, 2024

Thalley Jan 8, 2024

jori-nordic Jan 9, 2024

Thalley Jan 9, 2024

jori-nordic Jan 10, 2024

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

Thalley Jan 11, 2024

Thalley Jan 11, 2024

jori-nordic Jan 11, 2024

Thalley Jan 11, 2024

jori-nordic commented Jan 11, 2024

jori-nordic commented Jan 11, 2024

Thalley commented Jan 11, 2024

alwa-nordic left a comment

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

alwa-nordic Jan 11, 2024

jori-nordic commented Jan 11, 2024

	NET_BUF_POOL_DEFINE(sdu_pool, 1, L2CAP_MTU + BT_L2CAP_SDU_HDR_SIZE,
	NET_BUF_POOL_DEFINE(sdu_pool, 1, BT_L2CAP_SDU_BUF_SIZE(L2CAP_MTU),

		* The application can use the BT_L2CAP_SDU_BUF_SIZE() helper to correctly size
		* the buffers for the outgoing buffer pool.

		if (ch->chan.ops->alloc_seg) {
		/* Use the dedicated segment callback if registered */

Bluetooth: L2CAP: simplify codepath for SDU TX #67212

Bluetooth: L2CAP: simplify codepath for SDU TX #67212

Conversation

jori-nordic commented Jan 4, 2024 • edited Loading

jori-nordic commented Jan 4, 2024

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

rlubos commented Jan 10, 2024

jori-nordic commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jori-nordic commented Jan 11, 2024

jori-nordic commented Jan 11, 2024

Thalley commented Jan 11, 2024

alwa-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jori-nordic commented Jan 11, 2024

jori-nordic commented Jan 4, 2024 •

edited

Loading