Messages in this thread | | | From | Sebastian Urban <> | Subject | Re: [PATCH v5] Bluetooth: compute LE flow credits based on recvbuf space | Date | Tue, 30 Apr 2024 23:51:16 +0000 |
| |
Hi Luiz,
thanks for the review!
On 4/10/24 16:38, Luiz Augusto von Dentz wrote: >> --- >> include/net/bluetooth/l2cap.h | 10 +++++- >> net/bluetooth/l2cap_core.c | 51 ++++++++++++++++++++++---- >> net/bluetooth/l2cap_sock.c | 67 +++++++++++++++++++++++++---------- >> 3 files changed, 103 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; > > I'd probably use size_t for these above, and put some comments of what > they refer to e.g. rx_staged is what has been received already, right? > Couldn't we use chan->sdu->len instead?
Changed and replaced by chan->sdu->len.
>> @@ -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; > > Missing space.
Done.
> >> + return min_t(int, U16_MAX, >> + (chan->rx_avail - chan->rx_staged) / chan->mps); > > We probably need to use DIV_ROUND_UP since the division can return 0 > or is that intentional since that means we cannot store another full > mps? I think I'd give it a credit since this may impact the throughput > if we are not given credits when just a few bytes would be necessary > and in any case we would store the extra bytes in the rx_busy list if > that is over the rx_avail.
I agree. Changed.
>> @@ -6541,6 +6561,16 @@ 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); > > We should probably check if the value has changed though, or this > shall only be called when the buffer changes?
Function returns now immediately if rx_avail is unchanged.
Additionally I improved the available receive space estimation by taking the overhead of struct sk_buff into account.
I will submit a new version of the patch soon.
| |