[PATCH mptcp-next v3 00/29] userspace pm enhancements

[PATCH mptcp-next v3 00/29] userspace pm enhancements

[Geliang Tang]

v3:
 - drop patch 4 in v2
 - invert patch 10 and 11 in v2
 - rebased

1-7: some small cleanups, v3
8-9: address #428, add mptcpi_subflows_total
10-16: address #379, #391, userspace pm remove id 0 subflow & address, v11
17-19: address #403, add refcont for address entry
20: add userspace fullmesh tests
21-29: seltests cleanups

Geliang Tang (29):
  mptcp: drop useless ssk in pm_subflow_check_next
  mptcp: use mptcp_check_fallback helper
  mptcp: use mptcp_get_ext helper
  mptcp: move sk assignment statement ahead
  mptcp: define more local variables sk
  selftests: mptcp: sockopt: drop mptcp_connect var
  selftests: mptcp: display simult in extra_msg
  mptcp: add mptcpi_subflows_total counter
  selftests: mptcp: add evts_get_info helper
  selftests: mptcp: add chk_subflows_total helper
  selftests: mptcp: update userspace pm test helpers
  selftests: mptcp: userspace pm remove id 0 subflow
  mptcp: userspace pm allow creating id 0 subflow
  selftests: mptcp: userspace pm create id 0 subflow
  mptcp: userspace pm remove id 0 address
  selftests: mptcp: userspace pm remove id 0 address
  mptcp: add userspace_pm_get_entry helper
  mptcp: add userspace pm addr entry refcount
  mptcp: add netlink pm addr entry refcount
  selftests: mptcp: add userspace pm fullmesh tests
  selftests: mptcp: add mptcp_lib_kill_wait
  selftests: mptcp: add mptcp_lib_evts_*
  selftests: mptcp: userspace: print colored results
  selftests: mptcp: add mptcp_lib_verify_listener_events
  selftests: mptcp: add mptcp_lib_is_v6
  selftests: mptcp: add mptcp_lib_get_counter
  selftests: mptcp: add mptcp_lib_make_file
  selftests: mptcp: add mptcp_lib_check_transfer
  selftests: mptcp: add mptcp_lib_wait_local_port_listen

 include/uapi/linux/mptcp.h                    |   1 +
 net/mptcp/pm.c                                |   2 +-
 net/mptcp/pm_netlink.c                        |  25 +-
 net/mptcp/pm_userspace.c                      | 139 ++++--
 net/mptcp/protocol.c                          |   6 +-
 net/mptcp/protocol.h                          |  14 +-
 net/mptcp/sockopt.c                           |   4 +-
 tools/testing/selftests/net/mptcp/diag.sh     |  23 +-
 .../selftests/net/mptcp/mptcp_connect.sh      | 108 +---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 460 +++++++++---------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 188 +++++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  55 +--
 .../selftests/net/mptcp/simult_flows.sh       |  19 +-
 .../selftests/net/mptcp/userspace_pm.sh       | 199 +++-----
 14 files changed, 661 insertions(+), 582 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v3 01/29] mptcp: drop useless ssk in pm_subflow_check_next

[Geliang Tang]

The code using 'ssk' parameter of mptcp_pm_subflow_check_next() has been
dropped in commit "95d686517884 (mptcp: fix subflow accounting on close)".
So drop this useless parameter ssk.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c       | 2 +-
 net/mptcp/protocol.c | 2 +-
 net/mptcp/protocol.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d8da5374d9e1..4ae19113b8eb 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -184,7 +184,7 @@ void mptcp_pm_subflow_established(struct mptcp_sock *msk)
 	spin_unlock_bh(&pm->lock);
 }
 
-void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
+void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow)
 {
 	struct mptcp_pm_data *pm = &msk->pm;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6f9e116598ed..6dd1ceaee257 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2480,7 +2480,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	/* subflow aborted before reaching the fully_established status
 	 * attempt the creation of the next subflow
 	 */
-	mptcp_pm_subflow_check_next(mptcp_sk(sk), ssk, subflow);
+	mptcp_pm_subflow_check_next(mptcp_sk(sk), subflow);
 
 	__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_PUSH);
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ce4f5fad1202..37e38fe23dc0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -893,7 +893,7 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk);
 void mptcp_pm_connection_closed(struct mptcp_sock *msk);
 void mptcp_pm_subflow_established(struct mptcp_sock *msk);
 bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk);
-void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
+void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow);
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr);
-- 
2.35.3

[PATCH mptcp-next v3 02/29] mptcp: use mptcp_check_fallback helper

[Geliang Tang]

Use __mptcp_check_fallback() helper defined in net/mptcp/protocol.h,
instead of open-coding it in both __mptcp_do_fallback() and
mptcp_diag_fill_info().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.h | 2 +-
 net/mptcp/sockopt.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 37e38fe23dc0..340fdf2a7473 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1070,7 +1070,7 @@ static inline bool mptcp_check_fallback(const struct sock *sk)
 
 static inline void __mptcp_do_fallback(struct mptcp_sock *msk)
 {
-	if (test_bit(MPTCP_FALLBACK_DONE, &msk->flags)) {
+	if (__mptcp_check_fallback(msk)) {
 		pr_debug("TCP fallback already done (msk=%p)", msk);
 		return;
 	}
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 340e87195a27..a2845be0a4e0 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -916,7 +916,7 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 			mptcp_pm_get_local_addr_max(msk);
 	}
 
-	if (test_bit(MPTCP_FALLBACK_DONE, &msk->flags))
+	if (__mptcp_check_fallback(msk))
 		flags |= MPTCP_INFO_FLAG_FALLBACK;
 	if (READ_ONCE(msk->can_ack))
 		flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED;
-- 
2.35.3

[PATCH mptcp-next v3 03/29] mptcp: use mptcp_get_ext helper

[Geliang Tang]

Use mptcp_get_ext() helper defined in protocol.h instead of open-coding
it in mptcp_sendmsg_frag().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6dd1ceaee257..f11834d924ff 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1267,7 +1267,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 		 * queue management operation, to avoid breaking the ext <->
 		 * SSN association set here
 		 */
-		mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
+		mpext = mptcp_get_ext(skb);
 		if (!mptcp_skb_can_collapse_to(data_seq, skb, mpext)) {
 			TCP_SKB_CB(skb)->eor = 1;
 			goto alloc_skb;
@@ -1289,7 +1289,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 
 		i = skb_shinfo(skb)->nr_frags;
 		reuse_skb = false;
-		mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
+		mpext = mptcp_get_ext(skb);
 	}
 
 	/* Zero window and all data acked? Probe. */
-- 
2.35.3

[PATCH mptcp-next v3 04/29] mptcp: move sk assignment statement ahead

[Geliang Tang]

If we move the sk assignment statement ahead in mptcp_nl_cmd_sf_create()
or mptcp_nl_cmd_sf_destroy(), right after the msk null-check statements,
sk can be used after the create_err or destroy_err labels instead of
open-coding it again.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 07f602144d5e..bc1824a56dea 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -296,6 +296,8 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 		return err;
 	}
 
+	sk = (struct sock *)msk;
+
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
 		goto create_err;
@@ -319,8 +321,6 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
-	sk = (struct sock *)msk;
-
 	if (!mptcp_pm_addr_families_match(sk, &addr_l, &addr_r)) {
 		GENL_SET_ERR_MSG(info, "families mismatch");
 		err = -EINVAL;
@@ -348,7 +348,7 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	spin_unlock_bh(&msk->pm.lock);
 
  create_err:
-	sock_put((struct sock *)msk);
+	sock_put(sk);
 	return err;
 }
 
@@ -425,6 +425,8 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 		return err;
 	}
 
+	sk = (struct sock *)msk;
+
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
 		goto destroy_err;
@@ -454,7 +456,6 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 		goto destroy_err;
 	}
 
-	sk = (struct sock *)msk;
 	lock_sock(sk);
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
@@ -474,7 +475,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	release_sock(sk);
 
 destroy_err:
-	sock_put((struct sock *)msk);
+	sock_put(sk);
 	return err;
 }
 
-- 
2.35.3

[PATCH mptcp-next v3 05/29] mptcp: define more local variables sk

[Geliang Tang]

'(struct sock *)msk' is used several times in mptcp_nl_cmd_announce(),
mptcp_nl_cmd_remove() or mptcp_userspace_pm_set_flags() in pm_userspace.c,
it's worth adding a local variable sk to point it.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index bc1824a56dea..04e26eb01ba8 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -152,6 +152,7 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_pm_addr_entry addr_val;
 	struct mptcp_sock *msk;
 	int err = -EINVAL;
+	struct sock *sk;
 	u32 token_val;
 
 	if (!addr || !token) {
@@ -167,6 +168,8 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 		return err;
 	}
 
+	sk = (struct sock *)msk;
+
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
 		goto announce_err;
@@ -190,7 +193,7 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 		goto announce_err;
 	}
 
-	lock_sock((struct sock *)msk);
+	lock_sock(sk);
 	spin_lock_bh(&msk->pm.lock);
 
 	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
@@ -200,11 +203,11 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	spin_unlock_bh(&msk->pm.lock);
-	release_sock((struct sock *)msk);
+	release_sock(sk);
 
 	err = 0;
  announce_err:
-	sock_put((struct sock *)msk);
+	sock_put(sk);
 	return err;
 }
 
@@ -217,6 +220,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_sock *msk;
 	LIST_HEAD(free_list);
 	int err = -EINVAL;
+	struct sock *sk;
 	u32 token_val;
 	u8 id_val;
 
@@ -234,12 +238,14 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 		return err;
 	}
 
+	sk = (struct sock *)msk;
+
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
 		goto remove_err;
 	}
 
-	lock_sock((struct sock *)msk);
+	lock_sock(sk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
 		if (entry->addr.id == id_val) {
@@ -250,7 +256,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (!match) {
 		GENL_SET_ERR_MSG(info, "address with specified id not found");
-		release_sock((struct sock *)msk);
+		release_sock(sk);
 		goto remove_err;
 	}
 
@@ -258,15 +264,15 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	mptcp_pm_remove_addrs(msk, &free_list);
 
-	release_sock((struct sock *)msk);
+	release_sock(sk);
 
 	list_for_each_entry_safe(match, entry, &free_list, list) {
-		sock_kfree_s((struct sock *)msk, match, sizeof(*match));
+		sock_kfree_s(sk, match, sizeof(*match));
 	}
 
 	err = 0;
  remove_err:
-	sock_put((struct sock *)msk);
+	sock_put(sk);
 	return err;
 }
 
@@ -485,6 +491,7 @@ int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
 {
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
+	struct sock *sk;
 	u32 token_val;
 
 	token_val = nla_get_u32(token);
@@ -493,6 +500,8 @@ int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
 	if (!msk)
 		return ret;
 
+	sk = (struct sock *)msk;
+
 	if (!mptcp_pm_is_userspace(msk))
 		goto set_flags_err;
 
@@ -500,11 +509,11 @@ int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
 	    rem->addr.family == AF_UNSPEC)
 		goto set_flags_err;
 
-	lock_sock((struct sock *)msk);
+	lock_sock(sk);
 	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc->addr, &rem->addr, bkup);
-	release_sock((struct sock *)msk);
+	release_sock(sk);
 
 set_flags_err:
-	sock_put((struct sock *)msk);
+	sock_put(sk);
 	return ret;
 }
-- 
2.35.3

[PATCH mptcp-next v3 06/29] selftests: mptcp: sockopt: drop mptcp_connect var

[Geliang Tang]

Global var mptcp_connect defined at the front of mptcp_sockopt.sh is
duplicate with local var mptcp_connect defined in do_transfer(), drop
this useless global one.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 8c8694f21e7d..a817af6616ec 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -11,7 +11,6 @@ cout=""
 ksft_skip=4
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
-mptcp_connect=""
 iptables="iptables"
 ip6tables="ip6tables"
 
-- 
2.35.3

[PATCH mptcp-next v3 07/29] selftests: mptcp: display simult in extra_msg

[Geliang Tang]

Just like displaying "invert" after "Info: ", "simult" should be
displayed too when rm_subflow_nr dosen't match the expect value in
chk_rm_nr():

      syn                                 [ ok ]
      synack                              [ ok ]
      ack                                 [ ok ]
      add                                 [ ok ]
      echo                                [ ok ]
      rm                                  [ ok ]
      rmsf                                [ ok ] 3 in [2:4]
      Info: invert simult

      syn                                 [ ok ]
      synack                              [ ok ]
      ack                                 [ ok ]
      add                                 [ ok ]
      echo                                [ ok ]
      rm                                  [ ok ]
      rmsf                                [ ok ]
      Info: invert

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ee1f89a872b3..d02e53be8b31 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1766,7 +1766,9 @@ chk_rm_nr()
 		# in case of simult flush, the subflow removal count on each side is
 		# unreliable
 		count=$((count + cnt))
-		[ "$count" != "$rm_subflow_nr" ] && suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]"
+		[ "$count" != "$rm_subflow_nr" ] && \
+			suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]" && \
+			extra_msg="$extra_msg simult"
 		if [ $count -ge "$rm_subflow_nr" ] && \
 		   [ "$count" -le "$((rm_subflow_nr *2 ))" ]; then
 			print_ok "$suffix"
-- 
2.35.3

Re: [PATCH mptcp-next v3 07/29] selftests: mptcp: display simult in extra_msg

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> Just like displaying "invert" after "Info: ", "simult" should be
> displayed too when rm_subflow_nr dosen't match the expect value in
> chk_rm_nr():
> 
>       syn                                 [ ok ]
>       synack                              [ ok ]
>       ack                                 [ ok ]
>       add                                 [ ok ]
>       echo                                [ ok ]
>       rm                                  [ ok ]
>       rmsf                                [ ok ] 3 in [2:4]
>       Info: invert simult
> 
>       syn                                 [ ok ]
>       synack                              [ ok ]
>       ack                                 [ ok ]
>       add                                 [ ok ]
>       echo                                [ ok ]
>       rm                                  [ ok ]
>       rmsf                                [ ok ]
>       Info: invert
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ee1f89a872b3..d02e53be8b31 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1766,7 +1766,9 @@ chk_rm_nr()
>  		# in case of simult flush, the subflow removal count on each side is
>  		# unreliable
>  		count=$((count + cnt))
> -		[ "$count" != "$rm_subflow_nr" ] && suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]"
> +		[ "$count" != "$rm_subflow_nr" ] && \
> +			suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]" && \
> +			extra_msg="$extra_msg simult"

Good idea!

I think it would be clearer to use an explicit "if":

  if [ "$count" != "$rm_subflow_nr" ]; then
      (...)

I can do the modification when applying the patch if needed.

Cheers,
Matt

>  		if [ $count -ge "$rm_subflow_nr" ] && \
>  		   [ "$count" -le "$((rm_subflow_nr *2 ))" ]; then
>  			print_ok "$suffix"

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-next v3 08/29] mptcp: add mptcpi_subflows_total counter

[Geliang Tang]

If the initial subflow has been removed, we cannot know without checking
other counters, e.g. ss -ti <filter> | grep -c tcp-ulp-mptcp or
getsockopt(SOL_MPTCP, MPTCP_FULL_INFO, ...) (or others except MPTCP_INFO
of course) and then check mptcp_subflow_data->num_subflows to get the
total amount of subflows.

This patch adds a new counter mptcpi_subflows_total in mptcpi_flags to
store the total amount of subflows, including the initial one. A new
helper __mptcp_has_initial_subflow() is added to check whether the
initial subflow has been removed or not. With this helper, we can then
compute the total amount of subflows from mptcp_info by doing something
like:

    mptcpi_subflows_total = mptcpi_subflows +
            __mptcp_has_initial_subflow(msk).

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/428
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/uapi/linux/mptcp.h | 1 +
 net/mptcp/protocol.h       | 7 +++++++
 net/mptcp/sockopt.c        | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 64ecc8a3f9f2..166bb9bad05c 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -61,6 +61,7 @@ struct mptcp_info {
 	__u64	mptcpi_bytes_sent;
 	__u64	mptcpi_bytes_received;
 	__u64	mptcpi_bytes_acked;
+	__u8	mptcpi_subflows_total;
 };
 
 /* MPTCP Reset reason codes, rfc8684 */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 340fdf2a7473..6508179e94a6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1077,6 +1077,13 @@ static inline void __mptcp_do_fallback(struct mptcp_sock *msk)
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
 }
 
+static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
+{
+	struct sock *ssk = READ_ONCE(msk->first);
+
+	return ssk && inet_sk_state_load(ssk) != TCP_CLOSE;
+}
+
 static inline void mptcp_do_fallback(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index a2845be0a4e0..36602cc8c12d 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -935,6 +935,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_bytes_sent = msk->bytes_sent;
 	info->mptcpi_bytes_received = msk->bytes_received;
 	info->mptcpi_bytes_retrans = msk->bytes_retrans;
+	info->mptcpi_subflows_total = info->mptcpi_subflows +
+		__mptcp_has_initial_subflow(msk);
 	unlock_sock_fast(sk, slow);
 }
 EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
-- 
2.35.3

[PATCH mptcp-next v3 09/29] selftests: mptcp: add evts_get_info helper

[Geliang Tang]

This patch adds a new helper get_info_value(), using 'sed' command to
parse the value of the given item name in the line with the given keyword,
to make chk_mptcp_info() and pedit_action_pkts() more readable.

Also add another helper evts_get_info() to use get_info_value() to parse
the output of 'pm_nl_ctl events' command, to make all the userpsace pm
selftests more readable, both in mptcp_join.sh and userspace_pm.sh.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 19 ++--
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 10 +++
 .../selftests/net/mptcp/userspace_pm.sh       | 86 +++++++++----------
 3 files changed, 57 insertions(+), 58 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index d02e53be8b31..92285ef592fb 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1864,10 +1864,8 @@ chk_mptcp_info()
 
 	print_check "mptcp_info ${info1:0:8}=$exp1:$exp2"
 
-	cnt1=$(ss -N $ns1 -inmHM | grep "$info1:" |
-	       sed -n 's/.*\('"$info1"':\)\([[:digit:]]*\).*$/\2/p;q')
-	cnt2=$(ss -N $ns2 -inmHM | grep "$info2:" |
-	       sed -n 's/.*\('"$info2"':\)\([[:digit:]]*\).*$/\2/p;q')
+	cnt1=$(ss -N $ns1 -inmHM | mptcp_lib_get_info_value "$info1" "$info1")
+	cnt2=$(ss -N $ns2 -inmHM | mptcp_lib_get_info_value "$info2" "$info2")
 	# 'ss' only display active connections and counters that are not 0.
 	[ -z "$cnt1" ] && cnt1=0
 	[ -z "$cnt2" ] && cnt2=0
@@ -2830,13 +2828,13 @@ verify_listener_events()
 		return
 	fi
 
-	type=$(grep "type:$e_type," $evt | sed -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q')
-	family=$(grep "type:$e_type," $evt | sed -n 's/.*\(family:\)\([[:digit:]]*\).*$/\2/p;q')
-	sport=$(grep "type:$e_type," $evt | sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
+	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
+	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
+	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
 	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(grep "type:$e_type," $evt | sed -n 's/.*\(saddr6:\)\([0-9a-f:.]*\).*$/\2/p;q')
+		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
 	else
-		saddr=$(grep "type:$e_type," $evt | sed -n 's/.*\(saddr4:\)\([0-9.]*\).*$/\2/p;q')
+		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
 	fi
 
 	if [ $type ] && [ $type = $e_type ] &&
@@ -3231,8 +3229,7 @@ fastclose_tests()
 pedit_action_pkts()
 {
 	tc -n $ns2 -j -s action show action pedit index 100 | \
-		grep "packets" | \
-		sed 's/.*"packets":\([0-9]\+\),.*/\1/'
+		mptcp_lib_get_info_value \"packets\" packets
 }
 
 fail_tests()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 92a5befe8039..def35395a254 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -207,3 +207,13 @@ mptcp_lib_result_print_all_tap() {
 		printf "%s\n" "${subtest}"
 	done
 }
+
+# get the value of keyword $1 in the line marked by keyword $2
+mptcp_lib_get_info_value() {
+	grep "${2}" | sed -n 's/.*\('${1}':\)\([0-9a-f:.]*\).*$/\2/p;q'
+}
+
+# $1: info name ; $2: evts_ns ; $3: event type
+mptcp_lib_evts_get_info() {
+	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index b25a3e33eb25..2413059a42e5 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -247,14 +247,11 @@ make_connection()
 	local server_token
 	local server_serverside
 
-	client_token=$(sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
-	client_port=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
-	client_serverside=$(sed --unbuffered -n 's/.*\(server_side:\)\([[:digit:]]*\).*$/\2/p;q'\
-				      "$client_evts")
-	server_token=$(grep "type:1," "$server_evts" |
-		       sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
-	server_serverside=$(grep "type:1," "$server_evts" |
-			    sed --unbuffered -n 's/.*\(server_side:\)\([[:digit:]]*\).*$/\2/p;q')
+	client_token=$(mptcp_lib_evts_get_info token "$client_evts")
+	client_port=$(mptcp_lib_evts_get_info sport "$client_evts")
+	client_serverside=$(mptcp_lib_evts_get_info server_side "$client_evts")
+	server_token=$(mptcp_lib_evts_get_info token "$server_evts")
+	server_serverside=$(mptcp_lib_evts_get_info server_side "$server_evts")
 
 	print_test "Established IP${is_v6} MPTCP Connection ns2 => ns1"
 	if [ "$client_token" != "" ] && [ "$server_token" != "" ] && [ "$client_serverside" = 0 ] &&
@@ -340,16 +337,16 @@ verify_announce_event()
 	local dport
 	local id
 
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	token=$(sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
+	type=$(mptcp_lib_evts_get_info type "$evt" $e_type)
+	token=$(mptcp_lib_evts_get_info token "$evt" $e_type)
 	if [ "$e_af" = "v6" ]
 	then
-		addr=$(sed --unbuffered -n 's/.*\(daddr6:\)\([0-9a-f:.]*\).*$/\2/p;q' "$evt")
+		addr=$(mptcp_lib_evts_get_info daddr6 "$evt" $e_type)
 	else
-		addr=$(sed --unbuffered -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evt")
+		addr=$(mptcp_lib_evts_get_info daddr4 "$evt" $e_type)
 	fi
-	dport=$(sed --unbuffered -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	id=$(sed --unbuffered -n 's/.*\(rem_id:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
+	dport=$(mptcp_lib_evts_get_info dport "$evt" $e_type)
+	id=$(mptcp_lib_evts_get_info rem_id "$evt" $e_type)
 
 	check_expected "type" "token" "addr" "dport" "id"
 }
@@ -367,7 +364,7 @@ test_announce()
 	   $client_addr_id dev ns2eth1 > /dev/null 2>&1
 
 	local type
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	type=$(mptcp_lib_evts_get_info type "$server_evts")
 	print_test "ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token"
 	if [ "$type" = "" ]
 	then
@@ -446,9 +443,9 @@ verify_remove_event()
 	local token
 	local id
 
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	token=$(sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	id=$(sed --unbuffered -n 's/.*\(rem_id:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
+	type=$(mptcp_lib_evts_get_info type "$evt" $e_type)
+	token=$(mptcp_lib_evts_get_info token "$evt" $e_type)
+	id=$(mptcp_lib_evts_get_info rem_id "$evt" $e_type)
 
 	check_expected "type" "token" "id"
 }
@@ -466,7 +463,7 @@ test_remove()
 	   $client_addr_id > /dev/null 2>&1
 	print_test "RM_ADDR id:${client_addr_id} ns2 => ns1, invalid token"
 	local type
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	type=$(mptcp_lib_evts_get_info type "$server_evts")
 	if [ "$type" = "" ]
 	then
 		test_pass
@@ -479,7 +476,7 @@ test_remove()
 	ip netns exec "$ns2" ./pm_nl_ctl rem token "$client4_token" id\
 	   $invalid_id > /dev/null 2>&1
 	print_test "RM_ADDR id:${invalid_id} ns2 => ns1, invalid id"
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	type=$(mptcp_lib_evts_get_info type "$server_evts")
 	if [ "$type" = "" ]
 	then
 		test_pass
@@ -583,19 +580,19 @@ verify_subflow_events()
 		fi
 	fi
 
-	type=$(sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	token=$(sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	family=$(sed --unbuffered -n 's/.*\(family:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	dport=$(sed --unbuffered -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	locid=$(sed --unbuffered -n 's/.*\(loc_id:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
-	remid=$(sed --unbuffered -n 's/.*\(rem_id:\)\([[:digit:]]*\).*$/\2/p;q' "$evt")
+	type=$(mptcp_lib_evts_get_info type "$evt" $e_type)
+	token=$(mptcp_lib_evts_get_info token "$evt" $e_type)
+	family=$(mptcp_lib_evts_get_info family "$evt" $e_type)
+	dport=$(mptcp_lib_evts_get_info dport "$evt" $e_type)
+	locid=$(mptcp_lib_evts_get_info loc_id "$evt" $e_type)
+	remid=$(mptcp_lib_evts_get_info rem_id "$evt" $e_type)
 	if [ "$family" = "$AF_INET6" ]
 	then
-		saddr=$(sed --unbuffered -n 's/.*\(saddr6:\)\([0-9a-f:.]*\).*$/\2/p;q' "$evt")
-		daddr=$(sed --unbuffered -n 's/.*\(daddr6:\)\([0-9a-f:.]*\).*$/\2/p;q' "$evt")
+		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" $e_type)
+		daddr=$(mptcp_lib_evts_get_info daddr6 "$evt" $e_type)
 	else
-		saddr=$(sed --unbuffered -n 's/.*\(saddr4:\)\([0-9.]*\).*$/\2/p;q' "$evt")
-		daddr=$(sed --unbuffered -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evt")
+		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" $e_type)
+		daddr=$(mptcp_lib_evts_get_info daddr4 "$evt" $e_type)
 	fi
 
 	check_expected "type" "token" "daddr" "dport" "family" "saddr" "locid" "remid"
@@ -630,7 +627,7 @@ test_subflows()
 	kill_wait $listener_pid
 
 	local sport
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from server to client machine
 	:>"$server_evts"
@@ -668,7 +665,7 @@ test_subflows()
 	# Delete the listener from the client ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW6 from server to client machine
 	:>"$server_evts"
@@ -707,7 +704,7 @@ test_subflows()
 	# Delete the listener from the client ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$server_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from server to client machine
 	:>"$server_evts"
@@ -745,7 +742,7 @@ test_subflows()
 	# Delete the listener from the server ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
@@ -784,7 +781,7 @@ test_subflows()
 	# Delete the listener from the server ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW6 from client to server machine
 	:>"$client_evts"
@@ -821,7 +818,7 @@ test_subflows()
 	# Delete the listener from the server ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
@@ -867,7 +864,7 @@ test_subflows_v4_v6_mix()
 	# Delete the listener from the server ns, if one was created
 	kill_wait $listener_pid
 
-	sport=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
@@ -933,18 +930,13 @@ verify_listener_events()
 		print_test "CLOSE_LISTENER $e_saddr:$e_sport"
 	fi
 
-	type=$(grep "type:$e_type," $evt |
-	       sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q')
-	family=$(grep "type:$e_type," $evt |
-		 sed --unbuffered -n 's/.*\(family:\)\([[:digit:]]*\).*$/\2/p;q')
-	sport=$(grep "type:$e_type," $evt |
-		sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
+	type=$(mptcp_lib_evts_get_info type $evt $e_type)
+	family=$(mptcp_lib_evts_get_info family $evt $e_type)
+	sport=$(mptcp_lib_evts_get_info sport $evt $e_type)
 	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(grep "type:$e_type," $evt |
-			sed --unbuffered -n 's/.*\(saddr6:\)\([0-9a-f:.]*\).*$/\2/p;q')
+		saddr=$(mptcp_lib_evts_get_info saddr6 $evt $e_type)
 	else
-		saddr=$(grep "type:$e_type," $evt |
-			sed --unbuffered -n 's/.*\(saddr4:\)\([0-9.]*\).*$/\2/p;q')
+		saddr=$(mptcp_lib_evts_get_info saddr4 $evt $e_type)
 	fi
 
 	check_expected "type" "family" "saddr" "sport"
-- 
2.35.3

[PATCH mptcp-next v3 10/29] selftests: mptcp: add chk_subflows_total helper

[Geliang Tang]

This patch adds a new helper chk_subflows_total(), in it use the newly
added counter mptcpi_subflows_total to get the "correct" amount of
subflows, including the initial one. To be compatible with old 'ss'
version without this counter, get the total subflows using this 'ss'
command:

        ss -ti | grep -c tcp-ulp-mptcp.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 92285ef592fb..3e24ab7bc221 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1862,7 +1862,7 @@ chk_mptcp_info()
 	local cnt2
 	local dump_stats
 
-	print_check "mptcp_info ${info1:0:8}=$exp1:$exp2"
+	print_check "mptcp_info ${info1:0:15}=$exp1:$exp2"
 
 	cnt1=$(ss -N $ns1 -inmHM | mptcp_lib_get_info_value "$info1" "$info1")
 	cnt2=$(ss -N $ns2 -inmHM | mptcp_lib_get_info_value "$info2" "$info2")
@@ -1883,6 +1883,37 @@ chk_mptcp_info()
 	fi
 }
 
+# $1: subflows in ns1 ; $2: subflows in ns2
+# number of all subflows, including the initial subflow.
+chk_subflows_total()
+{
+	local cnt1
+	local cnt2
+	local info="subflows_total"
+
+	if [ $(ss -N $ns1 -inmHM | mptcp_lib_get_info_value $info $info) ]; then
+		chk_mptcp_info $info $1 $info $2
+		return
+	fi
+
+	print_check "$info $1:$2"
+
+	cnt1=$(ss -N $ns1 -ti | grep -c tcp-ulp-mptcp)
+	cnt2=$(ss -N $ns2 -ti | grep -c tcp-ulp-mptcp)
+
+	if [ "$1" != "$cnt1" ] || [ "$2" != "$cnt2" ]; then
+		fail_test "got subflows $cnt1:$cnt2 expected $1:$2"
+		dump_stats=1
+	else
+		print_ok
+	fi
+
+	if [ "$dump_stats" = 1 ]; then
+		ss -N $ns1 -ti
+		ss -N $ns2 -ti
+	fi
+}
+
 chk_link_usage()
 {
 	local ns=$1
@@ -3407,10 +3438,12 @@ userspace_tests()
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
 		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
 		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
 		userspace_pm_rm_sf_addr_ns1 10.0.2.1 10
 		chk_rm_nr 1 1 invert
 		chk_mptcp_info subflows 0 subflows 0
+		chk_subflows_total 1 1
 		kill_events_pids
 		wait $tests_pid
 	fi
@@ -3427,9 +3460,11 @@ userspace_tests()
 		userspace_pm_add_sf 10.0.3.2 20
 		chk_join_nr 1 1 1
 		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
 		userspace_pm_rm_sf_addr_ns2 10.0.3.2 20
 		chk_rm_nr 1 1
 		chk_mptcp_info subflows 0 subflows 0
+		chk_subflows_total 1 1
 		kill_events_pids
 		wait $tests_pid
 	fi
-- 
2.35.3

Re: [PATCH mptcp-next v3 10/29] selftests: mptcp: add chk_subflows_total helper

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a new helper chk_subflows_total(), in it use the newly
> added counter mptcpi_subflows_total to get the "correct" amount of
> subflows, including the initial one. To be compatible with old 'ss'
> version without this counter, get the total subflows using this 'ss'
> command:
> 
>         ss -ti | grep -c tcp-ulp-mptcp.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 37 ++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 92285ef592fb..3e24ab7bc221 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1862,7 +1862,7 @@ chk_mptcp_info()
>  	local cnt2
>  	local dump_stats
>  
> -	print_check "mptcp_info ${info1:0:8}=$exp1:$exp2"
> +	print_check "mptcp_info ${info1:0:15}=$exp1:$exp2"
>  
>  	cnt1=$(ss -N $ns1 -inmHM | mptcp_lib_get_info_value "$info1" "$info1")
>  	cnt2=$(ss -N $ns2 -inmHM | mptcp_lib_get_info_value "$info2" "$info2")
> @@ -1883,6 +1883,37 @@ chk_mptcp_info()
>  	fi
>  }
>  
> +# $1: subflows in ns1 ; $2: subflows in ns2
> +# number of all subflows, including the initial subflow.
> +chk_subflows_total()
> +{
> +	local cnt1
> +	local cnt2
> +	local info="subflows_total"
> +
> +	if [ $(ss -N $ns1 -inmHM | mptcp_lib_get_info_value $info $info) ]; then

Shellcheck just told me it would be better with quotes and '-n' because
you want to check if the output is not empty:

  if [ -n "$(...)" ]

Do not hesitate to use shellcheck, it is quite useful ;-)

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

[PATCH mptcp-next v3 11/29] selftests: mptcp: update userspace pm test helpers

[Geliang Tang]

This patch adds a new argument namespace to userspace_pm_add_addr() and
userspace_pm_add_sf() to make these two helper more versatile.

Add two more versatile helpers for userspace pm remove subflow or address:
userspace_pm_rm_addr() and userspace_pm_rm_sf(). The original test helpers
userspace_pm_rm_sf_addr_ns1() and userspace_pm_rm_sf_addr_ns2() can be
replaced by these new helpers.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 89 +++++++++----------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  2 +
 .../selftests/net/mptcp/userspace_pm.sh       |  1 -
 3 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 3e24ab7bc221..25c2948391f5 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3285,69 +3285,66 @@ fail_tests()
 	fi
 }
 
+# $1: ns ; $2: addr ; $3: id
 userspace_pm_add_addr()
 {
-	local addr=$1
-	local id=$2
+	local evts=$evts_ns1
 	local tk
 
-	tk=$(grep "type:1," "$evts_ns1" |
-	     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
-	ip netns exec $ns1 ./pm_nl_ctl ann $addr token $tk id $id
+	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	tk=$(mptcp_lib_evts_get_info token "$evts")
+
+	ip netns exec $1 ./pm_nl_ctl ann $2 token $tk id $3
 	sleep 1
 }
 
-userspace_pm_rm_sf_addr_ns1()
+# $1: ns ; $2: id
+userspace_pm_rm_addr()
 {
-	local addr=$1
-	local id=$2
-	local tk sp da dp
-
-	tk=$(grep "type:1," "$evts_ns1" |
-	     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
-	sp=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
-	da=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(daddr6:\)\([0-9a-f:.]*\).*$/\2/p;q')
-	dp=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q')
-	ip netns exec $ns1 ./pm_nl_ctl rem token $tk id $id
-	ip netns exec $ns1 ./pm_nl_ctl dsf lip "::ffff:$addr" \
-				lport $sp rip $da rport $dp token $tk
-	wait_rm_addr $ns1 1
-	wait_rm_sf $ns1 1
+	local evts=$evts_ns1
+	local tk
+
+	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	tk=$(mptcp_lib_evts_get_info token "$evts")
+
+	ip netns exec $1 ./pm_nl_ctl rem token $tk id $2
+	wait_rm_addr $1 1
 }
 
+# $1: ns ; $2: addr ; $3: id
 userspace_pm_add_sf()
 {
-	local addr=$1
-	local id=$2
+	local evts=$evts_ns1
 	local tk da dp
 
-	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	da=$(sed -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evts_ns2")
-	dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	ip netns exec $ns2 ./pm_nl_ctl csf lip $addr lid $id \
+	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	tk=$(mptcp_lib_evts_get_info token "$evts")
+	da=$(mptcp_lib_evts_get_info daddr4 "$evts")
+	dp=$(mptcp_lib_evts_get_info dport "$evts")
+
+	ip netns exec $1 ./pm_nl_ctl csf lip $2 lid $3 \
 				rip $da rport $dp token $tk
 	sleep 1
 }
 
-userspace_pm_rm_sf_addr_ns2()
+# $1: ns ; $2: addr $3: event type
+userspace_pm_rm_sf()
 {
-	local addr=$1
-	local id=$2
+	local evts=$evts_ns1
+	local t=${3:-1}
+	local ip=4
 	local tk da dp sp
 
-	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	da=$(sed -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evts_ns2")
-	dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	sp=$(grep "type:10" "$evts_ns2" |
-	     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
-	ip netns exec $ns2 ./pm_nl_ctl rem token $tk id $id
-	ip netns exec $ns2 ./pm_nl_ctl dsf lip $addr lport $sp \
+	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	if is_v6 $2; then ip=6; fi
+	tk=$(mptcp_lib_evts_get_info token "$evts")
+	da=$(mptcp_lib_evts_get_info "daddr$ip" "$evts" $t)
+	dp=$(mptcp_lib_evts_get_info dport "$evts" $t)
+	sp=$(mptcp_lib_evts_get_info sport "$evts" $t)
+
+	ip netns exec $1 ./pm_nl_ctl dsf lip $2 lport $sp \
 				rip $da rport $dp token $tk
-	wait_rm_addr $ns2 1
-	wait_rm_sf $ns2 1
+	wait_rm_sf $1 1
 }
 
 userspace_tests()
@@ -3434,13 +3431,14 @@ userspace_tests()
 			run_tests $ns1 $ns2 10.0.1.1 &
 		local tests_pid=$!
 		wait_mpj $ns1
-		userspace_pm_add_addr 10.0.2.1 10
+		userspace_pm_add_addr $ns1 10.0.2.1 10
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 2 2
 		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
-		userspace_pm_rm_sf_addr_ns1 10.0.2.1 10
+		userspace_pm_rm_addr $ns1 10
+		userspace_pm_rm_sf $ns1 "::ffff:10.0.2.1" $SUB_ESTABLISHED
 		chk_rm_nr 1 1 invert
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
@@ -3457,11 +3455,12 @@ userspace_tests()
 			run_tests $ns1 $ns2 10.0.1.1 &
 		local tests_pid=$!
 		wait_mpj $ns2
-		userspace_pm_add_sf 10.0.3.2 20
+		userspace_pm_add_sf $ns2 10.0.3.2 20
 		chk_join_nr 1 1 1
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 2 2
-		userspace_pm_rm_sf_addr_ns2 10.0.3.2 20
+		userspace_pm_rm_addr $ns2 20
+		userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED
 		chk_rm_nr 1 1
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index def35395a254..bb95dd967eb3 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -8,6 +8,8 @@ readonly KSFT_SKIP=4
 # shellcheck disable=SC2155 # declare and assign separately
 readonly KSFT_TEST=$(basename "${0}" | sed 's/\.sh$//g')
 
+SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
+
 MPTCP_LIB_SUBTESTS=()
 
 # only if supported (or forced) and not disabled, see no-color.org
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 2413059a42e5..283c62deb628 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -25,7 +25,6 @@ fi
 
 ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
 REMOVED=7          # MPTCP_EVENT_REMOVED
-SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
 SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
 LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
 LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
-- 
2.35.3

Re: [PATCH mptcp-next v3 11/29] selftests: mptcp: update userspace pm test helpers

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a new argument namespace to userspace_pm_add_addr() and
> userspace_pm_add_sf() to make these two helper more versatile.
> 
> Add two more versatile helpers for userspace pm remove subflow or address:
> userspace_pm_rm_addr() and userspace_pm_rm_sf(). The original test helpers
> userspace_pm_rm_sf_addr_ns1() and userspace_pm_rm_sf_addr_ns2() can be
> replaced by these new helpers.

Thank you for cleaning this!

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index def35395a254..bb95dd967eb3 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -8,6 +8,8 @@ readonly KSFT_SKIP=4
>  # shellcheck disable=SC2155 # declare and assign separately
>  readonly KSFT_TEST=$(basename "${0}" | sed 's/\.sh$//g')
>  
> +SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED

Because variables are "global" by default in Bash, it might be good to
prefix this variable with MPTCP_LIB_ (or "MPTCP_EVENT_SUB_ESTABLISHED")
to avoid any accidental conflicts when using "too generic names". But
then the name will be long and you will have to rename it in many places
in userspace_pm.sh and that's not nice I think...

If we want to keep it short, maybe easier to declare SUB_ESTABLISHED in
both userspace_pm.sh and mptcp_join.sh. It might be better like that
because otherwise it looks strange to only declare this one in the lib
and not the other ones (ANNOUNCED, REMOVED, etc.).

If we only need to copy it in mptcp_join.sh and userspace_pm.sh, I can
do that when applying the patches.

Cheers,
Matt

> +
>  MPTCP_LIB_SUBTESTS=()
>  
>  # only if supported (or forced) and not disabled, see no-color.org
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 2413059a42e5..283c62deb628 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -25,7 +25,6 @@ fi
>  
>  ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
>  REMOVED=7          # MPTCP_EVENT_REMOVED
> -SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
>  SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
>  LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
>  LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-next v3 12/29] selftests: mptcp: userspace pm remove id 0 subflow

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 subflow. Use
userspace_pm_add_sf() to add a subflow, and pass initial ip address to
userspace_pm_rm_sf() to remove id 0 subflow.

When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
false, then tcp_disconnect is invoked. This will send a MP_RST to close
a subflow on the peer too. So chk_rst_nr() is added in this test, and
chk_all_subflows after closing the initial subflow is '1 1', not '2 1'.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 25c2948391f5..ca75d2c6d415 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3467,6 +3467,28 @@ 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 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		userspace_pm_add_sf $ns2 10.0.3.2 20
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		userspace_pm_rm_sf $ns2 10.0.1.2
+		chk_rm_nr 0 1
+		chk_rst_nr 1 1 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v3 12/29] selftests: mptcp: userspace pm remove id 0 subflow

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a selftest for userpsace PM to remove id 0 subflow. Use
> userspace_pm_add_sf() to add a subflow, and pass initial ip address to
> userspace_pm_rm_sf() to remove id 0 subflow.
> 
> When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
> false, then tcp_disconnect is invoked. This will send a MP_RST to close
> a subflow on the peer too.

I don't think we should have a RST (with MP_TCPRST) when we close the
initial subflow, no? We don't have that when we remove subflows with
other IDs.

I guess we are not handling this case properly on the kernel side. We
should close the initial subflow normally (with FIN) like we would do
with any other subflow IDs (and here, we cannot fully destroy it and we
still need to keep a valid ref for msk->first).

> So chk_rst_nr() is added in this test, and
> chk_all_subflows after closing the initial subflow is '1 1', not '2 1'.

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

Re: [PATCH mptcp-next v3 12/29] selftests: mptcp: userspace pm remove id 0 subflow

[Geliang Tang]

Hi Matt,

On Thu, Sep 28, 2023 at 10:58:33PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 25/09/2023 10:41, Geliang Tang wrote:
> > This patch adds a selftest for userpsace PM to remove id 0 subflow. Use
> > userspace_pm_add_sf() to add a subflow, and pass initial ip address to
> > userspace_pm_rm_sf() to remove id 0 subflow.
> > 
> > When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
> > false, then tcp_disconnect is invoked. This will send a MP_RST to close
> > a subflow on the peer too.
> 
> I don't think we should have a RST (with MP_TCPRST) when we close the
> initial subflow, no? We don't have that when we remove subflows with
> other IDs.

We do have a RST in "in-kernel remove id 0 subflow and address" tests,
we can test them like this:

        # remove id 0 subflow
        if reset "remove id 0 subflow"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_set_limits $ns2 0 1
                pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
                addr_nr_ns2=-9 speed=slow \
                        run_tests $ns1 $ns2 10.0.1.1
                chk_join_nr 1 1 1
                chk_rm_nr 1 1
+               chk_rst_nr 1 1
        fi

        # remove id 0 address
        if reset "remove id 0 address"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
                pm_nl_set_limits $ns2 1 1
                addr_nr_ns1=-9 speed=slow \
                        run_tests $ns1 $ns2 10.0.1.1
                chk_join_nr 1 1 1
                chk_add_nr 1 1
                chk_rm_nr 1 1 invert
+               chk_rst_nr 1 1 invert
        fi

Maybe it's OK with the RSTs.

Thanks,
-Geliang

> 
> I guess we are not handling this case properly on the kernel side. We
> should close the initial subflow normally (with FIN) like we would do
> with any other subflow IDs (and here, we cannot fully destroy it and we
> still need to keep a valid ref for msk->first).
> 
> > So chk_rst_nr() is added in this test, and
> > chk_all_subflows after closing the initial subflow is '1 1', not '2 1'.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v3 12/29] selftests: mptcp: userspace pm remove id 0 subflow

[Matthieu Baerts]

Hi Geliang,

On 05/10/2023 10:32, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, Sep 28, 2023 at 10:58:33PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 25/09/2023 10:41, Geliang Tang wrote:
>>> This patch adds a selftest for userpsace PM to remove id 0 subflow. Use
>>> userspace_pm_add_sf() to add a subflow, and pass initial ip address to
>>> userspace_pm_rm_sf() to remove id 0 subflow.
>>>
>>> When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
>>> false, then tcp_disconnect is invoked. This will send a MP_RST to close
>>> a subflow on the peer too.
>>
>> I don't think we should have a RST (with MP_TCPRST) when we close the
>> initial subflow, no? We don't have that when we remove subflows with
>> other IDs.
> 
> We do have a RST in "in-kernel remove id 0 subflow and address" tests,
> we can test them like this:

Thank you for having checked!

Do we have a RST if we remove another subflow than the initial one or if
we send a RM_ADDR for another id than 0? We should have the same
behaviour as with the other subflows/ID, no exception for the initial
subflow/address ID 0.

Is the RST sent by the host which received the RM_ADDR?

> 
>         # remove id 0 subflow
>         if reset "remove id 0 subflow"; then

(I think we should change the name of this test, it is confusing because
there is no 'id 0 subflow', an ID is linked to an address. Maybe:
"remove initial subflow")

>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_set_limits $ns2 0 1
>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>                 addr_nr_ns2=-9 speed=slow \

Just to be sure (because 'addr_nr_ns2=-9' is really not clear): it will
delete the endpoint linked to the address used by the initial subflow,
right? And this should stop the subflow (TCP FIN) and send a RM_ADDR, right?

>                         run_tests $ns1 $ns2 10.0.1.1
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 1 1
> +               chk_rst_nr 1 1
>         fi
> 
>         # remove id 0 address
>         if reset "remove id 0 address"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>                 pm_nl_set_limits $ns2 1 1
>                 addr_nr_ns1=-9 speed=slow \

But then here, the behaviour should be the same as the previous test,
no? Why is it not?

>                         run_tests $ns1 $ns2 10.0.1.1
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
> +               chk_rst_nr 1 1 invert
>         fi
> 
> Maybe it's OK with the RSTs.

As long as the behaviour is the same as when removing another subflow
than the initial one or sending a RM_ADDR for another ID then 0.

Cheers,
Matt

[PATCH mptcp-next v3 13/29] mptcp: userspace pm 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.

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.

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 04e26eb01ba8..6b8083650bc1 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -315,12 +315,6 @@ int mptcp_pm_nl_subflow_create_doit(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

[PATCH mptcp-next v3 14/29] selftests: mptcp: userspace pm create id 0 subflow

[Geliang Tang]

This patch adds a selftest to create id 0 subflow. Pass id 0 to the
helper userspace_pm_add_sf() to create id 0 subflow. chk_mptcp_info
shows one subflow but chk_subflows_total shows two subflows in each
namespace.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ca75d2c6d415..22aa69db87ab 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3468,6 +3468,25 @@ userspace_tests()
 		wait $tests_pid
 	fi
 
+	# userspace pm create id 0 subflow
+	if reset_with_events "userspace pm create 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 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		chk_mptcp_info subflows 0 subflows 0
+		chk_subflows_total 1 1
+		userspace_pm_add_sf $ns2 10.0.3.2 0
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		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
-- 
2.35.3

[PATCH mptcp-next v3 15/29] mptcp: userspace pm remove id 0 address

[Geliang Tang]

This patch adds the ability to send RM_ADDR for local ID 0. Check
whether id 0 address is removed, if not, put id 0 into a removing
list, pass it to mptcp_pm_remove_addr() to remove id 0 address.

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

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6b8083650bc1..8d97cf475cac 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -211,6 +211,38 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
+						  struct genl_info *info)
+{
+	struct mptcp_rm_list list = { .nr = 0 };
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	bool has_id_0 = false;
+	int err = -EINVAL;
+
+	lock_sock(sk);
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_for_each_subflow(msk, subflow) {
+		if (subflow->remote_id == 0) {
+			has_id_0 = true;
+			break;
+		}
+	}
+	if (!has_id_0) {
+		GENL_SET_ERR_MSG(info, "address with id 0 not found");
+		goto out;
+	}
+
+	list.ids[list.nr++] = 0;
+	mptcp_pm_remove_addr(msk, &list);
+	err = 0;
+out:
+	spin_unlock_bh(&msk->pm.lock);
+	release_sock(sk);
+	sock_put(sk);
+	return err;
+}
+
 int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
@@ -245,6 +277,9 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (id_val == 0)
+		return mptcp_userspace_remove_id_zero_address(msk, info);
+
 	lock_sock(sk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
-- 
2.35.3

Re: [PATCH mptcp-next v3 15/29] mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds the ability to send RM_ADDR for local ID 0. Check
> whether id 0 address is removed, if not, put id 0 into a removing
> list, pass it to mptcp_pm_remove_addr() to remove id 0 address.
> 
> There is no reason not to allow the userspace to remove the initial
> address (ID 0). This special case was not taken into account not
> letting the userspace to delete all addresses as announced.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6b8083650bc1..8d97cf475cac 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -211,6 +211,38 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>  
> +static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
> +						  struct genl_info *info)
> +{
> +	struct mptcp_rm_list list = { .nr = 0 };
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
> +	bool has_id_0 = false;
> +	int err = -EINVAL;
> +
> +	lock_sock(sk);
> +	spin_lock_bh(&msk->pm.lock);

Why do you need to lock the 'pm' structure? mptcp_pm_remove_addr() will
modify the 'pm' structure but it will also do the lock, no?

> +	mptcp_for_each_subflow(msk, subflow) {
> +		if (subflow->remote_id == 0) {

I only noticed that now but you need to look at local_id, not remote_id:
when we send a RM_ADDR, we announce that a previously announced (local)
address ID is now invalid → so we need to check for local IDs here.

> +			has_id_0 = true;
> +			break;
> +		}
> +	}
> +	if (!has_id_0) {
> +		GENL_SET_ERR_MSG(info, "address with id 0 not found");
> +		goto out;
> +	}
> +
> +	list.ids[list.nr++] = 0;
> +	mptcp_pm_remove_addr(msk, &list);
> +	err = 0;
> +out:
> +	spin_unlock_bh(&msk->pm.lock);
> +	release_sock(sk);
> +	sock_put(sk);

It looks strange to me to do a 'put' here because we didn't "hold" the
socket here in this function.

Should we not do the 'sock_put(sk)' in mptcp_pm_nl_remove_doit() instead?

> +	return err;
> +}
> +
>  int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> @@ -245,6 +277,9 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto remove_err;
>  	}
>  
> +	if (id_val == 0)
> +		return mptcp_userspace_remove_id_zero_address(msk, info);

Linked to what I said above: maybe here it would be better to have:

  err = mptcp_userspace_remove_id_zero_address(msk, info);
  goto out; // the sock_put() will be done there

(rename 'remove_err' to 'out' as there is potentially no errors here)

WDYT?


> +
>  	lock_sock(sk);
>  
>  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {

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

Re: [PATCH mptcp-next v3 15/29] mptcp: userspace pm remove id 0 address

[Geliang Tang]

On Thu, Sep 28, 2023 at 11:00:55PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 25/09/2023 10:41, Geliang Tang wrote:
> > This patch adds the ability to send RM_ADDR for local ID 0. Check
> > whether id 0 address is removed, if not, put id 0 into a removing
> > list, pass it to mptcp_pm_remove_addr() to remove id 0 address.
> > 
> > There is no reason not to allow the userspace to remove the initial
> > address (ID 0). This special case was not taken into account not
> > letting the userspace to delete all addresses as announced.
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> > Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/pm_userspace.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 6b8083650bc1..8d97cf475cac 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -211,6 +211,38 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
> >  	return err;
> >  }
> >  
> > +static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
> > +						  struct genl_info *info)
> > +{
> > +	struct mptcp_rm_list list = { .nr = 0 };
> > +	struct mptcp_subflow_context *subflow;
> > +	struct sock *sk = (struct sock *)msk;
> > +	bool has_id_0 = false;
> > +	int err = -EINVAL;
> > +
> > +	lock_sock(sk);
> > +	spin_lock_bh(&msk->pm.lock);
> 
> Why do you need to lock the 'pm' structure? mptcp_pm_remove_addr() will
> modify the 'pm' structure but it will also do the lock, no?

mptcp_pm_remove_addrs will do the lock but mptcp_pm_remove_addr does need
the locks.

Thanks,
-Geliang

> 
> > +	mptcp_for_each_subflow(msk, subflow) {
> > +		if (subflow->remote_id == 0) {
> 
> I only noticed that now but you need to look at local_id, not remote_id:
> when we send a RM_ADDR, we announce that a previously announced (local)
> address ID is now invalid → so we need to check for local IDs here.
> 
> > +			has_id_0 = true;
> > +			break;
> > +		}
> > +	}
> > +	if (!has_id_0) {
> > +		GENL_SET_ERR_MSG(info, "address with id 0 not found");
> > +		goto out;
> > +	}
> > +
> > +	list.ids[list.nr++] = 0;
> > +	mptcp_pm_remove_addr(msk, &list);
> > +	err = 0;
> > +out:
> > +	spin_unlock_bh(&msk->pm.lock);
> > +	release_sock(sk);
> > +	sock_put(sk);
> 
> It looks strange to me to do a 'put' here because we didn't "hold" the
> socket here in this function.
> 
> Should we not do the 'sock_put(sk)' in mptcp_pm_nl_remove_doit() instead?
> 
> > +	return err;
> > +}
> > +
> >  int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
> >  {
> >  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> > @@ -245,6 +277,9 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
> >  		goto remove_err;
> >  	}
> >  
> > +	if (id_val == 0)
> > +		return mptcp_userspace_remove_id_zero_address(msk, info);
> 
> Linked to what I said above: maybe here it would be better to have:
> 
>   err = mptcp_userspace_remove_id_zero_address(msk, info);
>   goto out; // the sock_put() will be done there
> 
> (rename 'remove_err' to 'out' as there is potentially no errors here)
> 
> WDYT?
> 
> 
> > +
> >  	lock_sock(sk);
> >  
> >  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v3 15/29] mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 05/10/2023 10:35, Geliang Tang wrote:
> On Thu, Sep 28, 2023 at 11:00:55PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 25/09/2023 10:41, Geliang Tang wrote:
>>> This patch adds the ability to send RM_ADDR for local ID 0. Check
>>> whether id 0 address is removed, if not, put id 0 into a removing
>>> list, pass it to mptcp_pm_remove_addr() to remove id 0 address.
>>>
>>> There is no reason not to allow the userspace to remove the initial
>>> address (ID 0). This special case was not taken into account not
>>> letting the userspace to delete all addresses as announced.
>>>
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
>>> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/pm_userspace.c | 35 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>> index 6b8083650bc1..8d97cf475cac 100644
>>> --- a/net/mptcp/pm_userspace.c
>>> +++ b/net/mptcp/pm_userspace.c
>>> @@ -211,6 +211,38 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>>>  	return err;
>>>  }
>>>  
>>> +static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
>>> +						  struct genl_info *info)
>>> +{
>>> +	struct mptcp_rm_list list = { .nr = 0 };
>>> +	struct mptcp_subflow_context *subflow;
>>> +	struct sock *sk = (struct sock *)msk;
>>> +	bool has_id_0 = false;
>>> +	int err = -EINVAL;
>>> +
>>> +	lock_sock(sk);
>>> +	spin_lock_bh(&msk->pm.lock);
>>
>> Why do you need to lock the 'pm' structure? mptcp_pm_remove_addr() will
>> modify the 'pm' structure but it will also do the lock, no?
> 
> mptcp_pm_remove_addrs will do the lock but mptcp_pm_remove_addr does need
> the locks.

Ah yes, the names are confusing :)

Why not moving the PM lock just around mptcp_pm_remove_addr() then?

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

[PATCH mptcp-next v3 16/29] selftests: mptcp: userspace pm remove id 0 address

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 address.
Use userspace_pm_add_addr() helper to add a id 10 address, then use
userspace_pm_rm_addr() helper to remove id 0 address.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 22aa69db87ab..aac50ef86785 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3508,6 +3508,30 @@ userspace_tests()
 		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 $ns1
+		pm_nl_set_limits $ns2 1 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns1
+		userspace_pm_add_addr $ns1 10.0.2.1 10
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
+		userspace_pm_rm_addr $ns1 0
+		chk_rm_nr 1 0 invert
+		chk_rst_nr 1 1 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v3 16/29] selftests: mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a selftest for userpsace PM to remove id 0 address.
> Use userspace_pm_add_addr() helper to add a id 10 address, then use
> userspace_pm_rm_addr() helper to remove id 0 address.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 22aa69db87ab..aac50ef86785 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3508,6 +3508,30 @@ userspace_tests()
>  		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 $ns1
> +		pm_nl_set_limits $ns2 1 1
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns1
> +		userspace_pm_add_addr $ns1 10.0.2.1 10
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2
> +		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
> +		userspace_pm_rm_addr $ns1 0
> +		chk_rm_nr 1 0 invert
> +		chk_rst_nr 1 1 invert

Why do we have RST here?

Is it because a RM_ADDR is sent from ns1 and on ns2, the kernel reacts
by sending an RST+MP_TCPRST? (or are these RST sent from ns1 just after
the RM_ADDR?)

I don't think the kernel should send a RST: sending a RM_ADDR for ID 0
should not be different than sending it for any other IDs, no?

> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 1 1
> +		kill_events_pids
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

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

[PATCH mptcp-next v3 17/29] mptcp: add userspace_pm_get_entry helper

[Geliang Tang]

This patch adds a new helper mptcp_userspace_pm_get_entry() to find out
the address entry on the userspace_pm_local_addr_list through the given
address. Use this helper in mptcp_userspace_pm_delete_local_addr() and
mptcp_nl_cmd_sf_destroy().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 43 ++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 8d97cf475cac..30f4dd074a70 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -80,6 +80,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 	return ret;
 }
 
+static struct mptcp_pm_addr_entry *mptcp_userspace_pm_get_entry(struct mptcp_sock *msk,
+								struct mptcp_addr_info *addr)
+{
+	struct mptcp_pm_addr_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+		if (mptcp_addresses_equal(&entry->addr, addr, false))
+			return entry;
+	}
+
+	return NULL;
+}
+
 /* If the subflow is closed from the other peer (not via a
  * subflow destroy command then), we want to keep the entry
  * not to assign the same ID to another address and to be
@@ -88,21 +101,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 						struct mptcp_pm_addr_entry *addr)
 {
-	struct mptcp_pm_addr_entry *entry, *tmp;
+	struct mptcp_pm_addr_entry *entry;
 
-	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
-			/* TODO: a refcount is needed because the entry can
-			 * be used multiple times (e.g. fullmesh mode).
-			 */
-			list_del_rcu(&entry->list);
-			kfree(entry);
-			msk->pm.local_addr_used--;
-			return 0;
-		}
-	}
+	entry = mptcp_userspace_pm_get_entry(msk, &addr->addr);
+	if (!entry)
+		return -EINVAL;
 
-	return -EINVAL;
+	/* TODO: a refcount is needed because the entry can
+	 * be used multiple times (e.g. fullmesh mode).
+	 */
+	list_del_rcu(&entry->list);
+	kfree(entry);
+	msk->pm.local_addr_used--;
+	return 0;
 }
 
 int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
@@ -495,10 +506,12 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
-		struct mptcp_pm_addr_entry entry = { .addr = addr_l };
+		struct mptcp_pm_addr_entry *entry;
 
 		spin_lock_bh(&msk->pm.lock);
-		mptcp_userspace_pm_delete_local_addr(msk, &entry);
+		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);
+		if (entry)
+			mptcp_userspace_pm_delete_local_addr(msk, entry);
 		spin_unlock_bh(&msk->pm.lock);
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
-- 
2.35.3

Re: [PATCH mptcp-next v3 17/29] mptcp: add userspace_pm_get_entry helper

[Matthieu Baerts]

Hi Geliang,

Thank you for looking at the issue 403!

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a new helper mptcp_userspace_pm_get_entry() to find out
> the address entry on the userspace_pm_local_addr_list through the given
> address. Use this helper in mptcp_userspace_pm_delete_local_addr() and
> mptcp_nl_cmd_sf_destroy().
Please always explain why this is needed, e.g. for this case:

  This will be reused in the following commits.

(also, please see my comment here below and on the next commit: maybe
you don't need this helper? → Except to clarify the code?)

> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 43 ++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 8d97cf475cac..30f4dd074a70 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -80,6 +80,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  	return ret;
>  }
>  
> +static struct mptcp_pm_addr_entry *mptcp_userspace_pm_get_entry(struct mptcp_sock *msk,
> +								struct mptcp_addr_info *addr)
> +{
> +	struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, addr, false))
> +			return entry;
> +	}
> +
> +	return NULL;
> +}
> +
>  /* If the subflow is closed from the other peer (not via a
>   * subflow destroy command then), we want to keep the entry
>   * not to assign the same ID to another address and to be
> @@ -88,21 +101,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  						struct mptcp_pm_addr_entry *addr)
>  {
> -	struct mptcp_pm_addr_entry *entry, *tmp;
> +	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> -			/* TODO: a refcount is needed because the entry can
> -			 * be used multiple times (e.g. fullmesh mode).
> -			 */
> -			list_del_rcu(&entry->list);
> -			kfree(entry);
> -			msk->pm.local_addr_used--;
> -			return 0;
> -		}
> -	}
> +	entry = mptcp_userspace_pm_get_entry(msk, &addr->addr);
> +	if (!entry)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	/* TODO: a refcount is needed because the entry can
> +	 * be used multiple times (e.g. fullmesh mode).
> +	 */
> +	list_del_rcu(&entry->list);
> +	kfree(entry);
> +	msk->pm.local_addr_used--;
> +	return 0;
>  }
>  
>  int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> @@ -495,10 +506,12 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
>  	if (ssk) {
>  		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> -		struct mptcp_pm_addr_entry entry = { .addr = addr_l };
> +		struct mptcp_pm_addr_entry *entry;
>  
>  		spin_lock_bh(&msk->pm.lock);
> -		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> +		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);

Why are you calling this function here? ... (see below)

> +		if (entry)
> +			mptcp_userspace_pm_delete_local_addr(msk, entry);

Because this function will also call mptcp_userspace_pm_get_entry().

>  		spin_unlock_bh(&msk->pm.lock);
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);

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

[PATCH mptcp-next v3 18/29] mptcp: add userspace pm addr entry refcount

[Geliang Tang]

This patch adds userspace PM address entry refcount. Add a new filed
'refcont' in struct mptcp_pm_addr_entry, inited to 1.

Increase this counter in mptcp_nl_cmd_sf_create(), and decrease it in
mptcp_userspace_pm_delete_local_addr() according the subflows value.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 21 +++++++++++++++------
 net/mptcp/protocol.h     |  2 ++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 30f4dd074a70..8efca1602e11 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -70,6 +70,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 							1);
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
 		msk->pm.local_addr_used++;
+		refcount_set(&e->refcnt, 1);
 		ret = e->addr.id;
 	} else if (match) {
 		ret = entry->addr.id;
@@ -107,9 +108,12 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 	if (!entry)
 		return -EINVAL;
 
-	/* TODO: a refcount is needed because the entry can
-	 * be used multiple times (e.g. fullmesh mode).
-	 */
+	if (!refcount_dec_not_one(&entry->refcnt)) {
+		pr_debug("userspace refcount error: refcnt=%d",
+			 refcount_read(&entry->refcnt));
+		return -EINVAL;
+	}
+
 	list_del_rcu(&entry->list);
 	kfree(entry);
 	msk->pm.local_addr_used--;
@@ -387,10 +391,15 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	release_sock(sk);
 
 	spin_lock_bh(&msk->pm.lock);
-	if (err)
+	if (err) {
 		mptcp_userspace_pm_delete_local_addr(msk, &local);
-	else
-		msk->pm.subflows++;
+	} else {
+		struct mptcp_pm_addr_entry *entry;
+
+		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);
+		if (entry && refcount_inc_not_zero(&entry->refcnt))
+			msk->pm.subflows++;
+	}
 	spin_unlock_bh(&msk->pm.lock);
 
  create_err:
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6508179e94a6..a71b64565e04 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -8,6 +8,7 @@
 #define __MPTCP_PROTOCOL_H
 
 #include <linux/random.h>
+#include <linux/refcount.h>
 #include <net/tcp.h>
 #include <net/inet_connection_sock.h>
 #include <uapi/linux/mptcp.h>
@@ -244,6 +245,7 @@ struct mptcp_pm_addr_entry {
 	u8			flags;
 	int			ifindex;
 	struct socket		*lsk;
+	refcount_t		refcnt;
 };
 
 struct mptcp_data_frag {
-- 
2.35.3

Re: [PATCH mptcp-next v3 18/29] mptcp: add userspace pm addr entry refcount

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds userspace PM address entry refcount. Add a new filed
> 'refcont' in struct mptcp_pm_addr_entry, inited to 1.

Small nits: s/refcont/refcnt/  and s/inited/initiated/

(same comment for the next patch)

> Increase this counter in mptcp_nl_cmd_sf_create(), and decrease it in
> mptcp_userspace_pm_delete_local_addr() according the subflows value.

Please *always* explain why this commit is needed: why do we need a
refcount per address entry? Feel free to look at the ticket 403 for
inspirations.

(same comment for the next patch)

Also, please add the Closes tag:

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

Another thing: should we consider this as a bug-fix? I think we can
because without this modification, we were not able to send a RM_ADDR if
another subflow using the same local address has been removed before. It
should not be a too annoying issue but if we consider it as an issue,
this should target 'mptcp-net' and have a Fixes tag:

Fixes: 24430f8bf516 ("mptcp: add address into userspace pm list")

One last thing: do you mind adding a new test to cover this case please?
e.g.

- the client creates an MPTCP connection: A <-> A
- it asks the userspace PM to add 2 subflows using the same source IP
address: B <-> B ; B <-> C
- it deletes one subflow: B <-> B
- it sends a RM_ADDR for B

Before the patch, it should fail: the kernel has removed the
corresponding entry for the local address from the list when removing
the subflow while another subflow was using the same local address.
After the patch, it should succeed.

Also, trying to send yet another RM_ADDR for B after the previous one
should result in an expected error. (it is possible we don't handle that
correctly)

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 21 +++++++++++++++------
>  net/mptcp/protocol.h     |  2 ++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 30f4dd074a70..8efca1602e11 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -70,6 +70,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  							1);
>  		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
>  		msk->pm.local_addr_used++;
> +		refcount_set(&e->refcnt, 1);
>  		ret = e->addr.id;
>  	} else if (match) {
>  		ret = entry->addr.id;

Should you not increment the refcount here if there is a match?

> @@ -107,9 +108,12 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  	if (!entry)
>  		return -EINVAL;
>  
> -	/* TODO: a refcount is needed because the entry can
> -	 * be used multiple times (e.g. fullmesh mode).
> -	 */
> +	if (!refcount_dec_not_one(&entry->refcnt)) {
> +		pr_debug("userspace refcount error: refcnt=%d",
> +			 refcount_read(&entry->refcnt));
> +		return -EINVAL;

I'm not sure to understand why you treat that as an error.

Should you not simply use refcount_dec_and_test()? If after the
decrement, the counter is not 0, there is nothing to do: the entry is
still being used by another subflow, that's fine. You can keep a
pr_debug() but please do not mention 'error' and do not return a
negative value, no?

> +	}
> +
>  	list_del_rcu(&entry->list);
>  	kfree(entry);
>  	msk->pm.local_addr_used--;
> @@ -387,10 +391,15 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
>  	release_sock(sk);
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	if (err)
> +	if (err) {
>  		mptcp_userspace_pm_delete_local_addr(msk, &local);
> -	else
> -		msk->pm.subflows++;
> +	} else {
> +		struct mptcp_pm_addr_entry *entry;
> +
> +		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);
> +		if (entry && refcount_inc_not_zero(&entry->refcnt))

I don't think you need to change the code here: before creating the
subflow, 'mptcp_userspace_pm_append_new_local_addr()' has been called
which has initialised or incremented the refcount. Then it cannot be 0
and it doesn't need to be incremented, no?

> +			msk->pm.subflows++;
> +	}
>  	spin_unlock_bh(&msk->pm.lock);
>  
>   create_err:

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

Re: [PATCH mptcp-next v3 18/29] mptcp: add userspace pm addr entry refcount

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds userspace PM address entry refcount. Add a new filed
> 'refcont' in struct mptcp_pm_addr_entry, inited to 1.
> 
> Increase this counter in mptcp_nl_cmd_sf_create(), and decrease it in
> mptcp_userspace_pm_delete_local_addr() according the subflows value.

(...)

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 6508179e94a6..a71b64565e04 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -8,6 +8,7 @@
>  #define __MPTCP_PROTOCOL_H
>  
>  #include <linux/random.h>
> +#include <linux/refcount.h>
>  #include <net/tcp.h>
>  #include <net/inet_connection_sock.h>
>  #include <uapi/linux/mptcp.h>
> @@ -244,6 +245,7 @@ struct mptcp_pm_addr_entry {
>  	u8			flags;
>  	int			ifindex;
>  	struct socket		*lsk;
> +	refcount_t		refcnt;

Just a small idea, I didn't check: if only the userspace PM needs this
refcount, maybe it is possible to use an union? e.g. some fields are
maybe only needed for the userspace PM (refcnt?) and some others only
for the Netlink PM (flags?)?

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

[PATCH mptcp-next v3 19/29] mptcp: add netlink pm addr entry refcount

[Geliang Tang]

This patch adds netlink PM address entry refcount. Init 'refcont' of
every address entry to 1. And add a new filed 'subflows' in struct
mptcp_pm_addr_entry, inited to 0, to store how many subflows have
been established on this address entry.

Increase both values in mptcp_pm_create_subflow_or_signal_addr() and
fill_local_addresses_vec(), and decrease the counter 'refcont' in
__mptcp_pm_release_addr_entry() according its 'subflows' value.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
 net/mptcp/protocol.h   |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 1529ec358815..7d1a4922e931 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -603,8 +603,12 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 			continue;
 
 		spin_unlock_bh(&msk->pm.lock);
-		for (i = 0; i < nr; i++)
-			__mptcp_subflow_connect(sk, &local->addr, &addrs[i]);
+		for (i = 0; i < nr; i++) {
+			if (refcount_inc_not_zero(&local->refcnt)) {
+				__mptcp_subflow_connect(sk, &local->addr, &addrs[i]);
+				local->subflows++;
+			}
+		}
 		spin_lock_bh(&msk->pm.lock);
 	}
 	mptcp_pm_nl_check_work_pending(msk);
@@ -644,9 +648,11 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 		if (!mptcp_pm_addr_families_match(sk, &entry->addr, remote))
 			continue;
 
-		if (msk->pm.subflows < subflows_max) {
+		if (msk->pm.subflows < subflows_max &&
+		    refcount_inc_not_zero(&entry->refcnt)) {
 			msk->pm.subflows++;
 			addrs[i++] = entry->addr;
+			entry->subflows++;
 		}
 	}
 	rcu_read_unlock();
@@ -895,6 +901,16 @@ static bool address_use_port(struct mptcp_pm_addr_entry *entry)
 /* caller must ensure the RCU grace period is already elapsed */
 static void __mptcp_pm_release_addr_entry(struct mptcp_pm_addr_entry *entry)
 {
+	int i;
+
+	for (i = 0; i < entry->subflows; i++) {
+		if (!refcount_dec_not_one(&entry->refcnt)) {
+			pr_debug("netlink refcount error: refcnt=%d, subflows=%d",
+				 refcount_read(&entry->refcnt), entry->subflows);
+			return;
+		}
+	}
+
 	if (entry->lsk)
 		sock_release(entry->lsk);
 	kfree(entry);
@@ -1087,6 +1103,8 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	entry->ifindex = 0;
 	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 	entry->lsk = NULL;
+	entry->subflows = 0;
+	refcount_set(&entry->refcnt, 1);
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry);
 	if (ret < 0)
 		kfree(entry);
@@ -1314,6 +1332,7 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	*entry = addr;
+	refcount_set(&entry->refcnt, 1);
 	if (entry->addr.port) {
 		ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
 		if (ret) {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a71b64565e04..2194e53070d8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -245,6 +245,7 @@ struct mptcp_pm_addr_entry {
 	u8			flags;
 	int			ifindex;
 	struct socket		*lsk;
+	u8			subflows;
 	refcount_t		refcnt;
 };
 
-- 
2.35.3

Re: [PATCH mptcp-next v3 19/29] mptcp: add netlink pm addr entry refcount

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds netlink PM address entry refcount. Init 'refcont' of
> every address entry to 1. And add a new filed 'subflows' in struct
> mptcp_pm_addr_entry, inited to 0, to store how many subflows have
> been established on this address entry.
> 
> Increase both values in mptcp_pm_create_subflow_or_signal_addr() and
> fill_local_addresses_vec(), and decrease the counter 'refcont' in
> __mptcp_pm_release_addr_entry() according its 'subflows' value.

Please *always* explain WHY this patch is needed.

When reading the code, it is not clear to me what you are trying to fix
here: with the Netlink PM, the userspace adds and deletes endpoints, not
specific subflows. I mean: we don't have the same issue as we have with
the userspace PM. Or is there a different issue?

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

[PATCH mptcp-next v3 20/29] selftests: mptcp: add userspace pm fullmesh tests

[Geliang Tang]

This patch adds fullmesh selftests for userspace pm both on server side
and on client side. For the server side test, add two endpoints with
fullmesh flag on ns2, then signal an address on ns1 by userspace PM to
trigger the fullmesh connections. For the client side test, just use
userspace PM to create multiple subflows to do the fullmesh connections.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index aac50ef86785..d883c6c2426b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3532,6 +3532,46 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm server fullmesh
+	if reset_with_events "userspace pm server fullmesh" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns1
+		pm_nl_set_limits $ns2 5 5
+		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow,fullmesh
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,fullmesh
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns1
+		userspace_pm_add_addr $ns1 10.0.2.1 10
+		chk_join_nr 4 4 4
+		chk_add_nr 1 1
+		chk_mptcp_info subflows 4 subflows 4
+		chk_subflows_total 5 5
+		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
+		mptcp_lib_evts_kill
+		wait $tests_pid
+	fi
+
+	# userspace pm client fullmesh
+	if reset_with_events "userspace pm client fullmesh" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		pm_nl_set_limits $ns1 5 5
+		set_userspace_pm $ns2
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns1
+		userspace_pm_add_sf $ns2 10.0.2.2 20
+		userspace_pm_add_sf $ns2 10.0.3.2 30
+		userspace_pm_add_sf $ns2 10.0.4.2 40
+		chk_join_nr 3 3 3
+		chk_mptcp_info subflows 3 subflows 3
+		chk_subflows_total 4 4
+		mptcp_lib_evts_kill
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v3 20/29] selftests: mptcp: add userspace pm fullmesh tests

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds fullmesh selftests for userspace pm both on server side
> and on client side. For the server side test, add two endpoints with
> fullmesh flag on ns2, then signal an address on ns1 by userspace PM to
> trigger the fullmesh connections. For the client side test, just use
> userspace PM to create multiple subflows to do the fullmesh connections.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index aac50ef86785..d883c6c2426b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3532,6 +3532,46 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm server fullmesh
> +	if reset_with_events "userspace pm server fullmesh" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		set_userspace_pm $ns1
> +		pm_nl_set_limits $ns2 5 5
> +		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow,fullmesh
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,fullmesh
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns1
> +		userspace_pm_add_addr $ns1 10.0.2.1 10
> +		chk_join_nr 4 4 4
> +		chk_add_nr 1 1
> +		chk_mptcp_info subflows 4 subflows 4
> +		chk_subflows_total 5 5

It is strange to me to see 5 subflows. Is it because not all endpoints
are marked as fullmesh?

Would it not be clearer to do a typical fullmesh cases using 2 endpoints
(A, B) on each side? So we would have 4 subflows: A-A, A-B, B-B, B-A.

> +		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
> +		mptcp_lib_evts_kill
> +		wait $tests_pid
> +	fi
> +
> +	# userspace pm client fullmesh
> +	if reset_with_events "userspace pm client fullmesh" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		pm_nl_set_limits $ns1 5 5
> +		set_userspace_pm $ns2
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns1
> +		userspace_pm_add_sf $ns2 10.0.2.2 20
> +		userspace_pm_add_sf $ns2 10.0.3.2 30
> +		userspace_pm_add_sf $ns2 10.0.4.2 40

Same here, we should mix endpoints: here it looks like you create 4
subflows from 4 different IPs, that's not really a "fullmesh": the same
endpoint should be re-used multiple times to really validate the
"fullmesh" case, e.g.

 - Client has 2 endpoints: 1.1.1.1 and 2.2.2.2
 - Server has 2 endpoints: 8.8.8.8 and 9.9.9.9

We should have 2x2 subflows:

 - 1.1.1.1 <-> 8.8.8.8
 - 1.1.1.1 <-> 9.9.9.9
 - 2.2.2.2 <-> 8.8.8.8
 - 2.2.2.2 <-> 9.9.9.9

> +		chk_join_nr 3 3 3
> +		chk_mptcp_info subflows 3 subflows 3
> +		chk_subflows_total 4 4
> +		mptcp_lib_evts_kill
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

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

[PATCH mptcp-next v3 21/29] selftests: mptcp: add mptcp_lib_kill_wait

[Geliang Tang]

Export kill_wait() helper in userspace_pm.sh into mptcp_lib.sh and
rename it as mptcp_lib_kill_wait(). It can be used to instead of
kill_wait() in mptcp_join.sh. Use the new helper in both scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 10 ++----
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  8 +++++
 .../selftests/net/mptcp/userspace_pm.sh       | 31 +++++++------------
 3 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index d883c6c2426b..f335a782c793 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -682,16 +682,10 @@ wait_mpj()
 	done
 }
 
-kill_wait()
-{
-	kill $1 > /dev/null 2>&1
-	wait $1 2>/dev/null
-}
-
 kill_events_pids()
 {
-	kill_wait $evts_ns1_pid
-	kill_wait $evts_ns2_pid
+	mptcp_lib_kill_wait $evts_ns1_pid
+	mptcp_lib_kill_wait $evts_ns2_pid
 }
 
 kill_tests_wait()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index bb95dd967eb3..8051ac5507dc 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -219,3 +219,11 @@ mptcp_lib_get_info_value() {
 mptcp_lib_evts_get_info() {
 	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
 }
+
+mptcp_lib_kill_wait() {
+	[ $1 -eq 0 ] && return 0
+
+	kill -SIGUSR1 $1 > /dev/null 2>&1
+	kill $1 > /dev/null 2>&1
+	wait $1 2>/dev/null
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 283c62deb628..7cddf793950a 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -107,15 +107,6 @@ test_fail()
 	mptcp_lib_result_fail "${test_name}"
 }
 
-kill_wait()
-{
-	[ $1 -eq 0 ] && return 0
-
-	kill -SIGUSR1 $1 > /dev/null 2>&1
-	kill $1 > /dev/null 2>&1
-	wait $1 2>/dev/null
-}
-
 # This function is used in the cleanup trap
 #shellcheck disable=SC2317
 cleanup()
@@ -127,7 +118,7 @@ cleanup()
 	for pid in $client4_pid $server4_pid $client6_pid $server6_pid\
 		   $server_evts_pid $client_evts_pid
 	do
-		kill_wait $pid
+		mptcp_lib_kill_wait $pid
 	done
 
 	local netns
@@ -209,7 +200,7 @@ make_connection()
 	fi
 	:>"$client_evts"
 	if [ $client_evts_pid -ne 0 ]; then
-		kill_wait $client_evts_pid
+		mptcp_lib_kill_wait $client_evts_pid
 	fi
 	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
 	client_evts_pid=$!
@@ -218,7 +209,7 @@ make_connection()
 	fi
 	:>"$server_evts"
 	if [ $server_evts_pid -ne 0 ]; then
-		kill_wait $server_evts_pid
+		mptcp_lib_kill_wait $server_evts_pid
 	fi
 	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &
 	server_evts_pid=$!
@@ -623,7 +614,7 @@ test_subflows()
 			      "10.0.2.2" "$client4_port" "23" "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	local sport
 	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
@@ -662,7 +653,7 @@ test_subflows()
 			      "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
 
@@ -701,7 +692,7 @@ test_subflows()
 			      "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
 
@@ -739,7 +730,7 @@ test_subflows()
 			      "10.0.2.1" "$app4_port" "23" "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
@@ -778,7 +769,7 @@ test_subflows()
 			      "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
@@ -815,7 +806,7 @@ test_subflows()
 			      "10.0.2.2" "10.0.2.1" "$new4_port" "23" "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
@@ -861,7 +852,7 @@ test_subflows_v4_v6_mix()
 			      "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
 
@@ -973,7 +964,7 @@ test_listener()
 	sleep 0.5
 
 	# Delete the listener from the client ns, if one was created
-	kill_wait $listener_pid
+	mptcp_lib_kill_wait $listener_pid
 
 	sleep 0.5
 	verify_listener_events $client_evts $LISTENER_CLOSED $AF_INET 10.0.2.2 $client4_port
-- 
2.35.3

Re: [PATCH mptcp-next v3 21/29] selftests: mptcp: add mptcp_lib_kill_wait

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> Export kill_wait() helper in userspace_pm.sh into mptcp_lib.sh and
> rename it as mptcp_lib_kill_wait(). It can be used to instead of
> kill_wait() in mptcp_join.sh. Use the new helper in both scripts.

Good idea!

Just to make things 100% clear, please always start by explaining the
reason why this patch is interesting. Here it can simply be:

  To avoid duplicated code in different MPTCP selftests, we can add
  and use helpers defined in mptcp_lib.sh.

Same in the following commits. You can even copy-paste this explanation
in the next ones.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index bb95dd967eb3..8051ac5507dc 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -219,3 +219,11 @@ mptcp_lib_get_info_value() {
>  mptcp_lib_evts_get_info() {
>  	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
>  }
> +
> +mptcp_lib_kill_wait() {

Do you mind adding a comment just above mentioning the parameters that
are accepted? Like it is done in other functions, e.g.:

  # $1: PID

> +	[ $1 -eq 0 ] && return 0

Could you add "{}" (${1}) like it is done in the rest of the file, just
to keep the uniformity? (and because it *seems* better to have an
explicit delimiter in Bash to avoid mistakes)

Also, please also add double quotes around variables ("${1}") to keep
shellcheck happy (and because it is safer and for the uniformity).

> +
> +	kill -SIGUSR1 $1 > /dev/null 2>&1
> +	kill $1 > /dev/null 2>&1
> +	wait $1 2>/dev/null

(Same here and in the following commits)

Regarding ShellCheck, please always run it on the different .sh scripts
not to introduce new issues.

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

[PATCH mptcp-next v3 22/29] selftests: mptcp: add mptcp_lib_evts_*

[Geliang Tang]

This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
and mptcp_join.sh into four functions: _init, _start, _kill and _remove.
Define them in mptcp_lib.sh and use these new helper in both scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 54 +++++++------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 40 ++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 31 +++--------
 3 files changed, 65 insertions(+), 60 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f335a782c793..8c708b92a942 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -35,10 +35,6 @@ ip_mptcp=0
 check_invert=0
 validate_checksum=0
 init=0
-evts_ns1=""
-evts_ns2=""
-evts_ns1_pid=0
-evts_ns2_pid=0
 last_test_failed=0
 last_test_skipped=0
 last_test_ignored=1
@@ -182,8 +178,7 @@ init() {
 	cin=$(mktemp)
 	cinsent=$(mktemp)
 	cout=$(mktemp)
-	evts_ns1=$(mktemp)
-	evts_ns2=$(mktemp)
+	mptcp_lib_evts_init
 
 	trap cleanup EXIT
 
@@ -196,7 +191,7 @@ cleanup()
 	rm -f "$cin" "$cout" "$sinfail"
 	rm -f "$sin" "$sout" "$cinsent" "$cinfail"
 	rm -f "$tmpfile"
-	rm -rf $evts_ns1 $evts_ns2
+	mptcp_lib_evts_remove
 	cleanup_partial
 }
 
@@ -460,12 +455,7 @@ reset_with_events()
 {
 	reset "${1}" || return 1
 
-	:> "$evts_ns1"
-	:> "$evts_ns2"
-	ip netns exec $ns1 ./pm_nl_ctl events >> "$evts_ns1" 2>&1 &
-	evts_ns1_pid=$!
-	ip netns exec $ns2 ./pm_nl_ctl events >> "$evts_ns2" 2>&1 &
-	evts_ns2_pid=$!
+	mptcp_lib_evts_start
 }
 
 reset_with_tcp_filter()
@@ -682,12 +672,6 @@ wait_mpj()
 	done
 }
 
-kill_events_pids()
-{
-	mptcp_lib_kill_wait $evts_ns1_pid
-	mptcp_lib_kill_wait $evts_ns2_pid
-}
-
 kill_tests_wait()
 {
 	#shellcheck disable=SC2046
@@ -2907,9 +2891,9 @@ add_addr_ports_tests()
 		chk_add_nr 1 1 1
 		chk_rm_nr 1 1 invert
 
-		verify_listener_events $evts_ns1 $LISTENER_CREATED $AF_INET 10.0.2.1 10100
-		verify_listener_events $evts_ns1 $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
-		kill_events_pids
+		verify_listener_events $server_evts $LISTENER_CREATED $AF_INET 10.0.2.1 10100
+		verify_listener_events $server_evts $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
+		mptcp_lib_evts_kill
 	fi
 
 	# subflow and signal with port, remove
@@ -3282,10 +3266,10 @@ fail_tests()
 # $1: ns ; $2: addr ; $3: id
 userspace_pm_add_addr()
 {
-	local evts=$evts_ns1
+	local evts=$server_evts
 	local tk
 
-	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	[ "$1" == "$ns2" ] && evts=$client_evts
 	tk=$(mptcp_lib_evts_get_info token "$evts")
 
 	ip netns exec $1 ./pm_nl_ctl ann $2 token $tk id $3
@@ -3295,10 +3279,10 @@ userspace_pm_add_addr()
 # $1: ns ; $2: id
 userspace_pm_rm_addr()
 {
-	local evts=$evts_ns1
+	local evts=$server_evts
 	local tk
 
-	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	[ "$1" == "$ns2" ] && evts=$client_evts
 	tk=$(mptcp_lib_evts_get_info token "$evts")
 
 	ip netns exec $1 ./pm_nl_ctl rem token $tk id $2
@@ -3308,10 +3292,10 @@ userspace_pm_rm_addr()
 # $1: ns ; $2: addr ; $3: id
 userspace_pm_add_sf()
 {
-	local evts=$evts_ns1
+	local evts=$server_evts
 	local tk da dp
 
-	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	[ "$1" == "$ns2" ] && evts=$client_evts
 	tk=$(mptcp_lib_evts_get_info token "$evts")
 	da=$(mptcp_lib_evts_get_info daddr4 "$evts")
 	dp=$(mptcp_lib_evts_get_info dport "$evts")
@@ -3324,12 +3308,12 @@ userspace_pm_add_sf()
 # $1: ns ; $2: addr $3: event type
 userspace_pm_rm_sf()
 {
-	local evts=$evts_ns1
+	local evts=$server_evts
 	local t=${3:-1}
 	local ip=4
 	local tk da dp sp
 
-	[ "$1" == "$ns2" ] && evts=$evts_ns2
+	[ "$1" == "$ns2" ] && evts=$client_evts
 	if is_v6 $2; then ip=6; fi
 	tk=$(mptcp_lib_evts_get_info token "$evts")
 	da=$(mptcp_lib_evts_get_info "daddr$ip" "$evts" $t)
@@ -3436,7 +3420,7 @@ userspace_tests()
 		chk_rm_nr 1 1 invert
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
-		kill_events_pids
+		mptcp_lib_evts_kill
 		wait $tests_pid
 	fi
 
@@ -3458,7 +3442,7 @@ userspace_tests()
 		chk_rm_nr 1 1
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
-		kill_events_pids
+		mptcp_lib_evts_kill
 		wait $tests_pid
 	fi
 
@@ -3477,7 +3461,7 @@ userspace_tests()
 		chk_join_nr 1 1 1
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 2 2
-		kill_events_pids
+		mptcp_lib_evts_kill
 		wait $tests_pid
 	fi
 
@@ -3499,7 +3483,7 @@ userspace_tests()
 		chk_rst_nr 1 1 invert
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 1 1
-		kill_events_pids
+		mptcp_lib_evts_kill
 		wait $tests_pid
 	fi
 
@@ -3523,7 +3507,7 @@ userspace_tests()
 		chk_rst_nr 1 1 invert
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 1 1
-		kill_events_pids
+		mptcp_lib_evts_kill
 		wait $tests_pid
 	fi
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 8051ac5507dc..899c21ced5a3 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -220,6 +220,11 @@ mptcp_lib_evts_get_info() {
 	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
 }
 
+server_evts=""
+client_evts=""
+server_evts_pid=0
+client_evts_pid=0
+
 mptcp_lib_kill_wait() {
 	[ $1 -eq 0 ] && return 0
 
@@ -227,3 +232,38 @@ mptcp_lib_kill_wait() {
 	kill $1 > /dev/null 2>&1
 	wait $1 2>/dev/null
 }
+
+mptcp_lib_evts_init() {
+	if [ -z "$server_evts" ]; then
+		server_evts=$(mktemp)
+	fi
+	if [ -z "$client_evts" ]; then
+		client_evts=$(mktemp)
+	fi
+}
+
+mptcp_lib_evts_start() {
+	:>"$server_evts"
+	:>"$client_evts"
+
+	if [ $server_evts_pid -ne 0 ]; then
+		mptcp_lib_kill_wait $server_evts_pid
+	fi
+	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &
+	server_evts_pid=$!
+
+	if [ $client_evts_pid -ne 0 ]; then
+		mptcp_lib_kill_wait $client_evts_pid
+	fi
+	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
+	client_evts_pid=$!
+}
+
+mptcp_lib_evts_kill() {
+	mptcp_lib_kill_wait $server_evts_pid
+	mptcp_lib_kill_wait $client_evts_pid
+}
+
+mptcp_lib_evts_remove() {
+	rm -rf $server_evts $client_evts
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 7cddf793950a..640273d4d7a1 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -33,10 +33,6 @@ AF_INET=2
 AF_INET6=10
 
 file=""
-server_evts=""
-client_evts=""
-server_evts_pid=0
-client_evts_pid=0
 client4_pid=0
 server4_pid=0
 client6_pid=0
@@ -115,18 +111,19 @@ cleanup()
 
 	# Terminate the MPTCP connection and related processes
 	local pid
-	for pid in $client4_pid $server4_pid $client6_pid $server6_pid\
-		   $server_evts_pid $client_evts_pid
+	for pid in $client4_pid $server4_pid $client6_pid $server6_pid
 	do
 		mptcp_lib_kill_wait $pid
 	done
+	mptcp_lib_evts_kill
 
 	local netns
 	for netns in "$ns1" "$ns2" ;do
 		ip netns del "$netns"
 	done
 
-	rm -rf $file $client_evts $server_evts
+	rm -rf $file
+	mptcp_lib_evts_remove
 
 	_printf "Done\n"
 }
@@ -195,24 +192,8 @@ make_connection()
 
 	# Capture netlink events over the two network namespaces running
 	# the MPTCP client and server
-	if [ -z "$client_evts" ]; then
-		client_evts=$(mktemp)
-	fi
-	:>"$client_evts"
-	if [ $client_evts_pid -ne 0 ]; then
-		mptcp_lib_kill_wait $client_evts_pid
-	fi
-	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
-	client_evts_pid=$!
-	if [ -z "$server_evts" ]; then
-		server_evts=$(mktemp)
-	fi
-	:>"$server_evts"
-	if [ $server_evts_pid -ne 0 ]; then
-		mptcp_lib_kill_wait $server_evts_pid
-	fi
-	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &
-	server_evts_pid=$!
+	mptcp_lib_evts_init
+	mptcp_lib_evts_start
 	sleep 0.5
 
 	# Run the server
-- 
2.35.3

Re: [PATCH mptcp-next v3 22/29] selftests: mptcp: add mptcp_lib_evts_*

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
> and mptcp_join.sh into four functions: _init, _start, _kill and _remove.
> Define them in mptcp_lib.sh and use these new helper in both scripts.

Good idea to avoid duplicated code!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 8051ac5507dc..899c21ced5a3 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -220,6 +220,11 @@ mptcp_lib_evts_get_info() {
>  	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
>  }
>  
> +server_evts=""
> +client_evts=""
> +server_evts_pid=0
> +client_evts_pid=0

I wonder if it wouldn't be clearer to prefix these variables with
'mptcp_lib_': we would avoid overriding global variables by mistake from
other fiels and it would be clearer to know where these variables have
been defined. But I guess if we do that, we would need to do quite a few
modifications in userspace_pm.sh and we want to avoid that, no?

Or passing them as arguments to 'mptcp_lib_evts_XXX()'? It would be more
explicit but maybe not worth it?

An alternative could be to keep these variables declared in the
different files and keep these helpers as they are here. In this case,
it would be good to add a comment before mptcp_lib_evts_init()
explaining that these variables are needed.

Please also make sure shellcheck if happy with that, e.g. by adding a
'?' in the variable name like ${server_evts?} or explicitly at the
beginning of the function with something like ': "${var?}"' or ':
"${var:?}"' if it has to be non empty, e.g.

  mptcp_lib_evts_init() {
      : "${server_evts?}"
      : "${client_evts?}"

      (...)
  }

  mptcp_lib_evts_start() {
      : "${server_evts:?}"
      : "${client_evts:?}"
      : "${server_evts_pid:?}"
      : "${client_evts_pid:?}"

      (...)
  }

  mptcp_lib_evts_kill() {
      mptcp_lib_kill_wait "${server_evts_pid:?}"
      mptcp_lib_kill_wait "${client_evts_pid:?}"
  }

  mptcp_lib_evts_remove() {
      rm -rf "${server_evts:?}" "${client_evts:?}"
  }

> +
>  mptcp_lib_kill_wait() {
>  	[ $1 -eq 0 ] && return 0
>  
> @@ -227,3 +232,38 @@ mptcp_lib_kill_wait() {
>  	kill $1 > /dev/null 2>&1
>  	wait $1 2>/dev/null
>  }
> +
> +mptcp_lib_evts_init() {
> +	if [ -z "$server_evts" ]; then

Same here and below for the {}. (see comment from patch 21/29)

+ the double quotes below to keep shellcheck happy and be safer.

> +		server_evts=$(mktemp)
> +	fi
> +	if [ -z "$client_evts" ]; then
> +		client_evts=$(mktemp)
> +	fi
> +}
> +
> +mptcp_lib_evts_start() {
> +	:>"$server_evts"
> +	:>"$client_evts"
> +
> +	if [ $server_evts_pid -ne 0 ]; then
> +		mptcp_lib_kill_wait $server_evts_pid
> +	fi
> +	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &

I think ${ns1} and ${ns2} should be passed as argument to this function.

> +	server_evts_pid=$!
> +
> +	if [ $client_evts_pid -ne 0 ]; then
> +		mptcp_lib_kill_wait $client_evts_pid
> +	fi
> +	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
> +	client_evts_pid=$!
> +}
> +
> +mptcp_lib_evts_kill() {
> +	mptcp_lib_kill_wait $server_evts_pid
> +	mptcp_lib_kill_wait $client_evts_pid

Should we eventually set the pid to 0 after that just to be on the safe
side?

  server_evts_pid=0
  client_evts_pid=0

It was not needed before, maybe fine not to do that? Up to you (but if
you do, please add a note in the commit message)

> +}
> +
> +mptcp_lib_evts_remove() {
> +	rm -rf $server_evts $client_evts
> +}

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

[PATCH mptcp-next v3 23/29] selftests: mptcp: userspace: print colored results

[Geliang Tang]

Use mptcp_lib_print_ok(), _warn() and _err() to instead print_results() in
test_pass(), _skip() and _fail() in userspace_pm.sh to print test
results with colors.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 640273d4d7a1..84a77ee3b633 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -73,27 +73,22 @@ print_test()
 	_printf "%-63s" "${test_name}"
 }
 
-print_results()
-{
-	_printf "[%s]\n" "${1}"
-}
-
 test_pass()
 {
-	print_results " OK "
+	mptcp_lib_print_ok "[ ok ]${1:+ ${*}}"
 	mptcp_lib_result_pass "${test_name}"
 }
 
 test_skip()
 {
-	print_results "SKIP"
+	mptcp_lib_print_warn "[skip]${1:+ ${*}}"
 	mptcp_lib_result_skip "${test_name}"
 }
 
 # $1: msg
 test_fail()
 {
-	print_results "FAIL"
+	mptcp_lib_print_err "[fail]${1:+ ${*}}"
 	ret=1
 
 	if [ -n "${1}" ]; then
-- 
2.35.3

Re: [PATCH mptcp-next v3 23/29] selftests: mptcp: userspace: print colored results

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> Use mptcp_lib_print_ok(), _warn() and _err() to instead print_results() in
> test_pass(), _skip() and _fail() in userspace_pm.sh to print test
> results with colors.

Even if it is obvious, please always clearly add the reason why this
commit is needed, e.g.

  Having colours helps to quickly identify issues when looking at a
  long list of output logs and results.

Other than that, it is a good idea and it looks good to me:

Reviewed-by: Matthieu Baerts <matttbe@kernel.org>

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

[PATCH mptcp-next v3 24/29] selftests: mptcp: add mptcp_lib_verify_listener_events

[Geliang Tang]

verify_listener_events() helper is defined both in mptcp_join.sh and
userspace_pm.sh, export it into mptcp_lib.sh and rename it with
mptcp_lib_ prefix. Use this new helper in both scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 59 ++-----------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 56 ++++++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 41 ++-----------
 3 files changed, 64 insertions(+), 92 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 8c708b92a942..e59867eed5c2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2803,59 +2803,6 @@ backup_tests()
 	fi
 }
 
-LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
-LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
-
-AF_INET=2
-AF_INET6=10
-
-verify_listener_events()
-{
-	local evt=$1
-	local e_type=$2
-	local e_family=$3
-	local e_saddr=$4
-	local e_sport=$5
-	local type
-	local family
-	local saddr
-	local sport
-	local name
-
-	if [ $e_type = $LISTENER_CREATED ]; then
-		name="LISTENER_CREATED"
-	elif [ $e_type = $LISTENER_CLOSED ]; then
-		name="LISTENER_CLOSED "
-	else
-		name="$e_type"
-	fi
-
-	print_check "$name $e_saddr:$e_sport"
-
-	if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
-		print_skip "event not supported"
-		return
-	fi
-
-	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
-	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
-	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
-	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
-	else
-		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
-	fi
-
-	if [ $type ] && [ $type = $e_type ] &&
-	   [ $family ] && [ $family = $e_family ] &&
-	   [ $saddr ] && [ $saddr = $e_saddr ] &&
-	   [ $sport ] && [ $sport = $e_sport ]; then
-		print_ok
-		return 0
-	fi
-	fail_test "$e_type:$type $e_family:$family $e_saddr:$saddr $e_sport:$sport"
-}
-
 add_addr_ports_tests()
 {
 	# signal address with port
@@ -2891,8 +2838,10 @@ add_addr_ports_tests()
 		chk_add_nr 1 1 1
 		chk_rm_nr 1 1 invert
 
-		verify_listener_events $server_evts $LISTENER_CREATED $AF_INET 10.0.2.1 10100
-		verify_listener_events $server_evts $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
+		mptcp_lib_verify_listener_events $server_evts $LISTENER_CREATED \
+			$AF_INET 10.0.2.1 10100
+		mptcp_lib_verify_listener_events $server_evts $LISTENER_CLOSED \
+			$AF_INET 10.0.2.1 10100
 		mptcp_lib_evts_kill
 	fi
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 899c21ced5a3..1301af71ad2c 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -9,6 +9,11 @@ readonly KSFT_SKIP=4
 readonly KSFT_TEST=$(basename "${0}" | sed 's/\.sh$//g')
 
 SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
+LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
+LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
+
+AF_INET=2
+AF_INET6=10
 
 MPTCP_LIB_SUBTESTS=()
 
@@ -267,3 +272,54 @@ mptcp_lib_evts_kill() {
 mptcp_lib_evts_remove() {
 	rm -rf $server_evts $client_evts
 }
+
+mptcp_lib_verify_listener_events() {
+	local evt=$1
+	local e_type=$2
+	local e_family=$3
+	local e_saddr=$4
+	local e_sport=$5
+	local type
+	local family
+	local saddr
+	local sport
+	local name
+
+	if [ $e_type = $LISTENER_CREATED ]; then
+		name="LISTENER_CREATED"
+	elif [ $e_type = $LISTENER_CLOSED ]; then
+		name="LISTENER_CLOSED "
+	else
+		name="$e_type"
+	fi
+
+	if [ "$(basename "$0")" == "mptcp_join.sh" ]; then
+		printf "%-6s%-36s" " " "$name $e_saddr:$e_sport"
+	elif [ "$(basename "$0")" == "userspace_pm.sh" ]; then
+		printf "%-63s" "$name $e_saddr:$e_sport"
+	fi
+
+	if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
+		mptcp_lib_print_warn "[skip] event not supported"
+		return
+	fi
+
+	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
+	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
+	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
+	if [ $family ] && [ $family = $AF_INET6 ]; then
+		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
+	else
+		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
+	fi
+
+	if [ $type ] && [ $type = $e_type ] &&
+	   [ $family ] && [ $family = $e_family ] &&
+	   [ $saddr ] && [ $saddr = $e_saddr ] &&
+	   [ $sport ] && [ $sport = $e_sport ]; then
+		mptcp_lib_print_ok "[ ok ]"
+		return 0
+	fi
+	mptcp_lib_print_err "[fail] $e_type:$type $e_family:$family \
+		$e_saddr:$saddr $e_sport:$sport"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 84a77ee3b633..98189b3f73dc 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -26,11 +26,6 @@ fi
 ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
 REMOVED=7          # MPTCP_EVENT_REMOVED
 SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
-LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
-LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
-
-AF_INET=2
-AF_INET6=10
 
 file=""
 client4_pid=0
@@ -878,36 +873,6 @@ test_prio()
 	fi
 }
 
-verify_listener_events()
-{
-	local evt=$1
-	local e_type=$2
-	local e_family=$3
-	local e_saddr=$4
-	local e_sport=$5
-	local type
-	local family
-	local saddr
-	local sport
-
-	if [ $e_type = $LISTENER_CREATED ]; then
-		print_test "CREATE_LISTENER $e_saddr:$e_sport"
-	elif [ $e_type = $LISTENER_CLOSED ]; then
-		print_test "CLOSE_LISTENER $e_saddr:$e_sport"
-	fi
-
-	type=$(mptcp_lib_evts_get_info type $evt $e_type)
-	family=$(mptcp_lib_evts_get_info family $evt $e_type)
-	sport=$(mptcp_lib_evts_get_info sport $evt $e_type)
-	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(mptcp_lib_evts_get_info saddr6 $evt $e_type)
-	else
-		saddr=$(mptcp_lib_evts_get_info saddr4 $evt $e_type)
-	fi
-
-	check_expected "type" "family" "saddr" "sport"
-}
-
 test_listener()
 {
 	print_title "Listener tests"
@@ -927,7 +892,8 @@ test_listener()
 	local listener_pid=$!
 
 	sleep 0.5
-	verify_listener_events $client_evts $LISTENER_CREATED $AF_INET 10.0.2.2 $client4_port
+	mptcp_lib_verify_listener_events $client_evts $LISTENER_CREATED \
+		$AF_INET 10.0.2.2 $client4_port
 
 	# ADD_ADDR from client to server machine reusing the subflow port
 	ip netns exec $ns2 ./pm_nl_ctl ann 10.0.2.2 token $client4_token id\
@@ -943,7 +909,8 @@ test_listener()
 	mptcp_lib_kill_wait $listener_pid
 
 	sleep 0.5
-	verify_listener_events $client_evts $LISTENER_CLOSED $AF_INET 10.0.2.2 $client4_port
+	mptcp_lib_verify_listener_events $client_evts $LISTENER_CLOSED \
+		$AF_INET 10.0.2.2 $client4_port
 }
 
 print_title "Make connections"
-- 
2.35.3

Re: [PATCH mptcp-next v3 24/29] selftests: mptcp: add mptcp_lib_verify_listener_events

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> verify_listener_events() helper is defined both in mptcp_join.sh and
> userspace_pm.sh, export it into mptcp_lib.sh and rename it with
> mptcp_lib_ prefix. Use this new helper in both scripts.

Good idea but please see below: because there are quite a few small
differences between mptcp_join.sh and userspace_pm.sh in the way we
print stuff, mark subtests as failed, print errors, etc. I think we are
not ready for such helper and it might be easier to drop this patch for
the moment. What do you think?


(if we want to keep this patch, please don't forget to clearly mention
the reason first (copy-paste from my comment on patch 21/29) before
describing what you did)

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 8c708b92a942..e59867eed5c2 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2803,59 +2803,6 @@ backup_tests()
>  	fi
>  }
>  
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10
> -
> -verify_listener_events()
> -{
> -	local evt=$1
> -	local e_type=$2
> -	local e_family=$3
> -	local e_saddr=$4
> -	local e_sport=$5
> -	local type
> -	local family
> -	local saddr
> -	local sport
> -	local name
> -
> -	if [ $e_type = $LISTENER_CREATED ]; then
> -		name="LISTENER_CREATED"
> -	elif [ $e_type = $LISTENER_CLOSED ]; then
> -		name="LISTENER_CLOSED "
> -	else
> -		name="$e_type"
> -	fi
> -
> -	print_check "$name $e_saddr:$e_sport"
> -
> -	if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
> -		print_skip "event not supported"
> -		return
> -	fi
> -
> -	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
> -	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
> -	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
> -	if [ $family ] && [ $family = $AF_INET6 ]; then
> -		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
> -	else
> -		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
> -	fi
> -
> -	if [ $type ] && [ $type = $e_type ] &&
> -	   [ $family ] && [ $family = $e_family ] &&
> -	   [ $saddr ] && [ $saddr = $e_saddr ] &&
> -	   [ $sport ] && [ $sport = $e_sport ]; then
> -		print_ok
> -		return 0
> -	fi
> -	fail_test "$e_type:$type $e_family:$family $e_saddr:$saddr $e_sport:$sport"

It is important to call fail_test() because that's what is going to mark
the subtest (and the whole test) as failed.

> -}
> -
>  add_addr_ports_tests()
>  {
>  	# signal address with port
> @@ -2891,8 +2838,10 @@ add_addr_ports_tests()
>  		chk_add_nr 1 1 1
>  		chk_rm_nr 1 1 invert
>  
> -		verify_listener_events $server_evts $LISTENER_CREATED $AF_INET 10.0.2.1 10100
> -		verify_listener_events $server_evts $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
> +		mptcp_lib_verify_listener_events $server_evts $LISTENER_CREATED \
> +			$AF_INET 10.0.2.1 10100
> +		mptcp_lib_verify_listener_events $server_evts $LISTENER_CLOSED \
> +			$AF_INET 10.0.2.1 10100
>  		mptcp_lib_evts_kill
>  	fi
>  
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 899c21ced5a3..1301af71ad2c 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -9,6 +9,11 @@ readonly KSFT_SKIP=4
>  readonly KSFT_TEST=$(basename "${0}" | sed 's/\.sh$//g')
>  
>  SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> +LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> +LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED

Same as SUB_ESTABLISHED, it would be better to prefix that with
MPTCP_LIB_ if it is declared here or keep it declared elsewhere.

> +
> +AF_INET=2
> +AF_INET6=10

For these two, I think it is fine without MPTCP_LIB_ but we should add
'readonly' keyword.

>  
>  MPTCP_LIB_SUBTESTS=()
>  
> @@ -267,3 +272,54 @@ mptcp_lib_evts_kill() {
>  mptcp_lib_evts_remove() {
>  	rm -rf $server_evts $client_evts
>  }
> +
> +mptcp_lib_verify_listener_events() {
> +	local evt=$1
> +	local e_type=$2
> +	local e_family=$3
> +	local e_saddr=$4
> +	local e_sport=$5
> +	local type
> +	local family
> +	local saddr
> +	local sport
> +	local name
> +
> +	if [ $e_type = $LISTENER_CREATED ]; then
> +		name="LISTENER_CREATED"
> +	elif [ $e_type = $LISTENER_CLOSED ]; then
> +		name="LISTENER_CLOSED "
> +	else
> +		name="$e_type"
> +	fi
> +
> +	if [ "$(basename "$0")" == "mptcp_join.sh" ]; then
> +		printf "%-6s%-36s" " " "$name $e_saddr:$e_sport"
> +	elif [ "$(basename "$0")" == "userspace_pm.sh" ]; then
> +		printf "%-63s" "$name $e_saddr:$e_sport"
> +	fi

I think we should avoid such a thing in the lib. Instead, we can pass
information in a parameter.

> +
> +	if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
> +		mptcp_lib_print_warn "[skip] event not supported"
> +		return
> +	fi
> +
> +	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
> +	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
> +	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
> +	if [ $family ] && [ $family = $AF_INET6 ]; then
> +		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
> +	else
> +		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
> +	fi
> +
> +	if [ $type ] && [ $type = $e_type ] &&
> +	   [ $family ] && [ $family = $e_family ] &&
> +	   [ $saddr ] && [ $saddr = $e_saddr ] &&
> +	   [ $sport ] && [ $sport = $e_sport ]; then
> +		mptcp_lib_print_ok "[ ok ]"
> +		return 0
> +	fi
> +	mptcp_lib_print_err "[fail] $e_type:$type $e_family:$family \
> +		$e_saddr:$saddr $e_sport:$sport"

That's not enough to just print a WARN, OK or ERR, some extra actions
needs to be taken, e.g. marking the subtest as failed, same for the
whole selftest.

> +}
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 84a77ee3b633..98189b3f73dc 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -26,11 +26,6 @@ fi
>  ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
>  REMOVED=7          # MPTCP_EVENT_REMOVED
>  SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10
>  
>  file=""
>  client4_pid=0
> @@ -878,36 +873,6 @@ test_prio()
>  	fi
>  }
>  
> -verify_listener_events()
> -{
> -	local evt=$1
> -	local e_type=$2
> -	local e_family=$3
> -	local e_saddr=$4
> -	local e_sport=$5
> -	local type
> -	local family
> -	local saddr
> -	local sport
> -
> -	if [ $e_type = $LISTENER_CREATED ]; then
> -		print_test "CREATE_LISTENER $e_saddr:$e_sport"
> -	elif [ $e_type = $LISTENER_CLOSED ]; then
> -		print_test "CLOSE_LISTENER $e_saddr:$e_sport"
> -	fi
> -
> -	type=$(mptcp_lib_evts_get_info type $evt $e_type)
> -	family=$(mptcp_lib_evts_get_info family $evt $e_type)
> -	sport=$(mptcp_lib_evts_get_info sport $evt $e_type)
> -	if [ $family ] && [ $family = $AF_INET6 ]; then
> -		saddr=$(mptcp_lib_evts_get_info saddr6 $evt $e_type)
> -	else
> -		saddr=$(mptcp_lib_evts_get_info saddr4 $evt $e_type)
> -	fi
> -
> -	check_expected "type" "family" "saddr" "sport"

This a good function to call because it will tell you which parameter is
wrong. In the new helper, we will need to do the comparison ourselves
(and the subtest will not be marked as failed).

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

[PATCH mptcp-next v3 25/29] selftests: mptcp: add mptcp_lib_is_v6

[Geliang Tang]

is_v6() helper is defined in mptcp_connect.sh, mptcp_join.sh and
mptcp_sockopt.sh, so export it into mptcp_lib.sh and rename it as
mptcp_lib_is_v6(). Use this new helper in all scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_connect.sh | 16 +++++-----------
 tools/testing/selftests/net/mptcp/mptcp_join.sh  | 14 ++++----------
 tools/testing/selftests/net/mptcp/mptcp_lib.sh   |  5 +++++
 .../testing/selftests/net/mptcp/mptcp_sockopt.sh |  8 +-------
 4 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index b1fc8afd072d..4cf62b2b0480 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -310,12 +310,6 @@ check_mptcp_disabled()
 	return 0
 }
 
-# $1: IP address
-is_v6()
-{
-	[ -z "${1##*:*}" ]
-}
-
 do_ping()
 {
 	local listener_ns="$1"
@@ -324,7 +318,7 @@ do_ping()
 	local ping_args="-q -c 1"
 	local rc=0
 
-	if is_v6 "${connect_addr}"; then
+	if mptcp_lib_is_v6 "${connect_addr}"; then
 		$ipv6 || return 0
 		ping_args="${ping_args} -6"
 	fi
@@ -635,12 +629,12 @@ run_tests_lo()
 	fi
 
 	# skip if we don't want v6
-	if ! $ipv6 && is_v6 "${connect_addr}"; then
+	if ! $ipv6 && mptcp_lib_is_v6 "${connect_addr}"; then
 		return 0
 	fi
 
 	local local_addr
-	if is_v6 "${connect_addr}"; then
+	if mptcp_lib_is_v6 "${connect_addr}"; then
 		local_addr="::"
 	else
 		local_addr="0.0.0.0"
@@ -708,7 +702,7 @@ run_test_transparent()
 	TEST_GROUP="${msg}"
 
 	# skip if we don't want v6
-	if ! $ipv6 && is_v6 "${connect_addr}"; then
+	if ! $ipv6 && mptcp_lib_is_v6 "${connect_addr}"; then
 		return 0
 	fi
 
@@ -741,7 +735,7 @@ EOF
 	fi
 
 	local local_addr
-	if is_v6 "${connect_addr}"; then
+	if mptcp_lib_is_v6 "${connect_addr}"; then
 		local_addr="::"
 		r6flag="-6"
 	else
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e59867eed5c2..8ef91a939c1b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -577,12 +577,6 @@ link_failure()
 	done
 }
 
-# $1: IP address
-is_v6()
-{
-	[ -z "${1##*:*}" ]
-}
-
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -879,7 +873,7 @@ pm_nl_set_endpoint()
 		local id=10
 		while [ $add_nr_ns1 -gt 0 ]; do
 			local addr
-			if is_v6 "${connect_addr}"; then
+			if mptcp_lib_is_v6 "${connect_addr}"; then
 				addr="dead:beef:$counter::1"
 			else
 				addr="10.0.$counter.1"
@@ -931,7 +925,7 @@ pm_nl_set_endpoint()
 		local id=20
 		while [ $add_nr_ns2 -gt 0 ]; do
 			local addr
-			if is_v6 "${connect_addr}"; then
+			if mptcp_lib_is_v6 "${connect_addr}"; then
 				addr="dead:beef:$counter::2"
 			else
 				addr="10.0.$counter.2"
@@ -973,7 +967,7 @@ pm_nl_set_endpoint()
 			pm_nl_flush_endpoint ${connector_ns}
 		elif [ $rm_nr_ns2 -eq 9 ]; then
 			local addr
-			if is_v6 "${connect_addr}"; then
+			if mptcp_lib_is_v6 "${connect_addr}"; then
 				addr="dead:beef:1::2"
 			else
 				addr="10.0.1.2"
@@ -3263,7 +3257,7 @@ userspace_pm_rm_sf()
 	local tk da dp sp
 
 	[ "$1" == "$ns2" ] && evts=$client_evts
-	if is_v6 $2; then ip=6; fi
+	if mptcp_lib_is_v6 $2; then ip=6; fi
 	tk=$(mptcp_lib_evts_get_info token "$evts")
 	da=$(mptcp_lib_evts_get_info "daddr$ip" "$evts" $t)
 	dp=$(mptcp_lib_evts_get_info dport "$evts" $t)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 1301af71ad2c..91a17ae81cb8 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -323,3 +323,8 @@ mptcp_lib_verify_listener_events() {
 	mptcp_lib_print_err "[fail] $e_type:$type $e_family:$family \
 		$e_saddr:$saddr $e_sport:$sport"
 }
+
+# $1: IP address
+mptcp_lib_is_v6() {
+	[ -z "${1##*:*}" ]
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index a817af6616ec..bfa744e350ef 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -161,12 +161,6 @@ check_transfer()
 	return 0
 }
 
-# $1: IP address
-is_v6()
-{
-	[ -z "${1##*:*}" ]
-}
-
 do_transfer()
 {
 	local listener_ns="$1"
@@ -183,7 +177,7 @@ do_transfer()
 	local mptcp_connect="./mptcp_connect -r 20"
 
 	local local_addr ip
-	if is_v6 "${connect_addr}"; then
+	if mptcp_lib_is_v6 "${connect_addr}"; then
 		local_addr="::"
 		ip=ipv6
 	else
-- 
2.35.3

Re: [PATCH mptcp-next v3 25/29] selftests: mptcp: add mptcp_lib_is_v6

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> is_v6() helper is defined in mptcp_connect.sh, mptcp_join.sh and
> mptcp_sockopt.sh, so export it into mptcp_lib.sh and rename it as
> mptcp_lib_is_v6(). Use this new helper in all scripts.

Good idea, looks good to me!

Reviewed-by: Matthieu Baerts <matttbe@kernel.org>

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

[PATCH mptcp-next v3 26/29] selftests: mptcp: add mptcp_lib_get_counter

[Geliang Tang]

The helper get_counter() in mptcp_join.sh and get_mib_counter() in
mptcp_connect.sh have the same functionality, export get_counter() into
mptcp_lib.sh and rename it as mptcp_lib_get_counter(). Use this new
helper instead of get_counter() and get_mib_counter().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../selftests/net/mptcp/mptcp_connect.sh      | 41 +++------
 .../testing/selftests/net/mptcp/mptcp_join.sh | 88 ++++++++-----------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 16 ++++
 3 files changed, 65 insertions(+), 80 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 4cf62b2b0480..3b971d1617d8 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -335,21 +335,6 @@ do_ping()
 	return 0
 }
 
-# $1: ns, $2: MIB counter
-get_mib_counter()
-{
-	local listener_ns="${1}"
-	local mib="${2}"
-
-	# strip the header
-	ip netns exec "${listener_ns}" \
-		nstat -z -a "${mib}" | \
-			tail -n+2 | \
-			while read a count c rest; do
-				echo $count
-			done
-}
-
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -435,12 +420,12 @@ do_transfer()
 			nstat -n
 	fi
 
-	local stat_synrx_last_l=$(get_mib_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
-	local stat_ackrx_last_l=$(get_mib_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
-	local stat_cookietx_last=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesSent")
-	local stat_cookierx_last=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesRecv")
-	local stat_csum_err_s=$(get_mib_counter "${listener_ns}" "MPTcpExtDataCsumErr")
-	local stat_csum_err_c=$(get_mib_counter "${connector_ns}" "MPTcpExtDataCsumErr")
+	local stat_synrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
+	local stat_ackrx_last_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
+	local stat_cookietx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
+	local stat_cookierx_last=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
+	local stat_csum_err_s=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtDataCsumErr")
+	local stat_csum_err_c=$(mptcp_lib_get_counter "${connector_ns}" "MPTcpExtDataCsumErr")
 
 	timeout ${timeout_test} \
 		ip netns exec ${listener_ns} \
@@ -503,11 +488,11 @@ do_transfer()
 	check_transfer $cin $sout "file received by server"
 	rets=$?
 
-	local stat_synrx_now_l=$(get_mib_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
-	local stat_ackrx_now_l=$(get_mib_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
-	local stat_cookietx_now=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesSent")
-	local stat_cookierx_now=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesRecv")
-	local stat_ooo_now=$(get_mib_counter "${listener_ns}" "TcpExtTCPOFOQueue")
+	local stat_synrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
+	local stat_ackrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
+	local stat_cookietx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesSent")
+	local stat_cookierx_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtSyncookiesRecv")
+	local stat_ooo_now=$(mptcp_lib_get_counter "${listener_ns}" "TcpExtTCPOFOQueue")
 
 	expect_synrx=$((stat_synrx_last_l))
 	expect_ackrx=$((stat_ackrx_last_l))
@@ -536,8 +521,8 @@ do_transfer()
 	fi
 
 	if $checksum; then
-		local csum_err_s=$(get_mib_counter "${listener_ns}" "MPTcpExtDataCsumErr")
-		local csum_err_c=$(get_mib_counter "${connector_ns}" "MPTcpExtDataCsumErr")
+		local csum_err_s=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtDataCsumErr")
+		local csum_err_c=$(mptcp_lib_get_counter "${connector_ns}" "MPTcpExtDataCsumErr")
 
 		local csum_err_s_nr=$((csum_err_s - stat_csum_err_s))
 		if [ $csum_err_s_nr -gt 0 ]; then
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 8ef91a939c1b..26ea0919810f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -595,25 +595,9 @@ wait_local_port_listen()
 	done
 }
 
-# $1: ns ; $2: counter
-get_counter()
-{
-	local ns="${1}"
-	local counter="${2}"
-	local count
-
-	count=$(ip netns exec ${ns} nstat -asz "${counter}" | awk 'NR==1 {next} {print $2}')
-	if [ -z "${count}" ]; then
-		mptcp_lib_fail_if_expected_feature "${counter} counter"
-		return 1
-	fi
-
-	echo "${count}"
-}
-
 rm_addr_count()
 {
-	get_counter "${1}" "MPTcpExtRmAddr"
+	mptcp_lib_get_counter "${1}" "MPTcpExtRmAddr"
 }
 
 # $1: ns, $2: old rm_addr counter in $ns
@@ -633,7 +617,7 @@ wait_rm_addr()
 
 rm_sf_count()
 {
-	get_counter "${1}" "MPTcpExtRmSubflow"
+	mptcp_lib_get_counter "${1}" "MPTcpExtRmSubflow"
 }
 
 # $1: ns, $2: old rm_sf counter in $ns
@@ -656,11 +640,11 @@ wait_mpj()
 	local ns="${1}"
 	local cnt old_cnt
 
-	old_cnt=$(get_counter ${ns} "MPTcpExtMPJoinAckRx")
+	old_cnt=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPJoinAckRx")
 
 	local i
 	for i in $(seq 10); do
-		cnt=$(get_counter ${ns} "MPTcpExtMPJoinAckRx")
+		cnt=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPJoinAckRx")
 		[ "$cnt" = "${old_cnt}" ] || break
 		sleep 0.1
 	done
@@ -1256,7 +1240,7 @@ chk_csum_nr()
 	fi
 
 	print_check "sum"
-	count=$(get_counter ${ns1} "MPTcpExtDataCsumErr")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
 	if [ "$count" != "$csum_ns1" ]; then
 		extra_msg="$extra_msg ns1=$count"
 	fi
@@ -1269,7 +1253,7 @@ chk_csum_nr()
 		print_ok
 	fi
 	print_check "csum"
-	count=$(get_counter ${ns2} "MPTcpExtDataCsumErr")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
 	if [ "$count" != "$csum_ns2" ]; then
 		extra_msg="$extra_msg ns2=$count"
 	fi
@@ -1313,7 +1297,7 @@ chk_fail_nr()
 	fi
 
 	print_check "ftx"
-	count=$(get_counter ${ns_tx} "MPTcpExtMPFailTx")
+	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
 	if [ "$count" != "$fail_tx" ]; then
 		extra_msg="$extra_msg,tx=$count"
 	fi
@@ -1327,7 +1311,7 @@ chk_fail_nr()
 	fi
 
 	print_check "failrx"
-	count=$(get_counter ${ns_rx} "MPTcpExtMPFailRx")
+	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
 	if [ "$count" != "$fail_rx" ]; then
 		extra_msg="$extra_msg,rx=$count"
 	fi
@@ -1360,7 +1344,7 @@ chk_fclose_nr()
 	fi
 
 	print_check "ctx"
-	count=$(get_counter ${ns_tx} "MPTcpExtMPFastcloseTx")
+	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFastcloseTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$fclose_tx" ]; then
@@ -1371,7 +1355,7 @@ chk_fclose_nr()
 	fi
 
 	print_check "fclzrx"
-	count=$(get_counter ${ns_rx} "MPTcpExtMPFastcloseRx")
+	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFastcloseRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$fclose_rx" ]; then
@@ -1401,7 +1385,7 @@ chk_rst_nr()
 	fi
 
 	print_check "rtx"
-	count=$(get_counter ${ns_tx} "MPTcpExtMPRstTx")
+	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPRstTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ $count -lt $rst_tx ]; then
@@ -1411,7 +1395,7 @@ chk_rst_nr()
 	fi
 
 	print_check "rstrx"
-	count=$(get_counter ${ns_rx} "MPTcpExtMPRstRx")
+	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPRstRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" -lt "$rst_rx" ]; then
@@ -1430,7 +1414,7 @@ chk_infi_nr()
 	local count
 
 	print_check "itx"
-	count=$(get_counter ${ns2} "MPTcpExtInfiniteMapTx")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtInfiniteMapTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$infi_tx" ]; then
@@ -1440,7 +1424,7 @@ chk_infi_nr()
 	fi
 
 	print_check "infirx"
-	count=$(get_counter ${ns1} "MPTcpExtInfiniteMapRx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtInfiniteMapRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$infi_rx" ]; then
@@ -1469,7 +1453,7 @@ chk_join_nr()
 	fi
 
 	print_check "syn"
-	count=$(get_counter ${ns1} "MPTcpExtMPJoinSynRx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinSynRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$syn_nr" ]; then
@@ -1480,7 +1464,7 @@ chk_join_nr()
 
 	print_check "synack"
 	with_cookie=$(ip netns exec $ns2 sysctl -n net.ipv4.tcp_syncookies)
-	count=$(get_counter ${ns2} "MPTcpExtMPJoinSynAckRx")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynAckRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$syn_ack_nr" ]; then
@@ -1497,7 +1481,7 @@ chk_join_nr()
 	fi
 
 	print_check "ack"
-	count=$(get_counter ${ns1} "MPTcpExtMPJoinAckRx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$ack_nr" ]; then
@@ -1530,8 +1514,8 @@ chk_stale_nr()
 
 	print_check "stale"
 
-	stale_nr=$(get_counter ${ns} "MPTcpExtSubflowStale")
-	recover_nr=$(get_counter ${ns} "MPTcpExtSubflowRecover")
+	stale_nr=$(mptcp_lib_get_counter ${ns} "MPTcpExtSubflowStale")
+	recover_nr=$(mptcp_lib_get_counter ${ns} "MPTcpExtSubflowRecover")
 	if [ -z "$stale_nr" ] || [ -z "$recover_nr" ]; then
 		print_skip
 	elif [ $stale_nr -lt $stale_min ] ||
@@ -1568,7 +1552,7 @@ chk_add_nr()
 	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
 
 	print_check "add"
-	count=$(get_counter ${ns2} "MPTcpExtAddAddr")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtAddAddr")
 	if [ -z "$count" ]; then
 		print_skip
 	# if the test configured a short timeout tolerate greater then expected
@@ -1580,7 +1564,7 @@ chk_add_nr()
 	fi
 
 	print_check "echo"
-	count=$(get_counter ${ns1} "MPTcpExtEchoAdd")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtEchoAdd")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$echo_nr" ]; then
@@ -1591,7 +1575,7 @@ chk_add_nr()
 
 	if [ $port_nr -gt 0 ]; then
 		print_check "pt"
-		count=$(get_counter ${ns2} "MPTcpExtPortAdd")
+		count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtPortAdd")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$port_nr" ]; then
@@ -1601,7 +1585,7 @@ chk_add_nr()
 		fi
 
 		print_check "syn"
-		count=$(get_counter ${ns1} "MPTcpExtMPJoinPortSynRx")
+		count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$syn_nr" ]; then
@@ -1612,7 +1596,7 @@ chk_add_nr()
 		fi
 
 		print_check "synack"
-		count=$(get_counter ${ns2} "MPTcpExtMPJoinPortSynAckRx")
+		count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinPortSynAckRx")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$syn_ack_nr" ]; then
@@ -1623,7 +1607,7 @@ chk_add_nr()
 		fi
 
 		print_check "ack"
-		count=$(get_counter ${ns1} "MPTcpExtMPJoinPortAckRx")
+		count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$ack_nr" ]; then
@@ -1634,7 +1618,7 @@ chk_add_nr()
 		fi
 
 		print_check "syn"
-		count=$(get_counter ${ns1} "MPTcpExtMismatchPortSynRx")
+		count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$mis_syn_nr" ]; then
@@ -1645,7 +1629,7 @@ chk_add_nr()
 		fi
 
 		print_check "ack"
-		count=$(get_counter ${ns1} "MPTcpExtMismatchPortAckRx")
+		count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
 		elif [ "$count" != "$mis_ack_nr" ]; then
@@ -1667,7 +1651,7 @@ chk_add_tx_nr()
 	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
 
 	print_check "add TX"
-	count=$(get_counter ${ns1} "MPTcpExtAddAddrTx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
 	# if the test configured a short timeout tolerate greater then expected
@@ -1679,7 +1663,7 @@ chk_add_tx_nr()
 	fi
 
 	print_check "echo TX"
-	count=$(get_counter ${ns2} "MPTcpExtEchoAddTx")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtEchoAddTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$echo_tx_nr" ]; then
@@ -1717,7 +1701,7 @@ chk_rm_nr()
 	fi
 
 	print_check "rm"
-	count=$(get_counter ${addr_ns} "MPTcpExtRmAddr")
+	count=$(mptcp_lib_get_counter ${addr_ns} "MPTcpExtRmAddr")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$rm_addr_nr" ]; then
@@ -1727,13 +1711,13 @@ chk_rm_nr()
 	fi
 
 	print_check "rmsf"
-	count=$(get_counter ${subflow_ns} "MPTcpExtRmSubflow")
+	count=$(mptcp_lib_get_counter ${subflow_ns} "MPTcpExtRmSubflow")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ -n "$simult" ]; then
 		local cnt suffix
 
-		cnt=$(get_counter ${addr_ns} "MPTcpExtRmSubflow")
+		cnt=$(mptcp_lib_get_counter ${addr_ns} "MPTcpExtRmSubflow")
 
 		# in case of simult flush, the subflow removal count on each side is
 		# unreliable
@@ -1761,7 +1745,7 @@ chk_rm_tx_nr()
 	local rm_addr_tx_nr=$1
 
 	print_check "rm TX"
-	count=$(get_counter ${ns2} "MPTcpExtRmAddrTx")
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtRmAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$rm_addr_tx_nr" ]; then
@@ -1778,7 +1762,7 @@ chk_prio_nr()
 	local count
 
 	print_check "ptx"
-	count=$(get_counter ${ns1} "MPTcpExtMPPrioTx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$mp_prio_nr_tx" ]; then
@@ -1788,7 +1772,7 @@ chk_prio_nr()
 	fi
 
 	print_check "prx"
-	count=$(get_counter ${ns1} "MPTcpExtMPPrioRx")
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$mp_prio_nr_rx" ]; then
@@ -1917,7 +1901,7 @@ wait_attempt_fail()
 	while [ $time -lt $timeout_ms ]; do
 		local cnt
 
-		cnt=$(get_counter ${ns} "TcpAttemptFails")
+		cnt=$(mptcp_lib_get_counter ${ns} "TcpAttemptFails")
 
 		[ "$cnt" = 1 ] && return 1
 		time=$((time + 100))
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 91a17ae81cb8..0f5e34c76bb4 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -328,3 +328,19 @@ mptcp_lib_verify_listener_events() {
 mptcp_lib_is_v6() {
 	[ -z "${1##*:*}" ]
 }
+
+# $1: ns, $2: MIB counter
+mptcp_lib_get_counter() {
+	local ns="${1}"
+	local counter="${2}"
+	local count
+
+	count=$(ip netns exec ${ns} nstat -asz "${counter}" |
+		awk 'NR==1 {next} {print $2}')
+	if [ -z "${count}" ]; then
+		mptcp_lib_fail_if_expected_feature "${counter} counter"
+		return 1
+	fi
+
+	echo "${count}"
+}
-- 
2.35.3

Re: [PATCH mptcp-next v3 26/29] selftests: mptcp: add mptcp_lib_get_counter

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> The helper get_counter() in mptcp_join.sh and get_mib_counter() in
> mptcp_connect.sh have the same functionality, export get_counter() into
> mptcp_lib.sh and rename it as mptcp_lib_get_counter(). Use this new
> helper instead of get_counter() and get_mib_counter().

Good idea, looks good to me!

Reviewed-by: Matthieu Baerts <matttbe@kernel.org>

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

[PATCH mptcp-next v3 27/29] selftests: mptcp: add mptcp_lib_make_file

[Geliang Tang]

make_file() helper in mptcp_sockopt.sh and userspace_pm.sh are the same.
Export it into mptcp_lib.sh and rename it as mptcp_lib_kill_wait(). Use
it in both mptcp_connect.sh and mptcp_join.sh.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../selftests/net/mptcp/mptcp_connect.sh       |  3 +--
 .../testing/selftests/net/mptcp/mptcp_join.sh  |  3 +--
 tools/testing/selftests/net/mptcp/mptcp_lib.sh |  9 +++++++++
 .../selftests/net/mptcp/mptcp_sockopt.sh       | 18 ++++--------------
 .../selftests/net/mptcp/userspace_pm.sh        | 12 +-----------
 5 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 3b971d1617d8..dc4a1dd3566d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -592,9 +592,8 @@ make_file()
 	ksize=$((SIZE / 1024))
 	rem=$((SIZE - (ksize * 1024)))
 
-	dd if=/dev/urandom of="$name" bs=1024 count=$ksize 2> /dev/null
+	mptcp_lib_make_file $name 1024 $ksize
 	dd if=/dev/urandom conv=notrunc of="$name" bs=1 count=$rem 2> /dev/null
-	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
 
 	echo "Created $name (size $(du -b "$name")) containing data sent by $who"
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 26ea0919810f..f88168d66fdc 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1155,8 +1155,7 @@ make_file()
 	local who=$2
 	local size=$3
 
-	dd if=/dev/urandom of="$name" bs=1024 count=$size 2> /dev/null
-	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
+	mptcp_lib_make_file $name 1024 $size
 
 	print_info "Test file (size $size KB) for $who"
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 0f5e34c76bb4..7b0d03c40f89 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -344,3 +344,12 @@ mptcp_lib_get_counter() {
 
 	echo "${count}"
 }
+
+mptcp_lib_make_file() {
+	local name=$1
+	local bs=$2
+	local size=$3
+
+	dd if=/dev/urandom of="$name" bs=$bs count=$size 2> /dev/null
+	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index bfa744e350ef..39128fca99dd 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -245,18 +245,6 @@ do_transfer()
 	return 1
 }
 
-make_file()
-{
-	local name=$1
-	local who=$2
-	local size=$3
-
-	dd if=/dev/urandom of="$name" bs=1024 count=$size 2> /dev/null
-	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
-
-	echo "Created $name (size $size KB) containing data sent by $who"
-}
-
 do_mptcp_sockopt_tests()
 {
 	local lret=0
@@ -357,8 +345,10 @@ sout=$(mktemp)
 cin=$(mktemp)
 cout=$(mktemp)
 init
-make_file "$cin" "client" 1
-make_file "$sin" "server" 1
+mptcp_lib_make_file "$cin" 1024 1
+echo "Created $cin (size 1 KB) containing data sent by client"
+mptcp_lib_make_file "$sin" 1024 1
+echo "Created $sin (size 1 KB) containing data sent by server"
 trap cleanup EXIT
 
 run_tests $ns1 $ns2 10.0.1.1
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 98189b3f73dc..d5197d745171 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -150,22 +150,12 @@ print_title "Init"
 print_test "Created network namespaces ns1, ns2"
 test_pass
 
-make_file()
-{
-	# Store a chunk of data in a file to transmit over an MPTCP connection
-	local name=$1
-	local ksize=1
-
-	dd if=/dev/urandom of="$name" bs=2 count=$ksize 2> /dev/null
-	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
-}
-
 make_connection()
 {
 	if [ -z "$file" ]; then
 		file=$(mktemp)
 	fi
-	make_file "$file" "client"
+	mptcp_lib_make_file "$file" 2 1
 
 	local is_v6=$1
 	local app_port=$app4_port
-- 
2.35.3

Re: [PATCH mptcp-next v3 27/29] selftests: mptcp: add mptcp_lib_make_file

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> make_file() helper in mptcp_sockopt.sh and userspace_pm.sh are the same.
> Export it into mptcp_lib.sh and rename it as mptcp_lib_kill_wait(). Use
> it in both mptcp_connect.sh and mptcp_join.sh.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 3b971d1617d8..dc4a1dd3566d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -592,9 +592,8 @@ make_file()
>  	ksize=$((SIZE / 1024))
>  	rem=$((SIZE - (ksize * 1024)))
>  
> -	dd if=/dev/urandom of="$name" bs=1024 count=$ksize 2> /dev/null
> +	mptcp_lib_make_file $name 1024 $ksize
>  	dd if=/dev/urandom conv=notrunc of="$name" bs=1 count=$rem 2> /dev/null

Mmh, why were we doing that?

I guess we are missing something like "oflag=append" because this will
write "${rem}" bytes at the beginning of the file where there is already
some random bytes. It should write that at the end. Do you mind adding a
commit adding "oflag=append" please?

Then it means this should be done in the version of the "lib" as well
(an optional behaviour). That may increase a bit the complexity.

(Or we remove this second dd line (+ ${rem}) in a dedicated commit
because we were not using it correctly from the beginning and we are
fine like that (if I'm not mistaken)?)

> -	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
>  
>  	echo "Created $name (size $(du -b "$name")) containing data sent by $who"
>  }
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 26ea0919810f..f88168d66fdc 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1155,8 +1155,7 @@ make_file()
>  	local who=$2
>  	local size=$3
>  
> -	dd if=/dev/urandom of="$name" bs=1024 count=$size 2> /dev/null
> -	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
> +	mptcp_lib_make_file $name 1024 $size
>  
>  	print_info "Test file (size $size KB) for $who"
>  }
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 0f5e34c76bb4..7b0d03c40f89 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -344,3 +344,12 @@ mptcp_lib_get_counter() {
>  
>  	echo "${count}"
>  }
> +
> +mptcp_lib_make_file() {
> +	local name=$1
> +	local bs=$2
> +	local size=$3
> +
> +	dd if=/dev/urandom of="$name" bs=$bs count=$size 2> /dev/null
> +	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index bfa744e350ef..39128fca99dd 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -245,18 +245,6 @@ do_transfer()
>  	return 1
>  }
>  
> -make_file()
> -{
> -	local name=$1
> -	local who=$2
> -	local size=$3
> -
> -	dd if=/dev/urandom of="$name" bs=1024 count=$size 2> /dev/null
> -	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
> -
> -	echo "Created $name (size $size KB) containing data sent by $who"
> -}
> -
>  do_mptcp_sockopt_tests()
>  {
>  	local lret=0
> @@ -357,8 +345,10 @@ sout=$(mktemp)
>  cin=$(mktemp)
>  cout=$(mktemp)
>  init
> -make_file "$cin" "client" 1
> -make_file "$sin" "server" 1
> +mptcp_lib_make_file "$cin" 1024 1
> +echo "Created $cin (size 1 KB) containing data sent by client"
> +mptcp_lib_make_file "$sin" 1024 1
> +echo "Created $sin (size 1 KB) containing data sent by server"

Maybe easier to keep 'make_file()' helper to call mptcp_lib_make_file()
and print the message?

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

[PATCH mptcp-next v3 28/29] selftests: mptcp: add mptcp_lib_check_transfer

[Geliang Tang]

check_transfer() helper is defined both in mptcp_connect.sh and
mptcp_sockopt.sh, export it into mptcp_lib.sh and rename it with
mptcp_lib_ prefix. Use this new helper in both scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../selftests/net/mptcp/mptcp_connect.sh      | 29 ++-----------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 24 +++++++++++++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 28 +-----------------
 3 files changed, 27 insertions(+), 54 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index dc4a1dd3566d..0bd2392ae442 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -254,31 +254,6 @@ else
 	set_ethtool_flags "$ns4" ns4eth3 "$ethtool_args"
 fi
 
-print_file_err()
-{
-	ls -l "$1" 1>&2
-	echo "Trailing bytes are: "
-	tail -c 27 "$1"
-}
-
-check_transfer()
-{
-	local in=$1
-	local out=$2
-	local what=$3
-
-	cmp "$in" "$out" > /dev/null 2>&1
-	if [ $? -ne 0 ] ;then
-		echo "[ FAIL ] $what does not match (in, out):"
-		print_file_err "$in"
-		print_file_err "$out"
-
-		return 1
-	fi
-
-	return 0
-}
-
 check_mptcp_disabled()
 {
 	local disabled_ns="ns_disabled-$rndh"
@@ -483,9 +458,9 @@ do_transfer()
 		return 1
 	fi
 
-	check_transfer $sin $cout "file received by client"
+	mptcp_lib_check_transfer $sin $cout "file received by client"
 	retc=$?
-	check_transfer $cin $sout "file received by server"
+	mptcp_lib_check_transfer $cin $sout "file received by server"
 	rets=$?
 
 	local stat_synrx_now_l=$(mptcp_lib_get_counter "${listener_ns}" "MPTcpExtMPCapableSYNRX")
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 7b0d03c40f89..fba62cdef2cd 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -353,3 +353,27 @@ mptcp_lib_make_file() {
 	dd if=/dev/urandom of="$name" bs=$bs count=$size 2> /dev/null
 	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
 }
+
+print_file_err()
+{
+	ls -l "$1" 1>&2
+	echo "Trailing bytes are: "
+	tail -c 27 "$1"
+}
+
+mptcp_lib_check_transfer() {
+	local in=$1
+	local out=$2
+	local what=$3
+
+	cmp "$in" "$out" > /dev/null 2>&1
+	if [ $? -ne 0 ] ;then
+		echo "[ FAIL ] $what does not match (in, out):"
+		print_file_err "$in"
+		print_file_err "$out"
+
+		return 1
+	fi
+
+	return 0
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 39128fca99dd..aa4b9a4e6a56 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -135,32 +135,6 @@ check_mark()
 	return 0
 }
 
-print_file_err()
-{
-	ls -l "$1" 1>&2
-	echo "Trailing bytes are: "
-	tail -c 27 "$1"
-}
-
-check_transfer()
-{
-	local in=$1
-	local out=$2
-	local what=$3
-
-	cmp "$in" "$out" > /dev/null 2>&1
-	if [ $? -ne 0 ] ;then
-		echo "[ FAIL ] $what does not match (in, out):"
-		print_file_err "$in"
-		print_file_err "$out"
-		ret=1
-
-		return 1
-	fi
-
-	return 0
-}
-
 do_transfer()
 {
 	local listener_ns="$1"
@@ -232,7 +206,7 @@ do_transfer()
 		check_mark $connector_ns 4 || retc=1
 	fi
 
-	check_transfer $cin $sout "file received by server"
+	mptcp_lib_check_transfer $cin $sout "file received by server"
 	rets=$?
 
 	mptcp_lib_result_code "${retc}" "mark ${ip}"
-- 
2.35.3

Re: [PATCH mptcp-next v3 28/29] selftests: mptcp: add mptcp_lib_check_transfer

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> check_transfer() helper is defined both in mptcp_connect.sh and
> mptcp_sockopt.sh, export it into mptcp_lib.sh and rename it with
> mptcp_lib_ prefix. Use this new helper in both scripts.

Good idea!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 7b0d03c40f89..fba62cdef2cd 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -353,3 +353,27 @@ mptcp_lib_make_file() {
>  	dd if=/dev/urandom of="$name" bs=$bs count=$size 2> /dev/null
>  	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
>  }
> +
> +print_file_err()

Please prefix it with "mptcp_lib_" + add a comment:

  # $1: file

> +{
> +	ls -l "$1" 1>&2

Please use {} around variables: "${1}"

(like in the rest of the file)

> +	echo "Trailing bytes are: "
> +	tail -c 27 "$1"
> +}
> +
> +mptcp_lib_check_transfer() {

Please add a comment:

  # $1: input file ; $2: output file ; $3: what kind of file

> +	local in=$1

Please use {} and quotes around variables: "${1}". Same below.

> +	local out=$2
> +	local what=$3
> +
> +	cmp "$in" "$out" > /dev/null 2>&1
> +	if [ $? -ne 0 ] ;then

Small detail: probably better to do this (I guess Shellcheck would
complain if it is not done like that)

  if ! cmp "$in" "$out" > /dev/null 2>&1; then

> +		echo "[ FAIL ] $what does not match (in, out):"
> +		print_file_err "$in"
> +		print_file_err "$out"
> +
> +		return 1
> +	fi
> +
> +	return 0
> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 39128fca99dd..aa4b9a4e6a56 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -135,32 +135,6 @@ check_mark()
>  	return 0
>  }
>  
> -print_file_err()
> -{
> -	ls -l "$1" 1>&2
> -	echo "Trailing bytes are: "
> -	tail -c 27 "$1"
> -}
> -
> -check_transfer()
> -{
> -	local in=$1
> -	local out=$2
> -	local what=$3
> -
> -	cmp "$in" "$out" > /dev/null 2>&1
> -	if [ $? -ne 0 ] ;then
> -		echo "[ FAIL ] $what does not match (in, out):"
> -		print_file_err "$in"
> -		print_file_err "$out"
> -		ret=1

Could you add a comment in the commit message: here it is OK to drop
'ret=1' because it will be set in run_tests() anyway (if I'm not mistaken).

Otherwise a reviewer could think the test is no longer marked as failed
in case of error and he might ask the question or take time to find the
answer and we want to avoid that ;)

> -
> -		return 1
> -	fi
> -
> -	return 0
> -}
> -
>  do_transfer()
>  {
>  	local listener_ns="$1"
> @@ -232,7 +206,7 @@ do_transfer()
>  		check_mark $connector_ns 4 || retc=1
>  	fi
>  
> -	check_transfer $cin $sout "file received by server"
> +	mptcp_lib_check_transfer $cin $sout "file received by server"
>  	rets=$?
>  
>  	mptcp_lib_result_code "${retc}" "mark ${ip}"

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

[PATCH mptcp-next v3 29/29] selftests: mptcp: add mptcp_lib_wait_local_port_listen

[Geliang Tang]

wait_local_port_listen() helper is defined in diag.sh, mptcp_connect.sh,
mptcp_join.sh and simult_flows.sh, export it into mptcp_lib.sh and
rename it with mptcp_lib_ prefix. Use this new helper in all these
scripts.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/diag.sh     | 23 +++----------------
 .../selftests/net/mptcp/mptcp_connect.sh      | 19 +--------------
 .../testing/selftests/net/mptcp/mptcp_join.sh | 20 +---------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 18 +++++++++++++++
 .../selftests/net/mptcp/simult_flows.sh       | 19 +--------------
 5 files changed, 24 insertions(+), 75 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 85a8ee9395b3..95b498efacd1 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -182,23 +182,6 @@ chk_msk_inuse()
 	__chk_nr get_msk_inuse $expected "$msg" 0
 }
 
-# $1: ns, $2: port
-wait_local_port_listen()
-{
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex i
-
-	port_hex="$(printf "%04X" "${port}")"
-	for i in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
-}
-
 wait_connected()
 {
 	local listener_ns="${1}"
@@ -222,7 +205,7 @@ echo "a" | \
 		ip netns exec $ns \
 			./mptcp_connect -p 10000 -l -t ${timeout_poll} -w 20 \
 				0.0.0.0 >/dev/null &
-wait_local_port_listen $ns 10000
+mptcp_lib_wait_local_port_listen $ns 10000
 chk_msk_nr 0 "no msk on netns creation"
 chk_msk_listen 10000
 
@@ -245,7 +228,7 @@ echo "a" | \
 		ip netns exec $ns \
 			./mptcp_connect -p 10001 -l -s TCP -t ${timeout_poll} -w 20 \
 				0.0.0.0 >/dev/null &
-wait_local_port_listen $ns 10001
+mptcp_lib_wait_local_port_listen $ns 10001
 echo "b" | \
 	timeout ${timeout_test} \
 		ip netns exec $ns \
@@ -266,7 +249,7 @@ for I in `seq 1 $NR_CLIENTS`; do
 				./mptcp_connect -p $((I+10001)) -l -w 20 \
 					-t ${timeout_poll} 0.0.0.0 >/dev/null &
 done
-wait_local_port_listen $ns $((NR_CLIENTS + 10001))
+mptcp_lib_wait_local_port_listen $ns $((NR_CLIENTS + 10001))
 
 for I in `seq 1 $NR_CLIENTS`; do
 	echo "b" | \
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 0bd2392ae442..d6e3ef786694 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -310,23 +310,6 @@ do_ping()
 	return 0
 }
 
-# $1: ns, $2: port
-wait_local_port_listen()
-{
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex i
-
-	port_hex="$(printf "%04X" "${port}")"
-	for i in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
-}
-
 do_transfer()
 {
 	local listener_ns="$1"
@@ -408,7 +391,7 @@ do_transfer()
 				$extra_args $local_addr < "$sin" > "$sout" &
 	local spid=$!
 
-	wait_local_port_listen "${listener_ns}" "${port}"
+	mptcp_lib_wait_local_port_listen "${listener_ns}" "${port}"
 
 	local start
 	start=$(date +%s%3N)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f88168d66fdc..d11a2289f8f5 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -577,24 +577,6 @@ link_failure()
 	done
 }
 
-# $1: ns, $2: port
-wait_local_port_listen()
-{
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex
-	port_hex="$(printf "%04X" "${port}")"
-
-	local i
-	for i in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
-}
-
 rm_addr_count()
 {
 	mptcp_lib_get_counter "${1}" "MPTcpExtRmAddr"
@@ -1073,7 +1055,7 @@ do_transfer()
 	fi
 	local spid=$!
 
-	wait_local_port_listen "${listener_ns}" "${port}"
+	mptcp_lib_wait_local_port_listen "${listener_ns}" "${port}"
 
 	extra_cl_args="$extra_args $extra_cl_args"
 	if [ "$test_linkfail" -eq 0 ];then
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index fba62cdef2cd..1fb0950ce44d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -377,3 +377,21 @@ mptcp_lib_check_transfer() {
 
 	return 0
 }
+
+# $1: ns, $2: port
+mptcp_lib_wait_local_port_listen() {
+	local listener_ns="${1}"
+	local port="${2}"
+
+	local port_hex
+	port_hex="$(printf "%04X" "${port}")"
+
+	local i
+	for i in $(seq 10); do
+		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
+			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) \
+			     {rc=0; exit}} END {exit rc}" &&
+			break
+		sleep 0.1
+	done
+}
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index ce9203b817f8..ae8ad5d6fb9d 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -123,23 +123,6 @@ setup()
 	grep -q ' kmemleak_init$\| lockdep_init$\| kasan_init$\| prove_locking$' /proc/kallsyms && slack=$((slack+550))
 }
 
-# $1: ns, $2: port
-wait_local_port_listen()
-{
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex i
-
-	port_hex="$(printf "%04X" "${port}")"
-	for i in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
-}
-
 do_transfer()
 {
 	local cin=$1
@@ -179,7 +162,7 @@ do_transfer()
 				0.0.0.0 < "$sin" > "$sout" &
 	local spid=$!
 
-	wait_local_port_listen "${ns3}" "${port}"
+	mptcp_lib_wait_local_port_listen "${ns3}" "${port}"
 
 	timeout ${timeout_test} \
 		ip netns exec ${ns1} \
-- 
2.35.3

Re: selftests: mptcp: add mptcp_lib_wait_local_port_listen: 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/4696470427795456
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4696470427795456/summary/summary.txt

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

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

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

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


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: selftests: mptcp: add mptcp_lib_wait_local_port_listen: Build Failure

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/de9b9a8f31290a56012b2f2c925988e86ee83b3b.1695631132.git.geliang.tang@suse.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/6344241510

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7489646e8ff0

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: selftests: mptcp: add mptcp_lib_wait_local_port_listen: 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/5872240877633536
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5872240877633536/summary/summary.txt

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

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

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/6122316556402688
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6122316556402688/summary/summary.txt

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


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 v3 29/29] selftests: mptcp: add mptcp_lib_wait_local_port_listen

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> wait_local_port_listen() helper is defined in diag.sh, mptcp_connect.sh,
> mptcp_join.sh and simult_flows.sh, export it into mptcp_lib.sh and
> rename it with mptcp_lib_ prefix. Use this new helper in all these
> scripts.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh     | 23 +++----------------
>  .../selftests/net/mptcp/mptcp_connect.sh      | 19 +--------------
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 20 +---------------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 18 +++++++++++++++
>  .../selftests/net/mptcp/simult_flows.sh       | 19 +--------------
>  5 files changed, 24 insertions(+), 75 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 85a8ee9395b3..95b498efacd1 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -182,23 +182,6 @@ chk_msk_inuse()
>  	__chk_nr get_msk_inuse $expected "$msg" 0
>  }
>  
> -# $1: ns, $2: port
> -wait_local_port_listen()
> -{
> -	local listener_ns="${1}"
> -	local port="${2}"
> -
> -	local port_hex i
> -
> -	port_hex="$(printf "%04X" "${port}")"
> -	for i in $(seq 10); do
> -		ip netns exec "${listener_ns}" cat /proc/net/tcp | \

Here we were not looking at IPv6 (tcp6) but I guess that's OK because we
only have IPv4 connections here in diag.sh, right?)

Please mention that in the commit message to avoid further questions
from the reviewers when there are ambiguities that are not explained.

Other than that, it looks good to me!

Reviewed-by: Matthieu Baerts <matttbe@kernel.org>

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

Re: [PATCH mptcp-next v3 00/29] userspace pm enhancements

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> v3:
>  - drop patch 4 in v2
>  - invert patch 10 and 11 in v2
>  - rebased

Thank you for the v3! And again, sorry for the delays!

I think patches 1-11 + 13-14 are OK to be applied (with two small
modifications for patch 7 and 11 that I can do when applying these patches):

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Please note that patch 13 will go to "Fixes for -net". Patch 14 can stay
in "Features for net-next" due to other dependences.

I have some comments about patch 12 and 16: on the wire, sending a
remove address for ID 0 or actually removing the subflow(s) with ID 0
should not trigger any RST (+MP_TCPRST). I guess the kernel is not
behaving correctly if you observe them during the test (that's good you
added that in the verification!). It means we will need more patches for
the kernel side not to send these RST in these cases.

If you send a v4, please move patch 15 (mptcp: userspace pm remove id 0
address) to the top as this one is for -net. It is fine to keep patch 16
where it is if it depends on other features.

> 1-7: some small cleanups, v3
> 8-9: address #428, add mptcpi_subflows_total
> 10-16: address #379, #391, userspace pm remove id 0 subflow & address, v11
> 17-19: address #403, add refcont for address entry
> 20: add userspace fullmesh tests
> 21-29: seltests cleanups

Note: I didn't review patches 17-29 yet. I will try to do that ASAP (but
very likely, not before the end of next week :-/ ).

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

Re: [PATCH mptcp-next v3 00/29] userspace pm enhancements

[Matthieu Baerts]

Hi Geliang,

On 28/09/2023 22:54, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 25/09/2023 10:41, Geliang Tang wrote:
>> v3:
>>  - drop patch 4 in v2
>>  - invert patch 10 and 11 in v2
>>  - rebased
> 
> Thank you for the v3! And again, sorry for the delays!
> 
> I think patches 1-11 + 13-14 are OK to be applied (with two small
> modifications for patch 7 and 11 that I can do when applying these patches):
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> Please note that patch 13 will go to "Fixes for -net". Patch 14 can stay
> in "Features for net-next" due to other dependences.

I just did that:

New patches for t/upstream-net and t/upstream:
- 505a33d76a32: mptcp: userspace pm allow creating id 0 subflow
- Results: 05758f924fe3..999b9f7e47a5 (export-net)

New patches for t/upstream:
- 1cb016e94837: mptcp: drop useless ssk in pm_subflow_check_next
- 450deaf6dd71: mptcp: use mptcp_check_fallback helper
- 6df74a0ced73: mptcp: use mptcp_get_ext helper
- 03649553fd68: mptcp: move sk assignment statement ahead
- 6bc1e5144525: mptcp: define more local variables sk
- b7e3cc69d9fe: selftests: mptcp: sockopt: drop mptcp_connect var
- 14c099dbd7a7: selftests: mptcp: display simult in extra_msg
- 7d9e508e1117: mptcp: add mptcpi_subflows_total counter
- af6ed0819599: selftests: mptcp: add evts_get_info helper
- 9177dc0b474c: selftests: mptcp: add chk_subflows_total helper
- bc1edd827cfa: selftests: mptcp: update userspace pm test helpers
- 0dd61d4e14e8: selftests: mptcp: userspace pm create id 0 subflow
- Results: 3be1c8ce544a..516baae7a220 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230928T211416
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230928T213439

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

Re: [PATCH mptcp-next v3 00/29] userspace pm enhancements

[Matthieu Baerts]

Hi Geliang,

On 28/09/2023 23:37, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/09/2023 22:54, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 25/09/2023 10:41, Geliang Tang wrote:
>>> v3:
>>>  - drop patch 4 in v2
>>>  - invert patch 10 and 11 in v2
>>>  - rebased
>>
>> Thank you for the v3! And again, sorry for the delays!
>>
>> I think patches 1-11 + 13-14 are OK to be applied (with two small
>> modifications for patch 7 and 11 that I can do when applying these patches):
>>
>> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>
>> Please note that patch 13 will go to "Fixes for -net". Patch 14 can stay
>> in "Features for net-next" due to other dependences.
> 
> I just did that:
> 
> New patches for t/upstream-net and t/upstream:
> - 505a33d76a32: mptcp: userspace pm allow creating id 0 subflow
> - Results: 05758f924fe3..999b9f7e47a5 (export-net)
> 
> New patches for t/upstream:
> - 1cb016e94837: mptcp: drop useless ssk in pm_subflow_check_next
> - 450deaf6dd71: mptcp: use mptcp_check_fallback helper
> - 6df74a0ced73: mptcp: use mptcp_get_ext helper
> - 03649553fd68: mptcp: move sk assignment statement ahead
> - 6bc1e5144525: mptcp: define more local variables sk
> - b7e3cc69d9fe: selftests: mptcp: sockopt: drop mptcp_connect var
> - 14c099dbd7a7: selftests: mptcp: display simult in extra_msg
> - 7d9e508e1117: mptcp: add mptcpi_subflows_total counter
> - af6ed0819599: selftests: mptcp: add evts_get_info helper
> - 9177dc0b474c: selftests: mptcp: add chk_subflows_total helper
> - bc1edd827cfa: selftests: mptcp: update userspace pm test helpers
> - 0dd61d4e14e8: selftests: mptcp: userspace pm create id 0 subflow
> - Results: 3be1c8ce544a..516baae7a220 (export)

I think these patches are introducing regressions because these tests
are now regularly failing on the public CI:

  - 112 - mptcp_join: userspace pm add & remove address
  - 113 - mptcp_join: userspace pm create destroy subflow

I didn't check in more details but I see they are failing since
export/20230928T213439 and only when running with a debug kconfig (slow
environment), e.g.:

  - https://cirrus-ci.com/task/5444189572300800
  -
https://api.cirrus-ci.com/v1/artifact/task/5444189572300800/summary/summary.txt

Do you mind having a look?

Maybe the connection has been closed when looking at the number of subflows?
Also, it looks like the helper looking at 'subflows_total' with older
versions ss is producing a lot of debug output that can be muted.

Cheers,
Matt

Re: [PATCH mptcp-next v3 00/29] userspace pm enhancements

[Geliang Tang]

On Thu, Oct 05, 2023 at 05:53:13PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/09/2023 23:37, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 28/09/2023 22:54, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 25/09/2023 10:41, Geliang Tang wrote:
> >>> v3:
> >>>  - drop patch 4 in v2
> >>>  - invert patch 10 and 11 in v2
> >>>  - rebased
> >>
> >> Thank you for the v3! And again, sorry for the delays!
> >>
> >> I think patches 1-11 + 13-14 are OK to be applied (with two small
> >> modifications for patch 7 and 11 that I can do when applying these patches):
> >>
> >> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >>
> >> Please note that patch 13 will go to "Fixes for -net". Patch 14 can stay
> >> in "Features for net-next" due to other dependences.
> > 
> > I just did that:
> > 
> > New patches for t/upstream-net and t/upstream:
> > - 505a33d76a32: mptcp: userspace pm allow creating id 0 subflow
> > - Results: 05758f924fe3..999b9f7e47a5 (export-net)
> > 
> > New patches for t/upstream:
> > - 1cb016e94837: mptcp: drop useless ssk in pm_subflow_check_next
> > - 450deaf6dd71: mptcp: use mptcp_check_fallback helper
> > - 6df74a0ced73: mptcp: use mptcp_get_ext helper
> > - 03649553fd68: mptcp: move sk assignment statement ahead
> > - 6bc1e5144525: mptcp: define more local variables sk
> > - b7e3cc69d9fe: selftests: mptcp: sockopt: drop mptcp_connect var
> > - 14c099dbd7a7: selftests: mptcp: display simult in extra_msg
> > - 7d9e508e1117: mptcp: add mptcpi_subflows_total counter
> > - af6ed0819599: selftests: mptcp: add evts_get_info helper
> > - 9177dc0b474c: selftests: mptcp: add chk_subflows_total helper
> > - bc1edd827cfa: selftests: mptcp: update userspace pm test helpers
> > - 0dd61d4e14e8: selftests: mptcp: userspace pm create id 0 subflow
> > - Results: 3be1c8ce544a..516baae7a220 (export)
> 
> I think these patches are introducing regressions because these tests
> are now regularly failing on the public CI:
> 
>   - 112 - mptcp_join: userspace pm add & remove address
>   - 113 - mptcp_join: userspace pm create destroy subflow
> 
> I didn't check in more details but I see they are failing since
> export/20230928T213439 and only when running with a debug kconfig (slow
> environment), e.g.:
> 
>   - https://cirrus-ci.com/task/5444189572300800
>   -
> https://api.cirrus-ci.com/v1/artifact/task/5444189572300800/summary/summary.txt
> 
> Do you mind having a look?

Sure, I'll look at it.

-Geliang

> 
> Maybe the connection has been closed when looking at the number of subflows?
> Also, it looks like the helper looking at 'subflows_total' with older
> versions ss is producing a lot of debug output that can be muted.
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 00/29] userspace pm enhancements

[Matthieu Baerts]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:

(...)

> Geliang Tang (29):
>   mptcp: drop useless ssk in pm_subflow_check_next
>   mptcp: use mptcp_check_fallback helper
>   mptcp: use mptcp_get_ext helper
>   mptcp: move sk assignment statement ahead
>   mptcp: define more local variables sk
>   selftests: mptcp: sockopt: drop mptcp_connect var
>   selftests: mptcp: display simult in extra_msg
>   mptcp: add mptcpi_subflows_total counter
>   selftests: mptcp: add evts_get_info helper
>   selftests: mptcp: add chk_subflows_total helper
>   selftests: mptcp: update userspace pm test helpers
>   selftests: mptcp: userspace pm remove id 0 subflow
>   mptcp: userspace pm allow creating id 0 subflow
>   selftests: mptcp: userspace pm create id 0 subflow
>   mptcp: userspace pm remove id 0 address
>   selftests: mptcp: userspace pm remove id 0 address
>   mptcp: add userspace_pm_get_entry helper
>   mptcp: add userspace pm addr entry refcount
>   mptcp: add netlink pm addr entry refcount
>   selftests: mptcp: add userspace pm fullmesh tests
>   selftests: mptcp: add mptcp_lib_kill_wait
>   selftests: mptcp: add mptcp_lib_evts_*
>   selftests: mptcp: userspace: print colored results
>   selftests: mptcp: add mptcp_lib_verify_listener_events
>   selftests: mptcp: add mptcp_lib_is_v6
>   selftests: mptcp: add mptcp_lib_get_counter
>   selftests: mptcp: add mptcp_lib_make_file
>   selftests: mptcp: add mptcp_lib_check_transfer
>   selftests: mptcp: add mptcp_lib_wait_local_port_listen


These patches are still listed in patchwork:

13397525: [mptcp-next,v3,17/29] mptcp: add userspace_pm_get_entry helper
13397526: [mptcp-next,v3,18/29] mptcp: add userspace pm addr entry refcount
13397527: [mptcp-next,v3,19/29] mptcp: add netlink pm addr entry refcount
13397528: [mptcp-next,v3,20/29] selftests: mptcp: add userspace pm
fullmesh tests
13397530: [mptcp-next,v3,22/29] selftests: mptcp: add mptcp_lib_evts_*
13397531: [mptcp-next,v3,23/29] selftests: mptcp: userspace: print

@Geliang: just to know what's the status, do you plan to still work on them?

No hurry at all (especially now that net-next is closed) but it is just
to know what's the current status ;-)

Cheers,
Matt