[PATCH mptcp-next v2 0/4] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v2 0/4] userspace pm remove id 0 subflow & address

[Geliang Tang]

v2:
 - fix CI errors.

This patchset addresses #379 and #391, add the abilities to remove id 0
subflow and address for userspace PM. And a selftest.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/391

Geliang Tang (4):
  mptcp: allow creating id 0 subflow
  mptcp: remove id 0 subflow & address
  selftests: mptcp: add id argument for dsf
  selftests: mptcp: remove id 0 subflow & address

 net/mptcp/pm_userspace.c                      | 46 ++++++++++++++---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 49 +++++++++++++++++++
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 32 +++++++++++-
 3 files changed, 119 insertions(+), 8 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v2 1/4] mptcp: allow creating id 0 subflow

[Geliang Tang]

This patch drops id 0 limitation in mptcp_nl_cmd_sf_create() to allow
creating additional subflows with the local addr ID 0.

Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/391
Suggested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b5a8aa4c1ebd..d042d32beb4d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -307,12 +307,6 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
-	if (addr_l.id == 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "missing local addr id");
-		err = -EINVAL;
-		goto create_err;
-	}
-
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
-- 
2.35.3

Re: [PATCH mptcp-next v2 1/4] mptcp: allow creating id 0 subflow

[Matthieu Baerts]

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> This patch drops id 0 limitation in mptcp_nl_cmd_sf_create() to allow
> creating additional subflows with the local addr ID 0.

Please always add the reason explaining why this commit is needed, e.g.

  There is no reason not to allow additional subflows from this local
address: we should be able to create new subflows from the initial
endpoint. This limitation was breaking fullmesh support from userspace.

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

[PATCH mptcp-next v2 2/4] mptcp: remove id 0 subflow & address

[Geliang Tang]

This patch adds the ability to send RM_ADDR for local ID 0 and the
ability to remove id 0 subflow.

Put id 0 into a removing list, pass it to mptcp_pm_remove_addr() to
remve id 0 address and pass it to mptcp_pm_nl_rm_subflow_received() to
remove id 0 subflow.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d042d32beb4d..38629ebc4ec6 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -239,6 +239,21 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (id_val == 0) {
+		struct mptcp_rm_list list = { .nr = 0 };
+
+		list.ids[list.nr++] = 0;
+
+		lock_sock((struct sock *)msk);
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_remove_addr(msk, &list);
+		spin_unlock_bh(&msk->pm.lock);
+		release_sock((struct sock *)msk);
+
+		err = 0;
+		goto remove_err;
+	}
+
 	lock_sock((struct sock *)msk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
@@ -399,14 +414,16 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
 	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
+	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	struct mptcp_addr_info addr_l;
 	struct mptcp_addr_info addr_r;
 	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
 	int err = -EINVAL;
 	u32 token_val;
+	u8 id_val;
 
-	if (!laddr || !raddr || !token) {
+	if (((!laddr || !raddr) && !id) || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
 		return err;
 	}
@@ -424,6 +441,27 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 		goto destroy_err;
 	}
 
+	if (id) {
+		id_val = nla_get_u8(id);
+		if (id_val == 0) {
+			struct mptcp_rm_list list = { .nr = 0 };
+
+			list.ids[list.nr++] = 0;
+
+			sk = (struct sock *)msk;
+			lock_sock(sk);
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_nl_rm_subflow_received(msk, &list);
+			spin_unlock_bh(&msk->pm.lock);
+			release_sock(sk);
+
+			err = 0;
+		} else {
+			err = -EINVAL;
+		}
+		goto destroy_err;
+	}
+
 	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
-- 
2.35.3

Re: [PATCH mptcp-next v2 2/4] mptcp: remove id 0 subflow & address

[Matthieu Baerts]

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> This patch adds the ability to send RM_ADDR for local ID 0 and the
> ability to remove id 0 subflow.
> 
> Put id 0 into a removing list, pass it to mptcp_pm_remove_addr() to
> remve id 0 address and pass it to mptcp_pm_nl_rm_subflow_received() to

(s/remve/remove/)

> remove id 0 subflow.

Please always add the reason, e.g.

  There is no reason not to allow the userspace to remove the initial
address (ID 0) and destroy the initial subflow. This special case was
not taken into account not letting the userspace to delete all
addresses/subflows as announced.

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379

I wonder if we should consider this as a fix: it looks like a bug that
the remove address for ID 0 and the destroy of the initial subflow don't
work.

In this case, we might need:

  Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
  Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")

While at it, it might then be better to split this commit in two: one to
remove the address and one to destroy the subflow with id 0. WDYT?

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index d042d32beb4d..38629ebc4ec6 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -239,6 +239,21 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  		goto remove_err;
>  	}
>  
> +	if (id_val == 0) {
> +		struct mptcp_rm_list list = { .nr = 0 };
> +
> +		list.ids[list.nr++] = 0;
> +
> +		lock_sock((struct sock *)msk);
> +		spin_lock_bh(&msk->pm.lock);
> +		mptcp_pm_remove_addr(msk, &list);
> +		spin_unlock_bh(&msk->pm.lock);
> +		release_sock((struct sock *)msk);
> +
> +		err = 0;

I think an additional check is needed to make sure ID 0 has not already
been removed, no?

By doing that, we would have a similar behaviour to what we have with
the other IDs, e.g. if we send two remove address command for the same
ID, the second one will fail with "address with specified id not found"
message. We should do the same for ID 0.

> +		goto remove_err;

It is strange to use a label with "err" in the name when there is no
error. Probably better to rename it? ("out"?)

> +	}
> +
>  	lock_sock((struct sock *)msk);
>  
>  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> @@ -399,14 +414,16 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
>  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
>  	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
> +	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
>  	struct mptcp_addr_info addr_l;
>  	struct mptcp_addr_info addr_r;
>  	struct mptcp_sock *msk;
>  	struct sock *sk, *ssk;
>  	int err = -EINVAL;
>  	u32 token_val;
> +	u8 id_val;
>  
> -	if (!laddr || !raddr || !token) {
> +	if (((!laddr || !raddr) && !id) || !token) {
>  		GENL_SET_ERR_MSG(info, "missing required inputs");
>  		return err;
>  	}
> @@ -424,6 +441,27 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  		goto destroy_err;
>  	}
>  
> +	if (id) {
> +		id_val = nla_get_u8(id);
> +		if (id_val == 0) {
> +			struct mptcp_rm_list list = { .nr = 0 };
> +
> +			list.ids[list.nr++] = 0;
> +
> +			sk = (struct sock *)msk;
> +			lock_sock(sk);
> +			spin_lock_bh(&msk->pm.lock);
> +			mptcp_pm_nl_rm_subflow_received(msk, &list);
> +			spin_unlock_bh(&msk->pm.lock);
> +			release_sock(sk);
> +
> +			err = 0;
> +		} else {
> +			err = -EINVAL;
> +		}

I don't think we should do that to fix the issue #379: an ID is linked
to a local IP address and you can have multiple subflows using the same
local IP address, including the initial local IP address (with ID 0).
The current goal of the destroy subflow command is to destroy a specific
subflow. For the issue #379, we should instead accept the destruction of
the initial subflow when we give its local and remove address like we
would do for any other subflows. Or maybe this is something that already
work? e.g. if you create an MPTCP connection from A to B, then a new
subflow from C to D, can you remove the initial subflow (A <-> B)?

What you did here in mptcp_nl_cmd_sf_destroy() looks like a new feature
to be able to destroy all subflows linked to an ID address: an
optimisation not to send a destroy address for each subflows linked to
this address. But is there really a need to be able to do that? If yes,
there is no reason to restrict that to ID 0 and the comment in the
header file in uapi should be updated too. Anyway, that's for a
dedicated commit and that's for -next.

> +		goto destroy_err;

Same here: here, there is potentially no error, best to rename the label
to avoid confusions

> +	}
> +
>  	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
>  	if (err < 0) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");

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

[PATCH mptcp-next v2 3/4] selftests: mptcp: add id argument for dsf

[Geliang Tang]

This patch adds a id argument for userspace PM subflow removing
function dsf(), to remove id 0 subflow.

It can be used like this:

	./pm_nl_ctl dsf token $tk id 0

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 32 ++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 49369c4a5f26..8a5fa655404a 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -347,6 +347,7 @@ int dsf(int fd, int pm_family, int argc, char *argv[])
 	u_int32_t token;
 	int addr_start;
 	int off = 0;
+	u_int8_t id;
 	int arg;
 
 	const char *params[5];
@@ -358,9 +359,37 @@ int dsf(int fd, int pm_family, int argc, char *argv[])
 	off = init_genl_req(data, pm_family, MPTCP_PM_CMD_SUBFLOW_DESTROY,
 			    MPTCP_PM_VER);
 
-	if (argc < 12)
+	if (argc < 6)
 		syntax(argv);
 
+	if (argc < 12) {
+		for (arg = 2; arg < argc; arg++) {
+			if (!strcmp(argv[arg], "id")) {
+				if (++arg >= argc)
+					error(1, 0, " missing id value");
+
+				id = atoi(argv[arg]);
+				rta = (void *)(data + off);
+				rta->rta_type = MPTCP_PM_ATTR_LOC_ID;
+				rta->rta_len = RTA_LENGTH(1);
+				memcpy(RTA_DATA(rta), &id, 1);
+				off += NLMSG_ALIGN(rta->rta_len);
+			} else if (!strcmp(argv[arg], "token")) {
+				if (++arg >= argc)
+					error(1, 0, " missing token value");
+
+				token = strtoul(argv[arg], NULL, 10);
+				rta = (void *)(data + off);
+				rta->rta_type = MPTCP_PM_ATTR_TOKEN;
+				rta->rta_len = RTA_LENGTH(4);
+				memcpy(RTA_DATA(rta), &token, 4);
+				off += NLMSG_ALIGN(rta->rta_len);
+			} else
+				error(1, 0, "unknown keyword %s", argv[arg]);
+		}
+		goto out;
+	}
+
 	/* Params recorded in this order:
 	 * <local-ip>, <local-port>, <remote-ip>, <remote-port>, <token>
 	 */
@@ -443,6 +472,7 @@ int dsf(int fd, int pm_family, int argc, char *argv[])
 	memcpy(RTA_DATA(rta), &token, 4);
 	off += NLMSG_ALIGN(rta->rta_len);
 
+out:
 	do_nl_req(fd, nh, off, 0);
 
 	return 0;
-- 
2.35.3

Re: [PATCH mptcp-next v2 3/4] selftests: mptcp: add id argument for dsf

[Matthieu Baerts]

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> This patch adds a id argument for userspace PM subflow removing
> function dsf(), to remove id 0 subflow.
> 
> It can be used like this:
> 
> 	./pm_nl_ctl dsf token $tk id 0

Please see my comment in patch 2/4: this patch is maybe not needed (at
least not to fix issue #379 for mptcp-net).

(if it is needed, please add a reason and a link to the parent comment)

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

[PATCH mptcp-next v2 4/4] selftests: mptcp: remove id 0 subflow & address

[Geliang Tang]

This patch adds selftests for userpsace PM to remove id 0 subflow and
id 0 address.

A new helper userspace_pm_rm_id_0_subflow_or_address_ns2() is added,
in it use

        ./pm_nl_ctl dsf token $tk id 0

to remove id 0 subflow, and use

        ./pm_nl_ctl rem token $tk id 0

to remove id 0 address.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ee1f89a872b3..52f081738c36 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3320,6 +3320,15 @@ userspace_pm_rm_sf_addr_ns2()
 	wait_rm_sf $ns2 1
 }
 
+userspace_pm_rm_id_0_subflow_or_address_ns2()
+{
+	local tk
+
+	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
+	ip netns exec $ns2 ./pm_nl_ctl "$1" token $tk id 0
+	sleep 0.5
+}
+
 userspace_tests()
 {
 	# userspace pm type prevents add_addr
@@ -3434,6 +3443,46 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove id 0 subflow
+	if reset_with_events "userspace pm remove id 0 subflow" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns2
+		pm_nl_set_limits $ns1 0 2
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		userspace_pm_add_sf 10.0.1.2 0
+		userspace_pm_add_sf 10.0.3.2 20
+		chk_join_nr 2 2 2
+		chk_mptcp_info subflows 2 subflows 2
+		userspace_pm_rm_id_0_subflow_or_address_ns2 dsf
+		chk_mptcp_info subflows 1 subflows 1
+		chk_rm_nr 0 2
+		kill_events_pids
+		wait $tests_pid
+	fi
+
+	# userspace pm remove id 0 address
+	if reset_with_events "userspace pm remove id 0 address" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns2
+		pm_nl_set_limits $ns1 0 2
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		userspace_pm_add_sf 10.0.1.2 0
+		userspace_pm_add_sf 10.0.3.2 20
+		chk_join_nr 2 2 2
+		chk_mptcp_info subflows 2 subflows 2
+		userspace_pm_rm_id_0_subflow_or_address_ns2 rem
+		chk_mptcp_info subflows 1 subflows 1
+		chk_rm_nr 2 0
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: selftests: mptcp: remove id 0 subflow & address: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5155611524988928
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5155611524988928/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6281511431831552
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6281511431831552/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6000036455120896
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6000036455120896/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4874136548278272
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4874136548278272/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/49bea109b80c


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

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 (Tessares)

Re: [PATCH mptcp-next v2 4/4] selftests: mptcp: remove id 0 subflow & address

[Matthieu Baerts]

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> This patch adds selftests for userpsace PM to remove id 0 subflow and
> id 0 address.
> 
> A new helper userspace_pm_rm_id_0_subflow_or_address_ns2() is added,
> in it use
> 
>         ./pm_nl_ctl dsf token $tk id 0
> 
> to remove id 0 subflow, and use
> 
>         ./pm_nl_ctl rem token $tk id 0
> 
> to remove id 0 address.

See my comment on patch 2/4: OK for the "./pm_nl_ctl rem token $tk id 0"
but not for "dsf id 0" for -net to fix issue #379.

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ee1f89a872b3..52f081738c36 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3320,6 +3320,15 @@ userspace_pm_rm_sf_addr_ns2()
>  	wait_rm_sf $ns2 1
>  }
>  
> +userspace_pm_rm_id_0_subflow_or_address_ns2()

It might be useful to either add a comment about what the parameter are
(# $1: command (rem/dsf)) and/or use an dedicated local env var (local
cmd=${1}) otherwise we need to read the code to know which parameters
need to be given.


> +{
> +	local tk
> +
> +	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
> +	ip netns exec $ns2 ./pm_nl_ctl "$1" token $tk id 0
> +	sleep 0.5
> +}
> +
>  userspace_tests()
>  {
>  	# userspace pm type prevents add_addr
> @@ -3434,6 +3443,46 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm remove id 0 subflow
> +	if reset_with_events "userspace pm remove id 0 subflow" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		set_userspace_pm $ns2
> +		pm_nl_set_limits $ns1 0 2
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns2
> +		userspace_pm_add_sf 10.0.1.2 0

Is it not going to create a subflow using the same IPs as the original
subflow? Is it on purpose? It looks confusing.

> +		userspace_pm_add_sf 10.0.3.2 20
> +		chk_join_nr 2 2 2
> +		chk_mptcp_info subflows 2 subflows 2
> +		userspace_pm_rm_id_0_subflow_or_address_ns2 dsf

Linked to my comment on patch 2/4, here we should do something like:

> ip netns exec $ns2 ./pm_nl_ctl dsf token $tk lip $sa lport $sp rip $da rport $dp token $tk     

where the source IP + port and destination IP + port are the ones from
the initial connection (10.0.1.2 -> 10.0.1.1?)

WDYT?

> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_rm_nr 0 2
> +		kill_events_pids
> +		wait $tests_pid
> +	fi
> +
> +	# userspace pm remove id 0 address
> +	if reset_with_events "userspace pm remove id 0 address" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		set_userspace_pm $ns2
> +		pm_nl_set_limits $ns1 0 2
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns2
> +		userspace_pm_add_sf 10.0.1.2 0

(same here: are you not creating a new subflow with the same IPs as the
original one? It might be confusing, no? But maybe on purpose? In this
case, please add a comment)

> +		userspace_pm_add_sf 10.0.3.2 20
> +		chk_join_nr 2 2 2
> +		chk_mptcp_info subflows 2 subflows 2
> +		userspace_pm_rm_id_0_subflow_or_address_ns2 rem
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_rm_nr 2 0
> +		kill_events_pids
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

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

Re: [PATCH mptcp-next v2 0/4] userspace pm remove id 0 subflow & address

[Matthieu Baerts]

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> v2:
>  - fix CI errors.
> 
> This patchset addresses #379 and #391, add the abilities to remove id 0
> subflow and address for userspace PM. And a selftest.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/391

Thank you for these patches!

The code of the first one looks good to me, same for the first part of
patch 2/4. Please see the comments in the individual patches for more
details.

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