Currently the skb size after coalescing is only limited by the skb
layout (the skb must not carry frag_list). A single coalesced skb
covering several MSS can potentially fill completely the receive
buffer. In such a case, the snd win will zero until the receive buffer
will be empty again, affecting tput badly.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
No fixes tag because the problem is not very visible in practice
currently, but will be apparent after the rx refactor.
Still I hope this could affect positively the simlut flows self-tests
---
 net/mptcp/protocol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f768aa4473fb..fd9593f85a98 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -136,6 +136,7 @@ static bool mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	int delta;
 
 	if (MPTCP_SKB_CB(from)->offset ||
+	    ((to->len + from->len) > (sk->sk_rcvbuf >> 3)) ||
 	    !skb_try_coalesce(to, from, &fragstolen, &delta))
 		return false;
 
-- 
2.45.2

Hi Paolo,

On 06/12/2024 13:10, Paolo Abeni wrote:
> Currently the skb size after coalescing is only limited by the skb
> layout (the skb must not carry frag_list). A single coalesced skb
> covering several MSS can potentially fill completely the receive
> buffer. In such a case, the snd win will zero until the receive buffer
> will be empty again, affecting tput badly.

Good idea! Thank you for having looked at this!

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> No fixes tag because the problem is not very visible in practice
> currently, but will be apparent after the rx refactor.
> Still I hope this could affect positively the simlut flows self-tests

The modification looks safe enough to me: it should not make thing worst.

If it helps, a Fixes tag can be added and next to the 'Cc: stable' tag,
we can ask to backport this patch after a few weeks, no? e.g.

  Cc: <stable@vger.kernel.org> # after -rc6

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

Hi Paolo,

On 10/12/2024 13:03, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 06/12/2024 13:10, Paolo Abeni wrote:
>> Currently the skb size after coalescing is only limited by the skb
>> layout (the skb must not carry frag_list). A single coalesced skb
>> covering several MSS can potentially fill completely the receive
>> buffer. In such a case, the snd win will zero until the receive buffer
>> will be empty again, affecting tput badly.
> 
> Good idea! Thank you for having looked at this!
> 
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> No fixes tag because the problem is not very visible in practice
>> currently, but will be apparent after the rx refactor.
>> Still I hope this could affect positively the simlut flows self-tests
> 
> The modification looks safe enough to me: it should not make thing worst.

Do you still prefer not targetting -net for this patch here? Or is it OK
to do so and add a Fixes tag?

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

On 12/21/24 11:00, Matthieu Baerts wrote:
> On 10/12/2024 13:03, Matthieu Baerts wrote:
>> On 06/12/2024 13:10, Paolo Abeni wrote:
>>> Currently the skb size after coalescing is only limited by the skb
>>> layout (the skb must not carry frag_list). A single coalesced skb
>>> covering several MSS can potentially fill completely the receive
>>> buffer. In such a case, the snd win will zero until the receive buffer
>>> will be empty again, affecting tput badly.
>>
>> Good idea! Thank you for having looked at this!
>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> No fixes tag because the problem is not very visible in practice
>>> currently, but will be apparent after the rx refactor.
>>> Still I hope this could affect positively the simlut flows self-tests
>>
>> The modification looks safe enough to me: it should not make thing worst.
> 
> Do you still prefer not targetting -net for this patch here? Or is it OK
> to do so and add a Fixes tag?

Whatever is easier to do in practice works for me. Perhaps monitoring
the simult_flows ST flakes could give an idea - if that get more
stable/less unstable we should possibly target 'net'.

Cheers,

Paolo

On 27/12/2024 10:40, Paolo Abeni wrote:
> On 12/21/24 11:00, Matthieu Baerts wrote:
>> On 10/12/2024 13:03, Matthieu Baerts wrote:
>>> On 06/12/2024 13:10, Paolo Abeni wrote:
>>>> Currently the skb size after coalescing is only limited by the skb
>>>> layout (the skb must not carry frag_list). A single coalesced skb
>>>> covering several MSS can potentially fill completely the receive
>>>> buffer. In such a case, the snd win will zero until the receive buffer
>>>> will be empty again, affecting tput badly.
>>>
>>> Good idea! Thank you for having looked at this!
>>>
>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>> ---
>>>> No fixes tag because the problem is not very visible in practice
>>>> currently, but will be apparent after the rx refactor.
>>>> Still I hope this could affect positively the simlut flows self-tests
>>>
>>> The modification looks safe enough to me: it should not make thing worst.
>>
>> Do you still prefer not targetting -net for this patch here? Or is it OK
>> to do so and add a Fixes tag?
> 
> Whatever is easier to do in practice works for me. Perhaps monitoring
> the simult_flows ST flakes could give an idea - if that get more
> stable/less unstable we should possibly target 'net'.

Thank you for your reply!

It looks OK so far. I just sent this patch to netdev, so we can also get
some tests over there. I will monitor if it helps on NIPA as well.

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