[PATCH mptcp-net] mptcp: sched: check both backup in retrans

[PATCH mptcp-net] mptcp: sched: check both backup in retrans

[Matthieu Baerts (NGI0)]

The 'mptcp_subflow_context' structure has two items related to the
backup flags:

 - 'backup': the subflow has been marked as backup by the other peer

 - 'request_bkup': the backup flag has been set by the host

Looking only at the 'backup' flag can make sense in some cases, but it
is not the behaviour of the default packet scheduler when selecting
paths.

As explained in the commit b6a66e521a20 ("mptcp: sched: check both
directions for backup"), the packet scheduler should look at both flags,
because that was the behaviour from the beginning: the 'backup' flag was
set by accident instead of the 'request_bkup' one. Now that the latter
has been fixed, get_retrans() needs to be adapted as well.

Fixes: 3b1d6210a957 ("mptcp: implement and use MPTCP-level retransmission")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 13777c35496c..ad60d04f4981 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2326,7 +2326,7 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
 			continue;
 		}
 
-		if (subflow->backup) {
+		if (subflow->backup || subflow->request_bkup) {
 			if (!backup)
 				backup = ssk;
 			continue;

---
base-commit: 971367a56ebfd1fd70608813898e48681281da99
change-id: 20240813-mptcp-sched-retrans-bkp-291785163b52

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

Re: [PATCH mptcp-net] mptcp: sched: check both backup in retrans

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Critical: Global Timeout ❌
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10369004703

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


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] mptcp: sched: check both backup in retrans

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10369004703

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


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] mptcp: sched: check both backup in retrans

[Mat Martineau]

On Tue, 13 Aug 2024, Matthieu Baerts (NGI0) wrote:

> The 'mptcp_subflow_context' structure has two items related to the
> backup flags:
>
> - 'backup': the subflow has been marked as backup by the other peer
>
> - 'request_bkup': the backup flag has been set by the host
>
> Looking only at the 'backup' flag can make sense in some cases, but it
> is not the behaviour of the default packet scheduler when selecting
> paths.
>
> As explained in the commit b6a66e521a20 ("mptcp: sched: check both
> directions for backup"), the packet scheduler should look at both flags,
> because that was the behaviour from the beginning: the 'backup' flag was
> set by accident instead of the 'request_bkup' one. Now that the latter
> has been fixed, get_retrans() needs to be adapted as well.
>
> Fixes: 3b1d6210a957 ("mptcp: implement and use MPTCP-level retransmission")

I think this should have:

Fixes: b6a66e521a20 ("mptcp: sched: check both directions for backup")

and a Cc: stable ?


Otherwise LGTM:

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


> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 13777c35496c..ad60d04f4981 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2326,7 +2326,7 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
> 			continue;
> 		}
>
> -		if (subflow->backup) {
> +		if (subflow->backup || subflow->request_bkup) {
> 			if (!backup)
> 				backup = ssk;
> 			continue;
>
> ---
> base-commit: 971367a56ebfd1fd70608813898e48681281da99
> change-id: 20240813-mptcp-sched-retrans-bkp-291785163b52
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>

Re: [PATCH mptcp-net] mptcp: sched: check both backup in retrans

[Matthieu Baerts]

Hi Mat,

On 21/08/2024 03:24, Mat Martineau wrote:
> On Tue, 13 Aug 2024, Matthieu Baerts (NGI0) wrote:
> 
>> The 'mptcp_subflow_context' structure has two items related to the
>> backup flags:
>>
>> - 'backup': the subflow has been marked as backup by the other peer
>>
>> - 'request_bkup': the backup flag has been set by the host
>>
>> Looking only at the 'backup' flag can make sense in some cases, but it
>> is not the behaviour of the default packet scheduler when selecting
>> paths.
>>
>> As explained in the commit b6a66e521a20 ("mptcp: sched: check both
>> directions for backup"), the packet scheduler should look at both flags,
>> because that was the behaviour from the beginning: the 'backup' flag was
>> set by accident instead of the 'request_bkup' one. Now that the latter
>> has been fixed, get_retrans() needs to be adapted as well.
>>
>> Fixes: 3b1d6210a957 ("mptcp: implement and use MPTCP-level
>> retransmission")
> 
> I think this should have:
> 
> Fixes: b6a66e521a20 ("mptcp: sched: check both directions for backup")

Good idea, I switched to that one when applying the patch.

> and a Cc: stable ?

Yes, I add them when sending patches to netdev.

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

New patches for t/upstream-net and t/upstream:
- 353d48321ce0: mptcp: sched: check both backup in retrans
- Results: c025238d573a..76f6fba9ef6b (export-net)
- Results: a2eeb886f736..80d8c6b850fb (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/2e798b8d759eb6a9a1ee14544e625cae61a9cad3/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/91a502295edf01f2c23e9f11edad1d7db908d21f/checks

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