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>

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>
>
>
>

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.