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.