If there are only "staled" subflows, pick the first one we found (Mat).

Note that we could also check:

  !subflow>backup && !subflow->request_bkup

It's only a ACK, maybe OK to send that on the backup paths?

If we want this, should we have two alternatives?

  - First try on backup if any ; if not, on "staled" ones?

  - Or just on one of them?

We can also fill an issue, and wait for feedback.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Based-on: <20240802-mptcp-pm-avail-v6-0-964ba9ce279f@kernel.org>
---
 net/mptcp/pm_netlink.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index cb8f7de7ed6c..3bdb0219188f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -762,7 +762,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 
 void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
 {
-	struct mptcp_subflow_context *subflow;
+	struct mptcp_subflow_context *subflow, *alt = NULL;
 
 	msk_owned_by_me(msk);
 	lockdep_assert_held(&msk->pm.lock);
@@ -772,11 +772,19 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
 		return;
 
 	mptcp_for_each_subflow(msk, subflow) {
-		if (!subflow->stale && __mptcp_subflow_active(subflow)) {
-			mptcp_pm_send_ack(msk, subflow, false, false);
-			break;
+		if (__mptcp_subflow_active(subflow)) {
+			if (!subflow->stale) {
+				mptcp_pm_send_ack(msk, subflow, false, false);
+				return;
+			}
+
+			if (!alt)
+				alt = subflow;
 		}
 	}
+
+	if (alt)
+		mptcp_pm_send_ack(msk, alt, false, false);
 }
 
 int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
-- 
2.45.2

On Wed, 7 Aug 2024, Matthieu Baerts (NGI0) wrote:

> If there are only "staled" subflows, pick the first one we found (Mat).
>
> Note that we could also check:
>
>  !subflow>backup && !subflow->request_bkup
>
> It's only a ACK, maybe OK to send that on the backup paths?
>
> If we want this, should we have two alternatives?
>
>  - First try on backup if any ; if not, on "staled" ones?
>
>  - Or just on one of them?
>
> We can also fill an issue, and wait for feedback.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Thanks Matthieu, looks good to squash.

- Mat

> ---
> Based-on: <20240802-mptcp-pm-avail-v6-0-964ba9ce279f@kernel.org>
> ---
> net/mptcp/pm_netlink.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index cb8f7de7ed6c..3bdb0219188f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -762,7 +762,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>
> void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> {
> -	struct mptcp_subflow_context *subflow;
> +	struct mptcp_subflow_context *subflow, *alt = NULL;
>
> 	msk_owned_by_me(msk);
> 	lockdep_assert_held(&msk->pm.lock);
> @@ -772,11 +772,19 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> 		return;
>
> 	mptcp_for_each_subflow(msk, subflow) {
> -		if (!subflow->stale && __mptcp_subflow_active(subflow)) {
> -			mptcp_pm_send_ack(msk, subflow, false, false);
> -			break;
> +		if (__mptcp_subflow_active(subflow)) {
> +			if (!subflow->stale) {
> +				mptcp_pm_send_ack(msk, subflow, false, false);
> +				return;
> +			}
> +
> +			if (!alt)
> +				alt = subflow;
> 		}
> 	}
> +
> +	if (alt)
> +		mptcp_pm_send_ack(msk, alt, false, false);
> }
>
> int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> -- 
> 2.45.2
>
>
>

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 1 failed test(s): selftest_simult_flows 🔴
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10286533140

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


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)