[PATCH v3 mptcp-next 0/4] mptcp: just another receive path refactor

Paolo Abeni posted 4 patches 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/cover.1659165378.git.pabeni@redhat.com
Maintainers: Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>, "David S. Miller" <davem@davemloft.net>, Paolo Abeni <pabeni@redhat.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, David Ahern <dsahern@kernel.org>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
include/net/mptcp.h  |   2 +
net/ipv4/tcp.c       |   3 +
net/mptcp/options.c  |   1 -
net/mptcp/protocol.c | 219 +++++++++++--------------------------------
net/mptcp/protocol.h |  12 ++-
5 files changed, 68 insertions(+), 169 deletions(-)
[PATCH v3 mptcp-next 0/4] mptcp: just another receive path refactor
Posted by Paolo Abeni 1 year, 8 months ago
This is a refresh and rebase of an already shared work:

https://lore.kernel.org/mptcp/cover.1621963632.git.pabeni@redhat.com/
[1]

The motiviation for refreshing this is:

https://lore.kernel.org/mptcp/YtVhyGSsv1CWvPz4@xsang-OptiPlex-9020/

specifically it looks like that properly attaching mem_cg to the
msk socket would be (much?) easier if the RX path and the fwd memory
allocation would be under msk socket lock protection.

The first 2 patches are proably worthy even without the refactor,
but specifically the 2nd one is required to get a good mptcp-level
acking behavior when we move the rx under the socket lock.

The 3rd patch does the real work, and the 4th is a follow-up cleanup.

Back at [1], I measured relevant performance regressions in some
specific cases. I've done the same test here and I now see little to
noise changes. I guess that is mostly due to the better acking
strategy already introduce with commit 949dfdcf343c ("Merge branch 
'mptcp-improve-mptcp-level-window-tracking'") and refined here.

v2 -> v3:
 - dropped obsoleted comment in patch 2/4
 - fixed compile warning in patch 3/4

v1 -> v2:
 - fix build issue in patch 3/4 due to PEBKAC
 - added missing commit messages(!!!) in patch 3/4 & 4/4

Paolo Abeni (4):
  mptcp: move RCVPRUNE event later
  mptcp: more accurate receive buffer updates
  mptcp: move msk input path under full msk socket lock
  mptcp: use common helper for rmem memory accounting

 include/net/mptcp.h  |   2 +
 net/ipv4/tcp.c       |   3 +
 net/mptcp/options.c  |   1 -
 net/mptcp/protocol.c | 219 +++++++++++--------------------------------
 net/mptcp/protocol.h |  12 ++-
 5 files changed, 68 insertions(+), 169 deletions(-)

-- 
2.35.3


Re: [PATCH v3 mptcp-next 0/4] mptcp: just another receive path refactor
Posted by Matthieu Baerts 1 year, 8 months ago
Hi Paolo, Mat,

On 30/07/2022 10:04, Paolo Abeni wrote:
> This is a refresh and rebase of an already shared work:
> 
> https://lore.kernel.org/mptcp/cover.1621963632.git.pabeni@redhat.com/
> [1]
> 
> The motiviation for refreshing this is:
> 
> https://lore.kernel.org/mptcp/YtVhyGSsv1CWvPz4@xsang-OptiPlex-9020/
> 
> specifically it looks like that properly attaching mem_cg to the
> msk socket would be (much?) easier if the RX path and the fwd memory
> allocation would be under msk socket lock protection.
> 
> The first 2 patches are proably worthy even without the refactor,
> but specifically the 2nd one is required to get a good mptcp-level
> acking behavior when we move the rx under the socket lock.
> 
> The 3rd patch does the real work, and the 4th is a follow-up cleanup.
> 
> Back at [1], I measured relevant performance regressions in some
> specific cases. I've done the same test here and I now see little to
> noise changes. I guess that is mostly due to the better acking
> strategy already introduce with commit 949dfdcf343c ("Merge branch 
> 'mptcp-improve-mptcp-level-window-tracking'") and refined here.
> 
> Paolo Abeni (4):
>   mptcp: move RCVPRUNE event later
>   mptcp: more accurate receive buffer updates
>   mptcp: move msk input path under full msk socket lock
>   mptcp: use common helper for rmem memory accounting

Thank you for the patches, the review and breaking MPTCP selftests again :-D

These patches have just been applied in our tree (feat. for net-next)
with Mat's RvB tags and without a typo (s/acconting/accounting) detected
by codespell.


New patches for t/upstream:
- 6ad19008277f: mptcp: move RCVPRUNE event later
- 02da15069287: mptcp: more accurate receive buffer updates
- 187f1240938a: mptcp: move msk input path under full msk socket lock
- e81d36defc24: mptcp: use common helper for rmem memory accounting
- Results: 7f8f198f2f9b..fc2b3f4211d6 (export)


Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220803T163817


Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH v3 mptcp-next 0/4] mptcp: just another receive path refactor
Posted by Paolo Abeni 1 year, 8 months ago
On Sat, 2022-07-30 at 10:04 +0200, Paolo Abeni wrote:
> This is a refresh and rebase of an already shared work:
> 
> https://lore.kernel.org/mptcp/cover.1621963632.git.pabeni@redhat.com/
> [1]
> 
> The motiviation for refreshing this is:
> 
> https://lore.kernel.org/mptcp/YtVhyGSsv1CWvPz4@xsang-OptiPlex-9020/
> 
> specifically it looks like that properly attaching mem_cg to the
> msk socket would be (much?) easier if the RX path and the fwd memory
> allocation would be under msk socket lock protection.
> 
> The first 2 patches are proably worthy even without the refactor,
> but specifically the 2nd one is required to get a good mptcp-level
> acking behavior when we move the rx under the socket lock.
> 
> The 3rd patch does the real work, and the 4th is a follow-up cleanup.
> 
> Back at [1], I measured relevant performance regressions in some
> specific cases. I've done the same test here and I now see little to
> noise changes. I guess that is mostly due to the better acking
> strategy already introduce with commit 949dfdcf343c ("Merge branch 
> 'mptcp-improve-mptcp-level-window-tracking'") and refined here.
> 
> v2 -> v3:
>  - dropped obsoleted comment in patch 2/4
>  - fixed compile warning in patch 3/4
> 
> v1 -> v2:
>  - fix build issue in patch 3/4 due to PEBKAC
>  - added missing commit messages(!!!) in patch 3/4 & 4/4
> 
> Paolo Abeni (4):
>   mptcp: move RCVPRUNE event later
>   mptcp: more accurate receive buffer updates
>   mptcp: move msk input path under full msk socket lock
>   mptcp: use common helper for rmem memory accounting
> 
>  include/net/mptcp.h  |   2 +
>  net/ipv4/tcp.c       |   3 +
>  net/mptcp/options.c  |   1 -
>  net/mptcp/protocol.c | 219 +++++++++++--------------------------------
>  net/mptcp/protocol.h |  12 ++-
>  5 files changed, 68 insertions(+), 169 deletions(-)

I *think* there is a litte more self-tests instability with series
applied, specifically on simult_flows, and mptcp_join 32 && 99.

Behind the CI report I also observed a few sporadid failures there.

I'm wild guessing that the mpj failures are caused by the subflow
socket lock being acquired/relased more often under the msk socket lock
, possibly changing the timing for some events.

I still have to investigate the above.

I think we could still consider merging this on the export branch, and
later try to hammer the issue on the branch (to get more coverage from
the bots).

WDYT?

Thanks!

Paolo


Re: [PATCH v3 mptcp-next 0/4] mptcp: just another receive path refactor
Posted by Mat Martineau 1 year, 8 months ago
On Mon, 1 Aug 2022, Paolo Abeni wrote:

> On Sat, 2022-07-30 at 10:04 +0200, Paolo Abeni wrote:
>> This is a refresh and rebase of an already shared work:
>>
>> https://lore.kernel.org/mptcp/cover.1621963632.git.pabeni@redhat.com/
>> [1]
>>
>> The motiviation for refreshing this is:
>>
>> https://lore.kernel.org/mptcp/YtVhyGSsv1CWvPz4@xsang-OptiPlex-9020/
>>
>> specifically it looks like that properly attaching mem_cg to the
>> msk socket would be (much?) easier if the RX path and the fwd memory
>> allocation would be under msk socket lock protection.
>>
>> The first 2 patches are proably worthy even without the refactor,
>> but specifically the 2nd one is required to get a good mptcp-level
>> acking behavior when we move the rx under the socket lock.
>>
>> The 3rd patch does the real work, and the 4th is a follow-up cleanup.
>>
>> Back at [1], I measured relevant performance regressions in some
>> specific cases. I've done the same test here and I now see little to
>> noise changes. I guess that is mostly due to the better acking
>> strategy already introduce with commit 949dfdcf343c ("Merge branch
>> 'mptcp-improve-mptcp-level-window-tracking'") and refined here.
>>
>> v2 -> v3:
>>  - dropped obsoleted comment in patch 2/4
>>  - fixed compile warning in patch 3/4
>>
>> v1 -> v2:
>>  - fix build issue in patch 3/4 due to PEBKAC
>>  - added missing commit messages(!!!) in patch 3/4 & 4/4
>>
>> Paolo Abeni (4):
>>   mptcp: move RCVPRUNE event later
>>   mptcp: more accurate receive buffer updates
>>   mptcp: move msk input path under full msk socket lock
>>   mptcp: use common helper for rmem memory accounting
>>
>>  include/net/mptcp.h  |   2 +
>>  net/ipv4/tcp.c       |   3 +
>>  net/mptcp/options.c  |   1 -
>>  net/mptcp/protocol.c | 219 +++++++++++--------------------------------
>>  net/mptcp/protocol.h |  12 ++-
>>  5 files changed, 68 insertions(+), 169 deletions(-)
>
> I *think* there is a litte more self-tests instability with series
> applied, specifically on simult_flows, and mptcp_join 32 && 99.
>
> Behind the CI report I also observed a few sporadid failures there.
>
> I'm wild guessing that the mpj failures are caused by the subflow
> socket lock being acquired/relased more often under the msk socket lock
> , possibly changing the timing for some events.
>
> I still have to investigate the above.
>
> I think we could still consider merging this on the export branch, and
> later try to hammer the issue on the branch (to get more coverage from
> the bots).
>
> WDYT?
>

I think that's a good plan. Tests are running ok on my test VM and the 
code looks ok, and I agree your wild guess is plausible.

It's also a good time to let it bake a bit in the export branch, with 
net-next being newly closed.

For the series:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel