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 <surban@xxxxxxxxxx> 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.
