[PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage

Matthieu Baerts (NGI0) posted 9 patches 3 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20240718-mptcp-backup-mpj-v3-0-1f6cd9b89ee4@kernel.org
include/trace/events/mptcp.h                       |  2 +-
net/mptcp/mib.c                                    |  2 +
net/mptcp/mib.h                                    |  2 +
net/mptcp/options.c                                |  2 +-
net/mptcp/pm.c                                     | 12 ++++
net/mptcp/pm_netlink.c                             | 19 +++++-
net/mptcp/pm_userspace.c                           | 18 ++++++
net/mptcp/protocol.c                               | 10 +--
net/mptcp/protocol.h                               |  4 ++
net/mptcp/subflow.c                                | 10 +++
tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c |  3 +-
.../testing/selftests/bpf/progs/mptcp_bpf_burst.c  | 12 ++--
tools/testing/selftests/net/mptcp/mptcp_join.sh    | 72 +++++++++++++++++-----
13 files changed, 142 insertions(+), 26 deletions(-)
[PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by Matthieu Baerts (NGI0) 3 months, 2 weeks ago
In all the backup tests we have, the backup flag is set on one side, and
the expected behaviour is to have both sides respecting this decision.

On the scheduler side, only the 'backup' field is checked, which is
supposed to be set only if the other peer flagged a subflow as backup.
But in various places, this flag is set when the local host flagged the
subflow as backup.

One last thing that has been fixed is the backup flag in 'signal'
endpoints: it was working only when setting the backup flag while the
connection was already opened. That's what the MPJ selftests were doing
to check that the signal endpoint were supporting the backup flag (on
purpose I guess, because the support was broken in MP_JOIN...).

The last two patches adapt the BPF selftests to mimic what is now done
by the in-kernel packets scheduler.

Note that the Packetdrill tests need to be updated because the behaviour
has been changed / fixed:

  https://github.com/multipath-tcp/packetdrill/pull/152

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v3:
- Small fixes in patch 1 and 5: see the individual ChangeLog
- Use a new dedicated helper in patch 6: see the individual ChangeLog
- Link to v2: https://lore.kernel.org/r/20240716-mptcp-backup-mpj-v2-0-4d50247405fb@kernel.org

Changes in v2:
- new patches 4 and 5.
- split patch 6 in two (code then tests) to ease the backports.
- patch 6 addresses Mat's comments, and support the ID0 case.
- Link to v1: https://lore.kernel.org/r/20240711-mptcp-backup-mpj-v1-0-d45506182a9e@kernel.org

---
Matthieu Baerts (NGI0) (9):
      mptcp: sched: check both directions for backup
      mptcp: distinguish rcv vs sent backup flag in requests
      mptcp: pm: only set request_bkup flag when sending MP_PRIO
      mptcp: mib: count MPJ with backup flag
      selftests: mptcp: join: validate backup in MPJ
      mptcp: pm: fix backup support in signal endpoints
      selftests: mptcp: join: check backup support in signal endp
      Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
      Squash to "selftests/bpf: Add bpf_burst scheduler & test"

 include/trace/events/mptcp.h                       |  2 +-
 net/mptcp/mib.c                                    |  2 +
 net/mptcp/mib.h                                    |  2 +
 net/mptcp/options.c                                |  2 +-
 net/mptcp/pm.c                                     | 12 ++++
 net/mptcp/pm_netlink.c                             | 19 +++++-
 net/mptcp/pm_userspace.c                           | 18 ++++++
 net/mptcp/protocol.c                               | 10 +--
 net/mptcp/protocol.h                               |  4 ++
 net/mptcp/subflow.c                                | 10 +++
 tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c |  3 +-
 .../testing/selftests/bpf/progs/mptcp_bpf_burst.c  | 12 ++--
 tools/testing/selftests/net/mptcp/mptcp_join.sh    | 72 +++++++++++++++++-----
 13 files changed, 142 insertions(+), 26 deletions(-)
---
base-commit: 50d1a79f5003b88b03b37d05189ad02a84d7be49
change-id: 20240711-mptcp-backup-mpj-2f8369fad366

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>
Re: [PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by Mat Martineau 3 months, 1 week ago
On Thu, 18 Jul 2024, Matthieu Baerts (NGI0) wrote:

> In all the backup tests we have, the backup flag is set on one side, and
> the expected behaviour is to have both sides respecting this decision.
>
> On the scheduler side, only the 'backup' field is checked, which is
> supposed to be set only if the other peer flagged a subflow as backup.
> But in various places, this flag is set when the local host flagged the
> subflow as backup.
>
> One last thing that has been fixed is the backup flag in 'signal'
> endpoints: it was working only when setting the backup flag while the
> connection was already opened. That's what the MPJ selftests were doing
> to check that the signal endpoint were supporting the backup flag (on
> purpose I guess, because the support was broken in MP_JOIN...).
>
> The last two patches adapt the BPF selftests to mimic what is now done
> by the in-kernel packets scheduler.
>
> Note that the Packetdrill tests need to be updated because the behaviour
> has been changed / fixed:
>
>  https://github.com/multipath-tcp/packetdrill/pull/152
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Changes in v3:
> - Small fixes in patch 1 and 5: see the individual ChangeLog
> - Use a new dedicated helper in patch 6: see the individual ChangeLog
> - Link to v2: https://lore.kernel.org/r/20240716-mptcp-backup-mpj-v2-0-4d50247405fb@kernel.org
>

Hi Matthieu -

v3 LGTM. I'm not sure if the new MIBs are best targeted for net-next, they 
do help with testing but don't seem critical for -stable.

Reviewed-by: Mat Martineau <martineau@kernel.org>


> Changes in v2:
> - new patches 4 and 5.
> - split patch 6 in two (code then tests) to ease the backports.
> - patch 6 addresses Mat's comments, and support the ID0 case.
> - Link to v1: https://lore.kernel.org/r/20240711-mptcp-backup-mpj-v1-0-d45506182a9e@kernel.org
>
> ---
> Matthieu Baerts (NGI0) (9):
>      mptcp: sched: check both directions for backup
>      mptcp: distinguish rcv vs sent backup flag in requests
>      mptcp: pm: only set request_bkup flag when sending MP_PRIO
>      mptcp: mib: count MPJ with backup flag
>      selftests: mptcp: join: validate backup in MPJ
>      mptcp: pm: fix backup support in signal endpoints
>      selftests: mptcp: join: check backup support in signal endp
>      Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
>      Squash to "selftests/bpf: Add bpf_burst scheduler & test"
>
> include/trace/events/mptcp.h                       |  2 +-
> net/mptcp/mib.c                                    |  2 +
> net/mptcp/mib.h                                    |  2 +
> net/mptcp/options.c                                |  2 +-
> net/mptcp/pm.c                                     | 12 ++++
> net/mptcp/pm_netlink.c                             | 19 +++++-
> net/mptcp/pm_userspace.c                           | 18 ++++++
> net/mptcp/protocol.c                               | 10 +--
> net/mptcp/protocol.h                               |  4 ++
> net/mptcp/subflow.c                                | 10 +++
> tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c |  3 +-
> .../testing/selftests/bpf/progs/mptcp_bpf_burst.c  | 12 ++--
> tools/testing/selftests/net/mptcp/mptcp_join.sh    | 72 +++++++++++++++++-----
> 13 files changed, 142 insertions(+), 26 deletions(-)
> ---
> base-commit: 50d1a79f5003b88b03b37d05189ad02a84d7be49
> change-id: 20240711-mptcp-backup-mpj-2f8369fad366
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>
Re: [PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by Matthieu Baerts 3 months, 1 week ago
Hi Mat,

On 23/07/2024 02:24, Mat Martineau wrote:
> On Thu, 18 Jul 2024, Matthieu Baerts (NGI0) wrote:
> 
>> In all the backup tests we have, the backup flag is set on one side, and
>> the expected behaviour is to have both sides respecting this decision.
>>
>> On the scheduler side, only the 'backup' field is checked, which is
>> supposed to be set only if the other peer flagged a subflow as backup.
>> But in various places, this flag is set when the local host flagged the
>> subflow as backup.
>>
>> One last thing that has been fixed is the backup flag in 'signal'
>> endpoints: it was working only when setting the backup flag while the
>> connection was already opened. That's what the MPJ selftests were doing
>> to check that the signal endpoint were supporting the backup flag (on
>> purpose I guess, because the support was broken in MP_JOIN...).
>>
>> The last two patches adapt the BPF selftests to mimic what is now done
>> by the in-kernel packets scheduler.
>>
>> Note that the Packetdrill tests need to be updated because the behaviour
>> has been changed / fixed:
>>
>>  https://github.com/multipath-tcp/packetdrill/pull/152
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Changes in v3:
>> - Small fixes in patch 1 and 5: see the individual ChangeLog
>> - Use a new dedicated helper in patch 6: see the individual ChangeLog
>> - Link to v2: https://lore.kernel.org/r/20240716-mptcp-backup-mpj-
>> v2-0-4d50247405fb@kernel.org
>>
> 
> Hi Matthieu -
> 
> v3 LGTM. I'm not sure if the new MIBs are best targeted for net-next,
> they do help with testing but don't seem critical for -stable.
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

Thank you for the review!

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 9d426c00aef4: mptcp: sched: check both directions for backup
- cba82af312b3: mptcp: distinguish rcv vs sent backup flag in requests
- 1ed597db4a47: mptcp: pm: only set request_bkup flag when sending MP_PRIO
- 887ecb0faf1d: mptcp: mib: count MPJ with backup flag
- 5e248a08f195: selftests: mptcp: join: validate backup in MPJ
- 237a12154c8f: mptcp: pm: fix backup support in signal endpoints
- 34b6d5cac1f0: selftests: mptcp: join: check backup support in signal endp
- Results: 159097cdb0e8..001fe004e8be (export-net)
- Results: 140ff27ee472..939ca15e71e2 (export)

New patches for t/upstream:
- b4a459e0fd62: "squashed" patch 8/9 in "selftests/bpf: Add bpf_bkup
scheduler & test"
- ec4acb003f8a: "squashed" patch 9/9 in "selftests/bpf: Add bpf_burst
scheduler & test"
- Results: 939ca15e71e2..f0f02c71ff0d (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/293546bf79b70ea127af87026d99cfb91eacb77b/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/b601d6da52375e9a0a078ec38af104b5243c4658/checks

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

Re: [PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by Matthieu Baerts 3 months, 1 week ago
Hi Mat,

On 23/07/2024 02:24, Mat Martineau wrote:
> On Thu, 18 Jul 2024, Matthieu Baerts (NGI0) wrote:
> 
>> In all the backup tests we have, the backup flag is set on one side, and
>> the expected behaviour is to have both sides respecting this decision.
>>
>> On the scheduler side, only the 'backup' field is checked, which is
>> supposed to be set only if the other peer flagged a subflow as backup.
>> But in various places, this flag is set when the local host flagged the
>> subflow as backup.
>>
>> One last thing that has been fixed is the backup flag in 'signal'
>> endpoints: it was working only when setting the backup flag while the
>> connection was already opened. That's what the MPJ selftests were doing
>> to check that the signal endpoint were supporting the backup flag (on
>> purpose I guess, because the support was broken in MP_JOIN...).
>>
>> The last two patches adapt the BPF selftests to mimic what is now done
>> by the in-kernel packets scheduler.
>>
>> Note that the Packetdrill tests need to be updated because the behaviour
>> has been changed / fixed:
>>
>>  https://github.com/multipath-tcp/packetdrill/pull/152
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Changes in v3:
>> - Small fixes in patch 1 and 5: see the individual ChangeLog
>> - Use a new dedicated helper in patch 6: see the individual ChangeLog
>> - Link to v2: https://lore.kernel.org/r/20240716-mptcp-backup-mpj-
>> v2-0-4d50247405fb@kernel.org
>>
> 
> Hi Matthieu -
> 
> v3 LGTM. I'm not sure if the new MIBs are best targeted for net-next,
> they do help with testing but don't seem critical for -stable.

Indeed. My reasoning was that the MIB counters were not there, mainly
because this behaviour has never been verified. At the end, it is only
10 new but well controlled lines :)

I suggest applying them in our tree, and delay them if they got
rejected. At the end, the new MIB counters and the tests are well
isolated, and with a note about that.

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

Re: [PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by MPTCP CI 3 months, 2 weeks ago
Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴
- KVM Validation: debug: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9994916304

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/962a6c85c567
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=872303


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Re: [PATCH mptcp-net v3 0/9] mptcp: fix inconsistent backup usage
Posted by Matthieu Baerts 3 months, 1 week ago
Hello,

On 18/07/2024 18:44, MPTCP CI wrote:
> Our CI did some validations and here is its report:
> 
> - KVM Validation: normal: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴
> - KVM Validation: debug: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴

Just to avoid some confusions, and as mentioned in my cover-letter,
these errors were expected because these tests were looking for the
backup flags in the MPJ+SYN+ACK which were not supposed to be there:

  https://github.com/multipath-tcp/packetdrill/pull/152

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