LinuxLists.cc - [PATCH] l2cap: do not return LE flow credits when buf full

2024-04-05 09:41:10

Subject: [PATCH] l2cap: do not return LE flow credits when buf full

Previously LE flow credits were returned to the
sender even if the socket's receive buffer was
full. This meant that no back-pressure
was applied to the sender, thus it continued to
send data, resulting in data loss without any
error being reported.

This is fixed by stopping the return of LE flow
credits when the receive buffer of an L2CAP socket
is full. Returning of the credits is resumed, once
the receive buffer is half-empty.

Already received data is temporary stored within
l2cap_pinfo, since Bluetooth LE provides no
retransmission mechanism once the data has been
acked by the physical layer.

Signed-off-by: Sebastian Urban <[email protected]>
---
include/net/bluetooth/l2cap.h | 7 ++++-
net/bluetooth/l2cap_core.c | 38 ++++++++++++++++++++++---
net/bluetooth/l2cap_sock.c | 53 ++++++++++++++++++++++++++---------
3 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92d7197f9a56..230c14ea944c 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -682,10 +682,15 @@ struct l2cap_user {
/* ----- L2CAP socket info ----- */
#define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)

+struct l2cap_rx_busy {
+ struct list_head list;
+ struct sk_buff *skb;
+};
+
struct l2cap_pinfo {
struct bt_sock bt;
struct l2cap_chan *chan;
- struct sk_buff *rx_busy_skb;
+ struct list_head rx_busy;
};

enum {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ab5a9d42fae7..c78af7fad255 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -63,6 +63,8 @@ static void l2cap_retrans_timeout(struct work_struct *work);
static void l2cap_monitor_timeout(struct work_struct *work);
static void l2cap_ack_timeout(struct work_struct *work);

+static void l2cap_chan_le_send_credits(struct l2cap_chan *chan);
+
static inline u8 bdaddr_type(u8 link_type, u8 bdaddr_type)
{
if (link_type == LE_LINK) {
@@ -5714,17 +5716,34 @@ static int l2cap_resegment(struct l2cap_chan *chan)
return 0;
}

-void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
+static void l2cap_chan_busy_ertm(struct l2cap_chan *chan, int busy)
{
u8 event;

- if (chan->mode != L2CAP_MODE_ERTM)
- return;
-
event = busy ? L2CAP_EV_LOCAL_BUSY_DETECTED : L2CAP_EV_LOCAL_BUSY_CLEAR;
l2cap_tx(chan, NULL, NULL, event);
}

+static void l2cap_chan_busy_le(struct l2cap_chan *chan, int busy)
+{
+ if (busy) {
+ set_bit(CONN_LOCAL_BUSY, &chan->conn_state);
+ } else {
+ clear_bit(CONN_LOCAL_BUSY, &chan->conn_state);
+ l2cap_chan_le_send_credits(chan);
+ }
+}
+
+void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
+{
+ if (chan->mode == L2CAP_MODE_ERTM) {
+ l2cap_chan_busy_ertm(chan, busy);
+ } else if (chan->mode == L2CAP_MODE_LE_FLOWCTL ||
+ chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
+ l2cap_chan_busy_le(chan, busy);
+ }
+}
+
static int l2cap_rx_queued_iframes(struct l2cap_chan *chan)
{
int err = 0;
@@ -6514,6 +6533,11 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
struct l2cap_le_credits pkt;
u16 return_credits;

+ if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
+ BT_DBG("busy chan %p not returning credits to sender", chan);
+ return;
+ }
+
return_credits = (chan->imtu / chan->mps) + 1;

if (chan->rx_credits >= return_credits)
@@ -6542,6 +6566,12 @@ static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
/* Wait recv to confirm reception before updating the credits */
err = chan->ops->recv(chan, skb);

+ if (err < 0) {
+ BT_ERR("Queueing received LE L2CAP data failed");
+ l2cap_send_disconn_req(chan, ECONNRESET);
+ return err;
+ }
+
/* Update credits whenever an SDU is received */
l2cap_chan_le_send_credits(chan);

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ee7a41d6994f..3b0fb6e0b61b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1177,7 +1177,9 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
else
err = bt_sock_recvmsg(sock, msg, len, flags);

- if (pi->chan->mode != L2CAP_MODE_ERTM)
+ if (pi->chan->mode != L2CAP_MODE_ERTM &&
+ pi->chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ pi->chan->mode != L2CAP_MODE_EXT_FLOWCTL)
return err;

/* Attempt to put pending rx data in the socket buffer */
@@ -1187,11 +1189,15 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
goto done;

- if (pi->rx_busy_skb) {
- if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
- pi->rx_busy_skb = NULL;
- else
+ while (!list_empty(&pi->rx_busy)) {
+ struct l2cap_rx_busy *rx_busy =
+ list_first_entry(&pi->rx_busy,
+ struct l2cap_rx_busy,
+ list);
+ if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
goto done;
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

/* Restore data flow when half of the receive buffer is
@@ -1459,17 +1465,20 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
{
struct sock *sk = chan->data;
+ struct l2cap_pinfo *pi = l2cap_pi(sk);
int err;

lock_sock(sk);

- if (l2cap_pi(sk)->rx_busy_skb) {
+ if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
err = -ENOMEM;
goto done;
}

if (chan->mode != L2CAP_MODE_ERTM &&
- chan->mode != L2CAP_MODE_STREAMING) {
+ chan->mode != L2CAP_MODE_STREAMING &&
+ chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ chan->mode != L2CAP_MODE_EXT_FLOWCTL) {
/* Even if no filter is attached, we could potentially
* get errors from security modules, etc.
*/
@@ -1480,17 +1489,28 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)

err = __sock_queue_rcv_skb(sk, skb);

- /* For ERTM, handle one skb that doesn't fit into the recv
+ /* For ERTM and LE, handle a skb that doesn't fit into the recv
* buffer. This is important to do because the data frames
* have already been acked, so the skb cannot be discarded.
*
* Notify the l2cap core that the buffer is full, so the
* LOCAL_BUSY state is entered and no more frames are
* acked and reassembled until there is buffer space
- * available.
+ * available. In the case of LE this blocks returning of flow
+ * credits.
*/
- if (err < 0 && chan->mode == L2CAP_MODE_ERTM) {
- l2cap_pi(sk)->rx_busy_skb = skb;
+ if (err < 0 &&
+ (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_LE_FLOWCTL ||
+ chan->mode == L2CAP_MODE_EXT_FLOWCTL)) {
+ struct l2cap_rx_busy *rx_busy =
+ kmalloc(sizeof(*rx_busy), GFP_KERNEL);
+ if (!rx_busy) {
+ err = -ENOMEM;
+ goto done;
+ }
+ rx_busy->skb = skb;
+ list_add_tail(&rx_busy->list, &pi->rx_busy);
l2cap_chan_busy(chan, 1);
err = 0;
}
@@ -1716,6 +1736,8 @@ static const struct l2cap_ops l2cap_chan_ops = {

static void l2cap_sock_destruct(struct sock *sk)
{
+ struct l2cap_rx_busy *rx_busy, *next;
+
BT_DBG("sk %p", sk);

if (l2cap_pi(sk)->chan) {
@@ -1723,9 +1745,10 @@ static void l2cap_sock_destruct(struct sock *sk)
l2cap_chan_put(l2cap_pi(sk)->chan);
}

- if (l2cap_pi(sk)->rx_busy_skb) {
- kfree_skb(l2cap_pi(sk)->rx_busy_skb);
- l2cap_pi(sk)->rx_busy_skb = NULL;
+ list_for_each_entry_safe(rx_busy, next, &l2cap_pi(sk)->rx_busy, list) {
+ kfree_skb(rx_busy->skb);
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

skb_queue_purge(&sk->sk_receive_queue);
@@ -1830,6 +1853,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
sk->sk_destruct = l2cap_sock_destruct;
sk->sk_sndtimeo = L2CAP_CONN_TIMEOUT;

+ INIT_LIST_HEAD(&l2cap_pi(sk)->rx_busy);
+
chan = l2cap_chan_create();
if (!chan) {
sk_free(sk);
--
2.34.1

2024-04-05 10:27:15

by Sebastian Urban

[permalink] [raw]

Subject: [PATCH v2] Bluetooth: keep LE flow credits when recvbuf full

2024-04-05 10:33:49

by bluez.test.bot

[permalink] [raw]

Subject: RE: l2cap: do not return LE flow credits when buf full

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=841735

---Test result---

Test Summary:
CheckPatch PASS 1.13 seconds
GitLint PASS 0.29 seconds
SubjectPrefix FAIL 0.36 seconds
BuildKernel PASS 29.62 seconds
CheckAllWarning PASS 32.32 seconds
CheckSparse PASS 37.84 seconds
CheckSmatch FAIL 34.71 seconds
BuildKernel32 PASS 28.82 seconds
TestRunnerSetup PASS 517.48 seconds
TestRunner_l2cap-tester PASS 22.31 seconds
TestRunner_iso-tester PASS 32.29 seconds
TestRunner_bnep-tester PASS 4.77 seconds
TestRunner_mgmt-tester PASS 108.24 seconds
TestRunner_rfcomm-tester PASS 7.13 seconds
TestRunner_sco-tester PASS 14.91 seconds
TestRunner_ioctl-tester PASS 7.51 seconds
TestRunner_mesh-tester PASS 5.65 seconds
TestRunner_smp-tester PASS 6.60 seconds
TestRunner_userchan-tester PASS 4.86 seconds
IncrementalBuild PASS 27.56 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2

---
Regards,
Linux Bluetooth

2024-04-05 10:56:47

by bluez.test.bot

[permalink] [raw]

Subject: RE: [v2] Bluetooth: keep LE flow credits when recvbuf full

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=841749

---Test result---

Test Summary:
CheckPatch PASS 1.10 seconds
GitLint PASS 4.42 seconds
SubjectPrefix PASS 0.12 seconds
BuildKernel PASS 29.80 seconds
CheckAllWarning PASS 32.38 seconds
CheckSparse PASS 37.79 seconds
CheckSmatch FAIL 34.75 seconds
BuildKernel32 PASS 29.16 seconds
TestRunnerSetup PASS 516.52 seconds
TestRunner_l2cap-tester PASS 18.25 seconds
TestRunner_iso-tester FAIL 32.46 seconds
TestRunner_bnep-tester PASS 4.62 seconds
TestRunner_mgmt-tester FAIL 112.20 seconds
TestRunner_rfcomm-tester PASS 7.23 seconds
TestRunner_sco-tester PASS 14.88 seconds
TestRunner_ioctl-tester PASS 7.58 seconds
TestRunner_mesh-tester PASS 5.68 seconds
TestRunner_smp-tester PASS 6.66 seconds
TestRunner_userchan-tester PASS 4.81 seconds
IncrementalBuild PASS 28.17 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 121, Passed: 120 (99.2%), Failed: 1, Not Run: 0

Failed Test Cases
ISO Connect2 Suspend - Success Failed 6.168 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Remove Device 4 (Disable Adv) Timed out 2.157 seconds

---
Regards,
Linux Bluetooth

2024-04-05 15:32:42

by Luiz Augusto von Dentz

[permalink] [raw]

Subject: Re: [PATCH v2] Bluetooth: keep LE flow credits when recvbuf full

Hi Sebastian,

On Fri, Apr 5, 2024 at 6:26 AM Sebastian Urban <[email protected]> wrote:
>
> Previously LE flow credits were returned to the
> sender even if the socket's receive buffer was
> full. This meant that no back-pressure
> was applied to the sender, thus it continued to
> send data, resulting in data loss without any
> error being reported.
>
> This is fixed by stopping the return of LE flow
> credits when the receive buffer of an L2CAP socket
> is full. Returning of the credits is resumed, once
> the receive buffer is half-empty.
>
> Already received data is temporary stored within
> l2cap_pinfo, since Bluetooth LE provides no
> retransmission mechanism once the data has been
> acked by the physical layer.
>
> Signed-off-by: Sebastian Urban <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 7 ++++-
> net/bluetooth/l2cap_core.c | 38 ++++++++++++++++++++++---
> net/bluetooth/l2cap_sock.c | 53 ++++++++++++++++++++++++++---------
> 3 files changed, 79 insertions(+), 19 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 92d7197f9a56..230c14ea944c 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -682,10 +682,15 @@ struct l2cap_user {
> /* ----- L2CAP socket info ----- */
> #define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)
>
> +struct l2cap_rx_busy {
> + struct list_head list;
> + struct sk_buff *skb;
> +};

In theory we only really to queue 1 skb at most, since we would stop
giving credits, or perhaps this is because we had given enough credits
for MTU + 1, so the +1 segment could result in a second SDU/skb to be
completed while waiting the user space process to start reading again?

> struct l2cap_pinfo {
> struct bt_sock bt;
> struct l2cap_chan *chan;
> - struct sk_buff *rx_busy_skb;
> + struct list_head rx_busy;
> };
>
> enum {
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ab5a9d42fae7..c78af7fad255 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -63,6 +63,8 @@ static void l2cap_retrans_timeout(struct work_struct *work);
> static void l2cap_monitor_timeout(struct work_struct *work);
> static void l2cap_ack_timeout(struct work_struct *work);
>
> +static void l2cap_chan_le_send_credits(struct l2cap_chan *chan);

We probably need to change the way send_credits calculates the number
of credits to be restored, it needs to consider the actual available
buffer size at the socket rather then assuming we always shall have
space for MTU + 1, that way the remote side would always have the
exact information of how much buffer space is left. That said perhaps
we need a way to inform when user space reads then we need to call
into send_credits again.

> static inline u8 bdaddr_type(u8 link_type, u8 bdaddr_type)
> {
> if (link_type == LE_LINK) {
> @@ -5714,17 +5716,34 @@ static int l2cap_resegment(struct l2cap_chan *chan)
> return 0;
> }
>
> -void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
> +static void l2cap_chan_busy_ertm(struct l2cap_chan *chan, int busy)
> {
> u8 event;
>
> - if (chan->mode != L2CAP_MODE_ERTM)
> - return;
> -
> event = busy ? L2CAP_EV_LOCAL_BUSY_DETECTED : L2CAP_EV_LOCAL_BUSY_CLEAR;
> l2cap_tx(chan, NULL, NULL, event);
> }
>
> +static void l2cap_chan_busy_le(struct l2cap_chan *chan, int busy)
> +{
> + if (busy) {
> + set_bit(CONN_LOCAL_BUSY, &chan->conn_state);
> + } else {
> + clear_bit(CONN_LOCAL_BUSY, &chan->conn_state);
> + l2cap_chan_le_send_credits(chan);
> + }
> +}
> +
> +void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
> +{
> + if (chan->mode == L2CAP_MODE_ERTM) {
> + l2cap_chan_busy_ertm(chan, busy);
> + } else if (chan->mode == L2CAP_MODE_LE_FLOWCTL ||
> + chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
> + l2cap_chan_busy_le(chan, busy);
> + }
> +}
> +
> static int l2cap_rx_queued_iframes(struct l2cap_chan *chan)
> {
> int err = 0;
> @@ -6514,6 +6533,11 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
> struct l2cap_le_credits pkt;
> u16 return_credits;
>
> + if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
> + BT_DBG("busy chan %p not returning credits to sender", chan);
> + return;
> + }
> +
> return_credits = (chan->imtu / chan->mps) + 1;
>
> if (chan->rx_credits >= return_credits)
> @@ -6542,6 +6566,12 @@ static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
> /* Wait recv to confirm reception before updating the credits */
> err = chan->ops->recv(chan, skb);
>
> + if (err < 0) {
> + BT_ERR("Queueing received LE L2CAP data failed");
> + l2cap_send_disconn_req(chan, ECONNRESET);
> + return err;
> + }
> +
> /* Update credits whenever an SDU is received */
> l2cap_chan_le_send_credits(chan);
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ee7a41d6994f..3b0fb6e0b61b 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1177,7 +1177,9 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> else
> err = bt_sock_recvmsg(sock, msg, len, flags);
>
> - if (pi->chan->mode != L2CAP_MODE_ERTM)
> + if (pi->chan->mode != L2CAP_MODE_ERTM &&
> + pi->chan->mode != L2CAP_MODE_LE_FLOWCTL &&
> + pi->chan->mode != L2CAP_MODE_EXT_FLOWCTL)
> return err;
>
> /* Attempt to put pending rx data in the socket buffer */
> @@ -1187,11 +1189,15 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
> goto done;
>
> - if (pi->rx_busy_skb) {
> - if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
> - pi->rx_busy_skb = NULL;
> - else
> + while (!list_empty(&pi->rx_busy)) {
> + struct l2cap_rx_busy *rx_busy =
> + list_first_entry(&pi->rx_busy,
> + struct l2cap_rx_busy,
> + list);
> + if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
> goto done;
> + list_del(&rx_busy->list);
> + kfree(rx_busy);

I see now, this is trying to dequeue packets if the socket is read,
which in case we turn the send_credits function to calculate the
credits based on the socket buffer size that would not be necessary
but then we would need to call into send_credits here.

> }
>
> /* Restore data flow when half of the receive buffer is
> @@ -1459,17 +1465,20 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
> static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
> {
> struct sock *sk = chan->data;
> + struct l2cap_pinfo *pi = l2cap_pi(sk);
> int err;
>
> lock_sock(sk);
>
> - if (l2cap_pi(sk)->rx_busy_skb) {
> + if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
> err = -ENOMEM;
> goto done;
> }
>
> if (chan->mode != L2CAP_MODE_ERTM &&
> - chan->mode != L2CAP_MODE_STREAMING) {
> + chan->mode != L2CAP_MODE_STREAMING &&
> + chan->mode != L2CAP_MODE_LE_FLOWCTL &&
> + chan->mode != L2CAP_MODE_EXT_FLOWCTL) {
> /* Even if no filter is attached, we could potentially
> * get errors from security modules, etc.
> */
> @@ -1480,17 +1489,28 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
>
> err = __sock_queue_rcv_skb(sk, skb);
>
> - /* For ERTM, handle one skb that doesn't fit into the recv
> + /* For ERTM and LE, handle a skb that doesn't fit into the recv
> * buffer. This is important to do because the data frames
> * have already been acked, so the skb cannot be discarded.
> *
> * Notify the l2cap core that the buffer is full, so the
> * LOCAL_BUSY state is entered and no more frames are
> * acked and reassembled until there is buffer space
> - * available.
> + * available. In the case of LE this blocks returning of flow
> + * credits.
> */
> - if (err < 0 && chan->mode == L2CAP_MODE_ERTM) {
> - l2cap_pi(sk)->rx_busy_skb = skb;
> + if (err < 0 &&
> + (chan->mode == L2CAP_MODE_ERTM ||
> + chan->mode == L2CAP_MODE_LE_FLOWCTL ||
> + chan->mode == L2CAP_MODE_EXT_FLOWCTL)) {
> + struct l2cap_rx_busy *rx_busy =
> + kmalloc(sizeof(*rx_busy), GFP_KERNEL);
> + if (!rx_busy) {
> + err = -ENOMEM;
> + goto done;
> + }
> + rx_busy->skb = skb;
> + list_add_tail(&rx_busy->list, &pi->rx_busy);
> l2cap_chan_busy(chan, 1);
> err = 0;
> }
> @@ -1716,6 +1736,8 @@ static const struct l2cap_ops l2cap_chan_ops = {
>
> static void l2cap_sock_destruct(struct sock *sk)
> {
> + struct l2cap_rx_busy *rx_busy, *next;
> +
> BT_DBG("sk %p", sk);
>
> if (l2cap_pi(sk)->chan) {
> @@ -1723,9 +1745,10 @@ static void l2cap_sock_destruct(struct sock *sk)
> l2cap_chan_put(l2cap_pi(sk)->chan);
> }
>
> - if (l2cap_pi(sk)->rx_busy_skb) {
> - kfree_skb(l2cap_pi(sk)->rx_busy_skb);
> - l2cap_pi(sk)->rx_busy_skb = NULL;
> + list_for_each_entry_safe(rx_busy, next, &l2cap_pi(sk)->rx_busy, list) {
> + kfree_skb(rx_busy->skb);
> + list_del(&rx_busy->list);
> + kfree(rx_busy);
> }
>
> skb_queue_purge(&sk->sk_receive_queue);
> @@ -1830,6 +1853,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
> sk->sk_destruct = l2cap_sock_destruct;
> sk->sk_sndtimeo = L2CAP_CONN_TIMEOUT;
>
> + INIT_LIST_HEAD(&l2cap_pi(sk)->rx_busy);
> +
> chan = l2cap_chan_create();
> if (!chan) {
> sk_free(sk);
> --
> 2.34.1
>

--
Luiz Augusto von Dentz

2024-04-07 18:57:35

by Sebastian Urban

[permalink] [raw]

Subject: Re: [PATCH v2] Bluetooth: keep LE flow credits when recvbuf full

Hi Luiz,

On 4/5/24 17:30, Luiz Augusto von Dentz wrote:
> Hi Sebastian,
>
> On Fri, Apr 5, 2024 at 6:26 AM Sebastian Urban <[email protected]> wrote:
>>
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -682,10 +682,15 @@ struct l2cap_user {
>> /* ----- L2CAP socket info ----- */
>> #define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)
>>
>> +struct l2cap_rx_busy {
>> + struct list_head list;
>> + struct sk_buff *skb;
>> +};
>
> In theory we only really to queue 1 skb at most, since we would stop
> giving credits, or perhaps this is because we had given enough credits
> for MTU + 1, so the +1 segment could result in a second SDU/skb to be
> completed while waiting the user space process to start reading again?

Yes, during testing it became apparent that there might be a second
incoming skb, which also needs to be buffered.

Even if --as discussed below-- we change send_credits to return credits
based on the actual available receive buffer space, I believe we still
need to allow buffering more than one skb. This is because local
user-space might decide to resize the receive buffer size (SO_RCVBUF) to
a smaller value after the credits have already been given to the remote
side.

>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index ab5a9d42fae7..c78af7fad255 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -63,6 +63,8 @@ static void l2cap_retrans_timeout(struct work_struct *work);
>> static void l2cap_monitor_timeout(struct work_struct *work);
>> static void l2cap_ack_timeout(struct work_struct *work);
>>
>> +static void l2cap_chan_le_send_credits(struct l2cap_chan *chan);
>
> We probably need to change the way send_credits calculates the number
> of credits to be restored, it needs to consider the actual available
> buffer size at the socket rather then assuming we always shall have
> space for MTU + 1, that way the remote side would always have the
> exact information of how much buffer space is left. That said perhaps
> we need a way to inform when user space reads then we need to call
> into send_credits again.

Yes, this makes sense. I will extend the patch appropriately.

>> @@ -1187,11 +1189,15 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>> if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
>> goto done;
>>
>> - if (pi->rx_busy_skb) {
>> - if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
>> - pi->rx_busy_skb = NULL;
>> - else
>> + while (!list_empty(&pi->rx_busy)) {
>> + struct l2cap_rx_busy *rx_busy =
>> + list_first_entry(&pi->rx_busy,
>> + struct l2cap_rx_busy,
>> + list);
>> + if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
>> goto done;
>> + list_del(&rx_busy->list);
>> + kfree(rx_busy);
>
> I see now, this is trying to dequeue packets if the socket is read,
> which in case we turn the send_credits function to calculate the
> credits based on the socket buffer size that would not be necessary
> but then we would need to call into send_credits here.

This is followed by (unmodified):

if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
l2cap_chan_busy(pi->chan, 0);

And will in fact call send_credits through l2cap_chan_busy from here
once all queued skbs have been accepted by the socket and its receive
buffer has become half empty.

2024-04-08 00:56:58

by Sebastian Urban

[permalink] [raw]

Subject: [PATCH v3] Bluetooth: compute LE flow credits based on recvbuf space

Previously LE flow credits were returned to the
sender even if the socket's receive buffer was
full. This meant that no back-pressure
was applied to the sender, thus it continued to
send data, resulting in data loss without any
error being reported. Furthermore, the amount
of credits was essentially fixed to a small amount,
leading to reduced performance.

This is fixed by computing the number of returned
LE flow credits based on the available space in the
receive buffer of an L2CAP socket. Consequently,
if the receive buffer is full, no credits are returned
until the buffer is read and thus cleared by user-space.

Since the computation of available
receive buffer space can only be performed
approximately, i.e. sk_buff overhead is ignored,
and the receive buffer size may be changed by
user-space after flow credits have been sent,
superfluous received data is temporary stored within
l2cap_pinfo. This is necessary because Bluetooth LE
provides no retransmission mechanism once the
data has been acked by the physical layer.

Signed-off-by: Sebastian Urban <[email protected]>
---
include/net/bluetooth/l2cap.h | 10 +++++-
net/bluetooth/l2cap_core.c | 50 ++++++++++++++++++++++----
net/bluetooth/l2cap_sock.c | 67 +++++++++++++++++++++++++----------
3 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92d7197f9a56..8368463ada72 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -547,6 +547,8 @@ struct l2cap_chan {

__u16 tx_credits;
__u16 rx_credits;
+ int rx_avail;
+ int rx_staged;

__u8 tx_state;
__u8 rx_state;
@@ -682,10 +684,15 @@ struct l2cap_user {
/* ----- L2CAP socket info ----- */
#define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)

+struct l2cap_rx_busy {
+ struct list_head list;
+ struct sk_buff *skb;
+};
+
struct l2cap_pinfo {
struct bt_sock bt;
struct l2cap_chan *chan;
- struct sk_buff *rx_busy_skb;
+ struct list_head rx_busy;
};

enum {
@@ -943,6 +950,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
int l2cap_chan_reconfigure(struct l2cap_chan *chan, __u16 mtu);
int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
+void l2cap_chan_rx_avail(struct l2cap_chan *chan, int rx_avail);
int l2cap_chan_check_security(struct l2cap_chan *chan, bool initiator);
void l2cap_chan_set_defaults(struct l2cap_chan *chan);
int l2cap_ertm_init(struct l2cap_chan *chan);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ab5a9d42fae7..1b5f5c3c2b41 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -454,6 +454,9 @@ struct l2cap_chan *l2cap_chan_create(void)
/* Set default lock nesting level */
atomic_set(&chan->nesting, L2CAP_NESTING_NORMAL);

+ /* Available receive buffer space is initially unknown */
+ chan->rx_avail = -1;
+
write_lock(&chan_list_lock);
list_add(&chan->global_l, &chan_list);
write_unlock(&chan_list_lock);
@@ -535,6 +538,26 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan)
}
EXPORT_SYMBOL_GPL(l2cap_chan_set_defaults);

+static __u16 l2cap_le_rx_credits(struct l2cap_chan *chan)
+{
+ if (chan->mps == 0)
+ return 0;
+
+ /* If we don't know the available space in the receiver buffer, give
+ * enough credits for a full packet.
+ */
+ if (chan->rx_avail == -1)
+ return (chan->imtu / chan->mps) + 1;
+
+ /* If we know how much space is available in the receive buffer, give
+ * out as many credits as would fill the buffer.
+ */
+ if (chan->rx_avail <= chan->rx_staged)
+ return 0;
+ return min_t(int, U16_MAX,
+ (chan->rx_avail - chan->rx_staged) / chan->mps);
+}
+
static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits)
{
chan->sdu = NULL;
@@ -543,8 +566,7 @@ static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits)
chan->tx_credits = tx_credits;
/* Derive MPS from connection MTU to stop HCI fragmentation */
chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE);
- /* Give enough credits for a full packet */
- chan->rx_credits = (chan->imtu / chan->mps) + 1;
+ chan->rx_credits = l2cap_le_rx_credits(chan);

skb_queue_head_init(&chan->tx_q);
}
@@ -556,7 +578,7 @@ static void l2cap_ecred_init(struct l2cap_chan *chan, u16 tx_credits)
/* L2CAP implementations shall support a minimum MPS of 64 octets */
if (chan->mps < L2CAP_ECRED_MIN_MPS) {
chan->mps = L2CAP_ECRED_MIN_MPS;
- chan->rx_credits = (chan->imtu / chan->mps) + 1;
+ chan->rx_credits = l2cap_le_rx_credits(chan);
}
}

@@ -6512,9 +6534,7 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
{
struct l2cap_conn *conn = chan->conn;
struct l2cap_le_credits pkt;
- u16 return_credits;
-
- return_credits = (chan->imtu / chan->mps) + 1;
+ u16 return_credits = l2cap_le_rx_credits(chan);

if (chan->rx_credits >= return_credits)
return;
@@ -6533,6 +6553,15 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
l2cap_send_cmd(conn, chan->ident, L2CAP_LE_CREDITS, sizeof(pkt), &pkt);
}

+void l2cap_chan_rx_avail(struct l2cap_chan *chan, int rx_avail)
+{
+ BT_DBG("chan %p has %d bytes avail for rx", chan, rx_avail);
+
+ chan->rx_avail = rx_avail;
+
+ l2cap_chan_le_send_credits(chan);
+}
+
static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
{
int err;
@@ -6542,6 +6571,14 @@ static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
/* Wait recv to confirm reception before updating the credits */
err = chan->ops->recv(chan, skb);

+ chan->rx_staged = 0;
+
+ if (err < 0 && chan->rx_avail != -1) {
+ BT_ERR("Queueing received LE L2CAP data failed");
+ l2cap_send_disconn_req(chan, ECONNRESET);
+ return err;
+ }
+
/* Update credits whenever an SDU is received */
l2cap_chan_le_send_credits(chan);

@@ -6563,6 +6600,7 @@ static int l2cap_ecred_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
return -ENOBUFS;
}

+ chan->rx_staged += skb->len;
chan->rx_credits--;
BT_DBG("rx_credits %u -> %u", chan->rx_credits + 1, chan->rx_credits);

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ee7a41d6994f..8781f67f9c84 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1146,6 +1146,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
{
struct sock *sk = sock->sk;
struct l2cap_pinfo *pi = l2cap_pi(sk);
+ int avail;
int err;

lock_sock(sk);
@@ -1177,28 +1178,34 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
else
err = bt_sock_recvmsg(sock, msg, len, flags);

- if (pi->chan->mode != L2CAP_MODE_ERTM)
+ if (pi->chan->mode != L2CAP_MODE_ERTM &&
+ pi->chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ pi->chan->mode != L2CAP_MODE_EXT_FLOWCTL)
return err;

- /* Attempt to put pending rx data in the socket buffer */
-
lock_sock(sk);

- if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
- goto done;
+ avail = max(0, sk->sk_rcvbuf - atomic_read(&sk->sk_rmem_alloc));
+ l2cap_chan_rx_avail(pi->chan, avail);

- if (pi->rx_busy_skb) {
- if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
- pi->rx_busy_skb = NULL;
- else
+ /* Attempt to put pending rx data in the socket buffer */
+ while (!list_empty(&pi->rx_busy)) {
+ struct l2cap_rx_busy *rx_busy =
+ list_first_entry(&pi->rx_busy,
+ struct l2cap_rx_busy,
+ list);
+ if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
goto done;
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

/* Restore data flow when half of the receive buffer is
* available. This avoids resending large numbers of
* frames.
*/
- if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
+ if (test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state) &&
+ atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
l2cap_chan_busy(pi->chan, 0);

done:
@@ -1459,17 +1466,21 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
{
struct sock *sk = chan->data;
+ struct l2cap_pinfo *pi = l2cap_pi(sk);
+ int avail;
int err;

lock_sock(sk);

- if (l2cap_pi(sk)->rx_busy_skb) {
+ if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
err = -ENOMEM;
goto done;
}

if (chan->mode != L2CAP_MODE_ERTM &&
- chan->mode != L2CAP_MODE_STREAMING) {
+ chan->mode != L2CAP_MODE_STREAMING &&
+ chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ chan->mode != L2CAP_MODE_EXT_FLOWCTL) {
/* Even if no filter is attached, we could potentially
* get errors from security modules, etc.
*/
@@ -1480,7 +1491,10 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)

err = __sock_queue_rcv_skb(sk, skb);

- /* For ERTM, handle one skb that doesn't fit into the recv
+ avail = max(0, sk->sk_rcvbuf - atomic_read(&sk->sk_rmem_alloc));
+ l2cap_chan_rx_avail(chan, avail);
+
+ /* For ERTM and LE, handle a skb that doesn't fit into the recv
* buffer. This is important to do because the data frames
* have already been acked, so the skb cannot be discarded.
*
@@ -1489,8 +1503,18 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
* acked and reassembled until there is buffer space
* available.
*/
- if (err < 0 && chan->mode == L2CAP_MODE_ERTM) {
- l2cap_pi(sk)->rx_busy_skb = skb;
+ if (err < 0 &&
+ (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_LE_FLOWCTL ||
+ chan->mode == L2CAP_MODE_EXT_FLOWCTL)) {
+ struct l2cap_rx_busy *rx_busy =
+ kmalloc(sizeof(*rx_busy), GFP_KERNEL);
+ if (!rx_busy) {
+ err = -ENOMEM;
+ goto done;
+ }
+ rx_busy->skb = skb;
+ list_add_tail(&rx_busy->list, &pi->rx_busy);
l2cap_chan_busy(chan, 1);
err = 0;
}
@@ -1716,6 +1740,8 @@ static const struct l2cap_ops l2cap_chan_ops = {

static void l2cap_sock_destruct(struct sock *sk)
{
+ struct l2cap_rx_busy *rx_busy, *next;
+
BT_DBG("sk %p", sk);

if (l2cap_pi(sk)->chan) {
@@ -1723,9 +1749,10 @@ static void l2cap_sock_destruct(struct sock *sk)
l2cap_chan_put(l2cap_pi(sk)->chan);
}

- if (l2cap_pi(sk)->rx_busy_skb) {
- kfree_skb(l2cap_pi(sk)->rx_busy_skb);
- l2cap_pi(sk)->rx_busy_skb = NULL;
+ list_for_each_entry_safe(rx_busy, next, &l2cap_pi(sk)->rx_busy, list) {
+ kfree_skb(rx_busy->skb);
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

skb_queue_purge(&sk->sk_receive_queue);
@@ -1830,6 +1857,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
sk->sk_destruct = l2cap_sock_destruct;
sk->sk_sndtimeo = L2CAP_CONN_TIMEOUT;

+ INIT_LIST_HEAD(&l2cap_pi(sk)->rx_busy);
+
chan = l2cap_chan_create();
if (!chan) {
sk_free(sk);
@@ -1838,6 +1867,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,

l2cap_chan_hold(chan);

+ l2cap_chan_rx_avail(chan, sk->sk_rcvbuf);
+
l2cap_pi(sk)->chan = chan;

return sk;
--
2.34.1

2024-04-08 01:48:08

by bluez.test.bot

[permalink] [raw]

Subject: RE: [v3] Bluetooth: compute LE flow credits based on recvbuf space

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: include/net/bluetooth/l2cap.h:943
error: include/net/bluetooth/l2cap.h: patch does not apply
error: patch failed: net/bluetooth/l2cap_sock.c:1146
error: net/bluetooth/l2cap_sock.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.

---
Regards,
Linux Bluetooth

2024-04-08 09:42:24

by Sebastian Urban

[permalink] [raw]

Subject: [PATCH v4] Bluetooth: compute LE flow credits based on recvbuf space

Previously LE flow credits were returned to the
sender even if the socket's receive buffer was
full. This meant that no back-pressure
was applied to the sender, thus it continued to
send data, resulting in data loss without any
error being reported. Furthermore, the amount
of credits was essentially fixed to a small amount,
leading to reduced performance.

This is fixed by computing the number of returned
LE flow credits based on the available space in the
receive buffer of an L2CAP socket. Consequently,
if the receive buffer is full, no credits are returned
until the buffer is read and thus cleared by user-space.

Since the computation of available
receive buffer space can only be performed
approximately, i.e. sk_buff overhead is ignored,
and the receive buffer size may be changed by
user-space after flow credits have been sent,
superfluous received data is temporary stored within
l2cap_pinfo. This is necessary because Bluetooth LE
provides no retransmission mechanism once the
data has been acked by the physical layer.

Signed-off-by: Sebastian Urban <[email protected]>
---
include/net/bluetooth/l2cap.h | 10 +++++-
net/bluetooth/l2cap_core.c | 50 ++++++++++++++++++++++----
net/bluetooth/l2cap_sock.c | 67 +++++++++++++++++++++++++----------
3 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 3f4057ced971..bc6ff40ebc9f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -547,6 +547,8 @@ struct l2cap_chan {

__u16 tx_credits;
__u16 rx_credits;
+ int rx_avail;
+ int rx_staged;

__u8 tx_state;
__u8 rx_state;
@@ -682,10 +684,15 @@ struct l2cap_user {
/* ----- L2CAP socket info ----- */
#define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)

+struct l2cap_rx_busy {
+ struct list_head list;
+ struct sk_buff *skb;
+};
+
struct l2cap_pinfo {
struct bt_sock bt;
struct l2cap_chan *chan;
- struct sk_buff *rx_busy_skb;
+ struct list_head rx_busy;
};

enum {
@@ -944,6 +951,7 @@ int l2cap_chan_reconfigure(struct l2cap_chan *chan, __u16 mtu);
int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
const struct sockcm_cookie *sockc);
void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
+void l2cap_chan_rx_avail(struct l2cap_chan *chan, int rx_avail);
int l2cap_chan_check_security(struct l2cap_chan *chan, bool initiator);
void l2cap_chan_set_defaults(struct l2cap_chan *chan);
int l2cap_ertm_init(struct l2cap_chan *chan);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b0970462a689..f3fc63992b00 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -454,6 +454,9 @@ struct l2cap_chan *l2cap_chan_create(void)
/* Set default lock nesting level */
atomic_set(&chan->nesting, L2CAP_NESTING_NORMAL);

+ /* Available receive buffer space is initially unknown */
+ chan->rx_avail = -1;
+
write_lock(&chan_list_lock);
list_add(&chan->global_l, &chan_list);
write_unlock(&chan_list_lock);
@@ -535,6 +538,26 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan)
}
EXPORT_SYMBOL_GPL(l2cap_chan_set_defaults);

+static __u16 l2cap_le_rx_credits(struct l2cap_chan *chan)
+{
+ if (chan->mps == 0)
+ return 0;
+
+ /* If we don't know the available space in the receiver buffer, give
+ * enough credits for a full packet.
+ */
+ if (chan->rx_avail == -1)
+ return (chan->imtu / chan->mps) + 1;
+
+ /* If we know how much space is available in the receive buffer, give
+ * out as many credits as would fill the buffer.
+ */
+ if (chan->rx_avail <= chan->rx_staged)
+ return 0;
+ return min_t(int, U16_MAX,
+ (chan->rx_avail - chan->rx_staged) / chan->mps);
+}
+
static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits)
{
chan->sdu = NULL;
@@ -543,8 +566,7 @@ static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits)
chan->tx_credits = tx_credits;
/* Derive MPS from connection MTU to stop HCI fragmentation */
chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE);
- /* Give enough credits for a full packet */
- chan->rx_credits = (chan->imtu / chan->mps) + 1;
+ chan->rx_credits = l2cap_le_rx_credits(chan);

skb_queue_head_init(&chan->tx_q);
}
@@ -556,7 +578,7 @@ static void l2cap_ecred_init(struct l2cap_chan *chan, u16 tx_credits)
/* L2CAP implementations shall support a minimum MPS of 64 octets */
if (chan->mps < L2CAP_ECRED_MIN_MPS) {
chan->mps = L2CAP_ECRED_MIN_MPS;
- chan->rx_credits = (chan->imtu / chan->mps) + 1;
+ chan->rx_credits = l2cap_le_rx_credits(chan);
}
}

@@ -6520,9 +6542,7 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
{
struct l2cap_conn *conn = chan->conn;
struct l2cap_le_credits pkt;
- u16 return_credits;
-
- return_credits = (chan->imtu / chan->mps) + 1;
+ u16 return_credits = l2cap_le_rx_credits(chan);

if (chan->rx_credits >= return_credits)
return;
@@ -6541,6 +6561,15 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
l2cap_send_cmd(conn, chan->ident, L2CAP_LE_CREDITS, sizeof(pkt), &pkt);
}

+void l2cap_chan_rx_avail(struct l2cap_chan *chan, int rx_avail)
+{
+ BT_DBG("chan %p has %d bytes avail for rx", chan, rx_avail);
+
+ chan->rx_avail = rx_avail;
+
+ l2cap_chan_le_send_credits(chan);
+}
+
static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
{
int err;
@@ -6550,6 +6579,14 @@ static int l2cap_ecred_recv(struct l2cap_chan *chan, struct sk_buff *skb)
/* Wait recv to confirm reception before updating the credits */
err = chan->ops->recv(chan, skb);

+ chan->rx_staged = 0;
+
+ if (err < 0 && chan->rx_avail != -1) {
+ BT_ERR("Queueing received LE L2CAP data failed");
+ l2cap_send_disconn_req(chan, ECONNRESET);
+ return err;
+ }
+
/* Update credits whenever an SDU is received */
l2cap_chan_le_send_credits(chan);

@@ -6571,6 +6608,7 @@ static int l2cap_ecred_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
return -ENOBUFS;
}

+ chan->rx_staged += skb->len;
chan->rx_credits--;
BT_DBG("rx_credits %u -> %u", chan->rx_credits + 1, chan->rx_credits);

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 7846a068bf60..46603605cb69 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1157,6 +1157,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
{
struct sock *sk = sock->sk;
struct l2cap_pinfo *pi = l2cap_pi(sk);
+ int avail;
int err;

if (unlikely(flags & MSG_ERRQUEUE))
@@ -1192,28 +1193,34 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
else
err = bt_sock_recvmsg(sock, msg, len, flags);

- if (pi->chan->mode != L2CAP_MODE_ERTM)
+ if (pi->chan->mode != L2CAP_MODE_ERTM &&
+ pi->chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ pi->chan->mode != L2CAP_MODE_EXT_FLOWCTL)
return err;

- /* Attempt to put pending rx data in the socket buffer */
-
lock_sock(sk);

- if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
- goto done;
+ avail = max(0, sk->sk_rcvbuf - atomic_read(&sk->sk_rmem_alloc));
+ l2cap_chan_rx_avail(pi->chan, avail);

- if (pi->rx_busy_skb) {
- if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
- pi->rx_busy_skb = NULL;
- else
+ /* Attempt to put pending rx data in the socket buffer */
+ while (!list_empty(&pi->rx_busy)) {
+ struct l2cap_rx_busy *rx_busy =
+ list_first_entry(&pi->rx_busy,
+ struct l2cap_rx_busy,
+ list);
+ if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
goto done;
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

/* Restore data flow when half of the receive buffer is
* available. This avoids resending large numbers of
* frames.
*/
- if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
+ if (test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state) &&
+ atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
l2cap_chan_busy(pi->chan, 0);

done:
@@ -1474,17 +1481,21 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
{
struct sock *sk = chan->data;
+ struct l2cap_pinfo *pi = l2cap_pi(sk);
+ int avail;
int err;

lock_sock(sk);

- if (l2cap_pi(sk)->rx_busy_skb) {
+ if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
err = -ENOMEM;
goto done;
}

if (chan->mode != L2CAP_MODE_ERTM &&
- chan->mode != L2CAP_MODE_STREAMING) {
+ chan->mode != L2CAP_MODE_STREAMING &&
+ chan->mode != L2CAP_MODE_LE_FLOWCTL &&
+ chan->mode != L2CAP_MODE_EXT_FLOWCTL) {
/* Even if no filter is attached, we could potentially
* get errors from security modules, etc.
*/
@@ -1495,7 +1506,10 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)

err = __sock_queue_rcv_skb(sk, skb);

- /* For ERTM, handle one skb that doesn't fit into the recv
+ avail = max(0, sk->sk_rcvbuf - atomic_read(&sk->sk_rmem_alloc));
+ l2cap_chan_rx_avail(chan, avail);
+
+ /* For ERTM and LE, handle a skb that doesn't fit into the recv
* buffer. This is important to do because the data frames
* have already been acked, so the skb cannot be discarded.
*
@@ -1504,8 +1518,18 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
* acked and reassembled until there is buffer space
* available.
*/
- if (err < 0 && chan->mode == L2CAP_MODE_ERTM) {
- l2cap_pi(sk)->rx_busy_skb = skb;
+ if (err < 0 &&
+ (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_LE_FLOWCTL ||
+ chan->mode == L2CAP_MODE_EXT_FLOWCTL)) {
+ struct l2cap_rx_busy *rx_busy =
+ kmalloc(sizeof(*rx_busy), GFP_KERNEL);
+ if (!rx_busy) {
+ err = -ENOMEM;
+ goto done;
+ }
+ rx_busy->skb = skb;
+ list_add_tail(&rx_busy->list, &pi->rx_busy);
l2cap_chan_busy(chan, 1);
err = 0;
}
@@ -1731,6 +1755,8 @@ static const struct l2cap_ops l2cap_chan_ops = {

static void l2cap_sock_destruct(struct sock *sk)
{
+ struct l2cap_rx_busy *rx_busy, *next;
+
BT_DBG("sk %p", sk);

if (l2cap_pi(sk)->chan) {
@@ -1738,9 +1764,10 @@ static void l2cap_sock_destruct(struct sock *sk)
l2cap_chan_put(l2cap_pi(sk)->chan);
}

- if (l2cap_pi(sk)->rx_busy_skb) {
- kfree_skb(l2cap_pi(sk)->rx_busy_skb);
- l2cap_pi(sk)->rx_busy_skb = NULL;
+ list_for_each_entry_safe(rx_busy, next, &l2cap_pi(sk)->rx_busy, list) {
+ kfree_skb(rx_busy->skb);
+ list_del(&rx_busy->list);
+ kfree(rx_busy);
}

skb_queue_purge(&sk->sk_receive_queue);
@@ -1845,6 +1872,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
sk->sk_destruct = l2cap_sock_destruct;
sk->sk_sndtimeo = L2CAP_CONN_TIMEOUT;

+ INIT_LIST_HEAD(&l2cap_pi(sk)->rx_busy);
+
chan = l2cap_chan_create();
if (!chan) {
sk_free(sk);
@@ -1853,6 +1882,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,

l2cap_chan_hold(chan);

+ l2cap_chan_rx_avail(chan, sk->sk_rcvbuf);
+
l2cap_pi(sk)->chan = chan;

return sk;
--
2.34.1

2024-04-08 10:36:41

by bluez.test.bot

[permalink] [raw]

Subject: RE: [v4] Bluetooth: compute LE flow credits based on recvbuf space

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=842388

---Test result---

Test Summary:
CheckPatch PASS 0.96 seconds
GitLint PASS 0.22 seconds
SubjectPrefix PASS 0.07 seconds
BuildKernel PASS 30.01 seconds
CheckAllWarning PASS 32.77 seconds
CheckSparse PASS 38.02 seconds
CheckSmatch FAIL 35.04 seconds
BuildKernel32 PASS 29.01 seconds
TestRunnerSetup PASS 521.51 seconds
TestRunner_l2cap-tester FAIL 20.61 seconds
TestRunner_iso-tester PASS 127.88 seconds
TestRunner_bnep-tester PASS 4.73 seconds
TestRunner_mgmt-tester PASS 110.45 seconds
TestRunner_rfcomm-tester PASS 7.26 seconds
TestRunner_sco-tester PASS 15.03 seconds
TestRunner_ioctl-tester PASS 7.69 seconds
TestRunner_mesh-tester PASS 5.84 seconds
TestRunner_smp-tester PASS 6.78 seconds
TestRunner_userchan-tester PASS 4.91 seconds
IncrementalBuild PASS 28.05 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Total: 55, Passed: 51 (92.7%), Failed: 4, Not Run: 0

Failed Test Cases
L2CAP LE Server - Success Failed 0.093 seconds
L2CAP Ext-Flowctl Server - Success Failed 0.102 seconds
L2CAP LE EATT Server - Success Failed 0.111 seconds
L2CAP LE EATT Server - Reject Failed 0.090 seconds

---
Regards,
Linux Bluetooth