Re: [PATCH v6 mptcp-next 11/11] mptcp: leverage the backlog for RX packet processing

[Matthieu Baerts <matttbe@kernel.org>]

Hi Mat, Paolo,

On 23/10/2025 19:02, Mat Martineau wrote:
> On Thu, 23 Oct 2025, Matthieu Baerts wrote:
> 
>> Hi Paolo, Mat,
>>
>> On 23/10/2025 17:11, Paolo Abeni wrote:
>>>
>>>
>>> On 10/22/25 4:31 PM, Paolo Abeni wrote:
>>>> When the msk socket is owned or the msk receive buffer is full,
>>>> move the incoming skbs in a msk level backlog list. This avoid
>>>> traversing the joined subflows and acquiring the subflow level
>>>> socket lock at reception time, improving the RX performances.
>>>>
>>>> When processing the backlog, use the fwd alloc memory borrowed from
>>>> the incoming subflow. skbs exceeding the msk receive space are
>>>> not dropped; instead they are kept into the backlog until the receive
>>>> buffer is freed. Dropping packets already acked at the TCP level is
>>>> explicitly discouraged by the RFC and would corrupt the data stream
>>>> for fallback sockets.
>>>>
>>>> Move the conditional reschedule in release_cb() to take action only
>>>> after the first loop iteration, to avoid rescheduling just before
>>>> releasing the lock.
>>>>
>>>> Special care is needed to avoid adding skbs to the backlog of a closed
>>>> msk and to avoid leaving dangling references into the backlog
>>>> at subflow closing time.
>>
>> (...)
>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index 5a1d8f9e0fb0ec..0aae17ab77edb2 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>
>> (...)
>>
>>>> -static bool __mptcp_move_skbs(struct sock *sk)
>>>> +static bool mptcp_can_spool_backlog(struct sock *sk, u32 moved,
>>>> +                    struct list_head *skbs)
>>>>  {
>>>> -    struct mptcp_subflow_context *subflow;
>>>>      struct mptcp_sock *msk = mptcp_sk(sk);
>>>> -    bool ret = false;
>>>>
>>>> -    if (list_empty(&msk->conn_list))
>>>> +    if (list_empty(&msk->backlog_list))
>>>>          return false;
>>>>
>>>> -    subflow = list_first_entry(&msk->conn_list,
>>>> -                   struct mptcp_subflow_context, node);
>>>> -    for (;;) {
>>>> -        struct sock *ssk;
>>>> -        bool slowpath;
>>>> +    /* Borrowed mem could be zero only in the unlikely event that
>>>> the bl
>>>> +     * is full
>>>> +     */
>>>> +    if (likely(msk->borrowed_mem)) {
>>>> +        sk_forward_alloc_add(sk, msk->borrowed_mem);
>>>> +        msk->borrowed_mem = 0;
>>>> +        sk->sk_reserved_mem = msk->backlog_len;
>>>
>>> With the above I intended to prevent the fwd memory handling from
>>> releasing backlog_len bytes. Re-reading the relevant code, it does not
>>> allow that (experimentation confirmed), see:
>>>
>>> https://elixir.bootlin.com/linux/v6.18-rc2/source/include/net/
>>> sock.h#L1593
>>>
>>> and:
>>>
>>> https://elixir.bootlin.com/linux/v6.18-rc2/source/include/net/
>>> sock.h#L1580
>>>
>>> This will need some more care. Also patch 2 will require some
>>> significant rework.
>>
>> Thank you for looking at this complex part, and for having spot that!
>>
>>> @Mat, @Matttbe: could you please consider merging patches 1,3-9?
>>>
>>> I think they should be pretty uncontroversial, would make the series
>>> more manegeable for future iterations (and would alleviate my
>>> frustration to make this thing work correctly).
>>
>> It makes sense, fine by me. I will wait for Mat's review before applying
>> them (patch 1 is for 'net' I suppose).
>>
> 
> Applying 1,3-9 to our tree(s) makes sense to me. Maybe patch 5 to -net
> too? (just sent email about that before I saw this message).

Good catch! (I will wait for Paolo's reply before applying the patches.)

> Matthieu do you want me to reply to each so the RvB tag is in patchwork,
> or does this suffice for patches 1 & 3-9:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

No, that's fine, I can do a copy-paste. (Note that you can also send it
as a reply to the cover-letter, then I simply only apply the mentioned
patches.)


Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.