Re: [PATCH v5] Bluetooth: compute LE flow credits based on recvbuf space

[Sebastian Urban <surban@surban.net>]

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.