All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use
@ 2022-11-03 11:06 menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen() menglong8.dong
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: menglong8.dong @ 2022-11-03 11:06 UTC (permalink / raw)
  To: mathew.j.martineau, matthieu.baerts; +Cc: mptcp, Menglong Dong

From: Menglong Dong <imagedong@tencent.com>

In the 1th patch, we do some code cleanup with replease 'sock->sk'
with 'sk'. In the 2th patch, we add statistics for mptcp socket in
use. In the 3th patch, we make mptcp_connect can exit when receive
'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
for this commit.

Changes since v5:
- introduce MPTCP_INUSE flag to store if msk is in use, as I find
  that it's not correct to check is a msk is in use by
  !sk_unhashed(sk) in mptcp_destroy_common(), because the token
  can be release in mptcp_check_fastclose()
- add the 3th patch
- reuse __chk_nr in 4th patch

Changes since v4:
- rebase to solve merge conflict

Changes since v3:
- rename MPTCP_DESTROIED to MPTCP_DESTROYED in the 2th patch

Changes since v2:
- add testing

Changes since v1:
- split the code cleanup into the 1th patch.
- decrease the statistics for listening mptcp socket inuse with
  mptcp_listen_inuse_dec()
- add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
  called on the msk. For fallback case, we need to decrease the
  statistics only once, and mptcp_destroy_common() can be called
  more than once.

Menglong Dong (4):
  mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
  mptcp: add statistics for mptcp socket in use
  selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1
  selftest: mptcp: add test for mptcp socket in use

 net/mptcp/protocol.c                          | 21 ++++++---
 net/mptcp/protocol.h                          | 13 +++++
 net/mptcp/subflow.c                           |  1 +
 tools/testing/selftests/net/mptcp/diag.sh     | 47 +++++++++++++++++--
 .../selftests/net/mptcp/mptcp_connect.c       |  4 +-
 5 files changed, 72 insertions(+), 14 deletions(-)

-- 
2.37.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH mptcp-next v6 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
  2022-11-03 11:06 [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use menglong8.dong
@ 2022-11-03 11:06 ` menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 2/4] mptcp: add statistics for mptcp socket in use menglong8.dong
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: menglong8.dong @ 2022-11-03 11:06 UTC (permalink / raw)
  To: mathew.j.martineau, matthieu.baerts; +Cc: mptcp, Menglong Dong

From: Menglong Dong <imagedong@tencent.com>

'sock->sk' is used frequently in mptcp_listen(). Therefore, we can
introduce the 'sk' and replace 'sock->sk' with it.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/mptcp/protocol.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ddeb8b36a677..151d22c91b7e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3653,12 +3653,13 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 static int mptcp_listen(struct socket *sock, int backlog)
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
+	struct sock *sk = sock->sk;
 	struct socket *ssock;
 	int err;
 
 	pr_debug("msk=%p", msk);
 
-	lock_sock(sock->sk);
+	lock_sock(sk);
 	ssock = __mptcp_nmpc_socket(msk);
 	if (!ssock) {
 		err = -EINVAL;
@@ -3666,16 +3667,16 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	}
 
 	mptcp_token_destroy(msk);
-	inet_sk_state_store(sock->sk, TCP_LISTEN);
-	sock_set_flag(sock->sk, SOCK_RCU_FREE);
+	inet_sk_state_store(sk, TCP_LISTEN);
+	sock_set_flag(sk, SOCK_RCU_FREE);
 
 	err = ssock->ops->listen(ssock, backlog);
-	inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
+	inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
 	if (!err)
-		mptcp_copy_inaddrs(sock->sk, ssock->sk);
+		mptcp_copy_inaddrs(sk, ssock->sk);
 
 unlock:
-	release_sock(sock->sk);
+	release_sock(sk);
 	return err;
 }
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH mptcp-next v6 2/4] mptcp: add statistics for mptcp socket in use
  2022-11-03 11:06 [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen() menglong8.dong
@ 2022-11-03 11:06 ` menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 3/4] selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1 menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 4/4] selftest: mptcp: add test for mptcp socket in use menglong8.dong
  3 siblings, 0 replies; 9+ messages in thread
From: menglong8.dong @ 2022-11-03 11:06 UTC (permalink / raw)
  To: mathew.j.martineau, matthieu.baerts; +Cc: mptcp, Menglong Dong

From: Menglong Dong <imagedong@tencent.com>

Do the statistics of mptcp socket in use with sock_prot_inuse_add().
Therefore, we can get the count of used mptcp socket from
/proc/net/protocols:

& cat /proc/net/protocols
protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v6:
- introduce the 'MPTCP_INUSE' flag and check if msk is in use by it

v5:
- rebase to solve merge conflict

v4:
- rename MPTCP_DESTROIED to MPTCP_DESTROYED

v2:
- decrease the statistics for listening mptcp socket inuse with
  mptcp_listen_inuse_dec()
- add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
  called on the msk. For fallback case, we need to decrease the
  statistics only once, and mptcp_destroy_common() can be called
  more than once.
---
 net/mptcp/protocol.c |  8 +++++++-
 net/mptcp/protocol.h | 13 +++++++++++++
 net/mptcp/subflow.c  |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 151d22c91b7e..0abdba83a576 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3068,6 +3068,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	msk->snd_una = msk->write_seq;
 	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
 	msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
+	clear_bit(MPTCP_INUSE, &msk->flags);
 	mptcp_init_sched(msk, mptcp_sk(sk)->sched);
 
 	if (mp_opt->suboptions & OPTIONS_MPTCP_MPC) {
@@ -3177,6 +3178,8 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
 	skb_rbtree_purge(&msk->out_of_order_queue);
 	mptcp_data_unlock(sk);
 
+	mptcp_inuse_dec(sk);
+
 	/* move all the rx fwd alloc into the sk_mem_reclaim_final in
 	 * inet_sock_destruct() will dispose it
 	 */
@@ -3540,6 +3543,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 
 	mptcp_token_destroy(msk);
 	inet_sk_state_store(sk, TCP_SYN_SENT);
+	mptcp_inuse_inc(sk);
 	subflow = mptcp_subflow_ctx(ssock->sk);
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
@@ -3672,8 +3676,10 @@ static int mptcp_listen(struct socket *sock, int backlog)
 
 	err = ssock->ops->listen(ssock, backlog);
 	inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
-	if (!err)
+	if (!err) {
+		mptcp_inuse_inc(sk);
 		mptcp_copy_inaddrs(sk, ssock->sk);
+	}
 
 unlock:
 	release_sock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 8f48f881adf8..d6d2ab17129b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,6 +116,7 @@
 #define MPTCP_WORK_EOF		3
 #define MPTCP_FALLBACK_DONE	4
 #define MPTCP_WORK_CLOSE_SUBFLOW 5
+#define MPTCP_INUSE		6
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
@@ -383,6 +384,18 @@ static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk)
 	return list_first_entry_or_null(&msk->rtx_queue, struct mptcp_data_frag, list);
 }
 
+static inline void mptcp_inuse_inc(const struct sock *sk)
+{
+	if (!test_and_set_bit(MPTCP_INUSE, &mptcp_sk(sk)->flags))
+		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
+}
+
+static inline void mptcp_inuse_dec(const struct sock *sk)
+{
+	if (test_and_clear_bit(MPTCP_INUSE, &mptcp_sk(sk)->flags))
+		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+}
+
 struct csum_pseudo_header {
 	__be64 data_seq;
 	__be32 subflow_seq;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 02a54d59697b..c5934140f3f3 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -749,6 +749,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
 			mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
 			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
+			mptcp_inuse_inc(new_msk);
 			ctx->conn = new_msk;
 			new_msk = NULL;
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH mptcp-next v6 3/4] selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1
  2022-11-03 11:06 [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen() menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 2/4] mptcp: add statistics for mptcp socket in use menglong8.dong
@ 2022-11-03 11:06 ` menglong8.dong
  2022-11-03 11:06 ` [PATCH mptcp-next v6 4/4] selftest: mptcp: add test for mptcp socket in use menglong8.dong
  3 siblings, 0 replies; 9+ messages in thread
From: menglong8.dong @ 2022-11-03 11:06 UTC (permalink / raw)
  To: mathew.j.martineau, matthieu.baerts; +Cc: mptcp, Menglong Dong

From: Menglong Dong <imagedong@tencent.com>

For now, mptcp_connect won't exit after receiving the 'SIGUSR1' signal
if '-r' is set. Fix this by skipping poll and sleep in copyfd_io_poll()
if 'quit' is set.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index e54653ea2ed4..518c6d653c0e 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -586,7 +586,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd, bool *in_closed_after
 		char rbuf[8192];
 		ssize_t len;
 
-		if (fds.events == 0)
+		if (fds.events == 0 || quit)
 			break;
 
 		switch (poll(&fds, 1, poll_timeout)) {
@@ -692,7 +692,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd, bool *in_closed_after
 	}
 
 	/* leave some time for late join/announce */
-	if (cfg_remove)
+	if (cfg_remove && !quit)
 		usleep(cfg_wait);
 
 	return 0;
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH mptcp-next v6 4/4] selftest: mptcp: add test for mptcp socket in use
  2022-11-03 11:06 [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use menglong8.dong
                   ` (2 preceding siblings ...)
  2022-11-03 11:06 ` [PATCH mptcp-next v6 3/4] selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1 menglong8.dong
@ 2022-11-03 11:06 ` menglong8.dong
  2022-11-03 13:21   ` selftest: mptcp: add test for mptcp socket in use: Tests Results MPTCP CI
  3 siblings, 1 reply; 9+ messages in thread
From: menglong8.dong @ 2022-11-03 11:06 UTC (permalink / raw)
  To: mathew.j.martineau, matthieu.baerts; +Cc: mptcp, Menglong Dong

From: Menglong Dong <imagedong@tencent.com>

Add the function chk_msk_inuse() to diag.sh, which is used to check the
statistics of mptcp socket in use. As mptcp socket in listen state will
be closed randomly after 'accept', we need to get the count of listening
mptcp socket through 'ss' command.

All tests pass.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v6:
- reuse __chk_nr to check the number of msk in use
---
 tools/testing/selftests/net/mptcp/diag.sh | 47 ++++++++++++++++++++---
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 515859a5168b..9994cb23f8a0 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -36,15 +36,20 @@ if [ $? -ne 0 ];then
 	exit $ksft_skip
 fi
 
+get_msk_inuse()
+{
+	ip netns exec $ns cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
+}
+
 __chk_nr()
 {
-	local condition="$1"
+	local command="$1"
 	local expected=$2
 	local msg nr
 
 	shift 2
 	msg=$*
-	nr=$(ss -inmHMN $ns | $condition)
+	nr=$(eval $command)
 
 	printf "%-50s" "$msg"
 	if [ $nr != $expected ]; then
@@ -56,9 +61,17 @@ __chk_nr()
 	test_cnt=$((test_cnt+1))
 }
 
+__chk_msk_nr()
+{
+	local condition=$1
+	shift 1
+
+	__chk_nr "ss -inmHMN $ns | $condition" $*
+}
+
 chk_msk_nr()
 {
-	__chk_nr "grep -c token:" $*
+	__chk_msk_nr "grep -c token:" $*
 }
 
 wait_msk_nr()
@@ -96,12 +109,12 @@ wait_msk_nr()
 
 chk_msk_fallback_nr()
 {
-		__chk_nr "grep -c fallback" $*
+		__chk_msk_nr "grep -c fallback" $*
 }
 
 chk_msk_remote_key_nr()
 {
-		__chk_nr "grep -c remote_key" $*
+		__chk_msk_nr "grep -c remote_key" $*
 }
 
 __chk_listen()
@@ -141,6 +154,25 @@ chk_msk_listen()
 	nr=$(ss -Ml $filter | wc -l)
 }
 
+chk_msk_inuse()
+{
+	local expected=$1
+	local listen_nr
+
+	listen_nr=$(ss -N $ns -Ml | grep -c LISTEN)
+	expected=$(($expected+$listen_nr))
+	shift 1
+
+	for i in $(seq 10); do
+		if [ $(get_msk_inuse) -eq $expected ];then
+			break
+		fi
+		sleep 0.1
+	done
+
+	__chk_nr get_msk_inuse $expected $*
+}
+
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -194,8 +226,10 @@ wait_connected $ns 10000
 chk_msk_nr 2 "after MPC handshake "
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
+chk_msk_inuse 2 "chk 2 msk in use"
 flush_pids
 
+chk_msk_inuse 0 "chk 0 msk in use after flush"
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -231,6 +265,9 @@ for I in `seq 1 $NR_CLIENTS`; do
 done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
+chk_msk_inuse $((NR_CLIENTS*2)) "chk many msk in use"
 flush_pids
 
+chk_msk_inuse 0 "chk 0 msk in use after flush"
+
 exit $ret
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: selftest: mptcp: add test for mptcp socket in use: Tests Results
  2022-11-03 11:06 ` [PATCH mptcp-next v6 4/4] selftest: mptcp: add test for mptcp socket in use menglong8.dong
@ 2022-11-03 13:21   ` MPTCP CI
  2022-11-03 17:34     ` Matthieu Baerts
  0 siblings, 1 reply; 9+ messages in thread
From: MPTCP CI @ 2022-11-03 13:21 UTC (permalink / raw)
  To: Menglong Dong; +Cc: mptcp

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4909154062565376
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt

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

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


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)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: selftest: mptcp: add test for mptcp socket in use: Tests Results
  2022-11-03 13:21   ` selftest: mptcp: add test for mptcp socket in use: Tests Results MPTCP CI
@ 2022-11-03 17:34     ` Matthieu Baerts
  2022-11-04  0:04       ` Mat Martineau
  2022-11-04  2:58       ` [Internet]Re: " imagedong(董梦龙)
  0 siblings, 2 replies; 9+ messages in thread
From: Matthieu Baerts @ 2022-11-03 17:34 UTC (permalink / raw)
  To: Menglong Dong; +Cc: mptcp

Hi Menglong,

Thank you for the v6!

It looks like the CI is not happy with it:

On 03/11/2022 14:21, MPTCP CI wrote:
> Hi Menglong,
> 
> Thank you for your modifications, that's great!
> 
> Our CI did some validations and here is its report:
> 
> - KVM Validation: normal:
>   - Success! ✅:
>   - Task: https://cirrus-ci.com/task/4909154062565376
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt
> 
> - KVM Validation: debug:
>   - Unstable: 1 failed test(s): selftest_diag 🔴:
>   - Task: https://cirrus-ci.com/task/6035053969408000
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6035053969408000/summary/summary.txt

As you can see:

----------------------------
(...)
# all listen sockets                                [  ok  ]
# after MPC handshake                               [  ok  ]
# ....chk remote_key                                [  ok  ]
# ....chk no fallback                               [  ok  ]
# chk 2 msk in use                                  [  ok  ]
# chk 0 msk in use after flush                      [  ok  ]
# check fallback                                    [  ok  ]
# many msk socket present                           [ fail ] timeout
while expecting 200 max 201 last 1
# chk many msk in use                               [ fail ] expected
200 found 0
# chk 0 msk in use after flush                      [  ok  ]
----------------------------

I guess one socket is still present after the 'check fallback': you
probably need to modify flush_pids() to wait for the processes to be
over, as suggested on a comment in your v5, no?

https://lore.kernel.org/all/b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net/

I don't think it is a good idea to wait for >= 200 except if it takes a
very long time to have the previous socket terminated. If it does, maybe
we should re-order the test or re-create the netns instead of re-using it.


About the patch 3/4, note that the SIGUSR1 is probably stopping the test
earlier than expected because the interrupt will cause some actions to
stop but still good to check for the 'quit' variable.


Also, one small detail for patch 4/4: can you add "...." at the
beginning of the new lines you print in the selftest, similar to
"....chk no fallback"?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: selftest: mptcp: add test for mptcp socket in use: Tests Results
  2022-11-03 17:34     ` Matthieu Baerts
@ 2022-11-04  0:04       ` Mat Martineau
  2022-11-04  2:58       ` [Internet]Re: " imagedong(董梦龙)
  1 sibling, 0 replies; 9+ messages in thread
From: Mat Martineau @ 2022-11-04  0:04 UTC (permalink / raw)
  To: Menglong Dong; +Cc: Matthieu Baerts, mptcp

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Thu, 3 Nov 2022, Matthieu Baerts wrote:

> Hi Menglong,
>
> Thank you for the v6!
>
> It looks like the CI is not happy with it:
>
> On 03/11/2022 14:21, MPTCP CI wrote:
>> Hi Menglong,
>>
>> Thank you for your modifications, that's great!
>>
>> Our CI did some validations and here is its report:
>>
>> - KVM Validation: normal:
>>   - Success! ✅:
>>   - Task: https://cirrus-ci.com/task/4909154062565376
>>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt
>>
>> - KVM Validation: debug:
>>   - Unstable: 1 failed test(s): selftest_diag 🔴:
>>   - Task: https://cirrus-ci.com/task/6035053969408000
>>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6035053969408000/summary/summary.txt
>
> As you can see:
>
> ----------------------------
> (...)
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # chk 2 msk in use                                  [  ok  ]
> # chk 0 msk in use after flush                      [  ok  ]
> # check fallback                                    [  ok  ]
> # many msk socket present                           [ fail ] timeout
> while expecting 200 max 201 last 1
> # chk many msk in use                               [ fail ] expected
> 200 found 0
> # chk 0 msk in use after flush                      [  ok  ]
> ----------------------------
>
> I guess one socket is still present after the 'check fallback': you
> probably need to modify flush_pids() to wait for the processes to be
> over, as suggested on a comment in your v5, no?
>
> https://lore.kernel.org/all/b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net/
>
> I don't think it is a good idea to wait for >= 200 except if it takes a
> very long time to have the previous socket terminated. If it does, maybe
> we should re-order the test or re-create the netns instead of re-using it.
>
>
> About the patch 3/4, note that the SIGUSR1 is probably stopping the test
> earlier than expected because the interrupt will cause some actions to
> stop but still good to check for the 'quit' variable.
>
>
> Also, one small detail for patch 4/4: can you add "...." at the
> beginning of the new lines you print in the selftest, similar to
> "....chk no fallback"?
>

Menglong -

Thanks for the updated patches. The test ran ok on my local system, but 
the CI is slow on the debug build which makes the timing trickier. I don't 
have anything to add to Matthieu's comments above, seems like his 
suggestions will resolve the CI issue.

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Internet]Re: selftest: mptcp: add test for mptcp socket in use: Tests Results
  2022-11-03 17:34     ` Matthieu Baerts
  2022-11-04  0:04       ` Mat Martineau
@ 2022-11-04  2:58       ` imagedong(董梦龙)
  1 sibling, 0 replies; 9+ messages in thread
From: imagedong(董梦龙) @ 2022-11-04  2:58 UTC (permalink / raw)
  To: Matthieu Baerts; +Cc: mptcp



> On 2022/11/4 01:34,“Matthieu Baerts”<matthieu.baerts@tessares.net> write:
> 
> Hi Menglong,
> 
> Thank you for the v6!
> 
> It looks like the CI is not happy with it:
> 
> On 03/11/2022 14:21, MPTCP CI wrote:
> > Hi Menglong,
> > 
> > Thank you for your modifications, that's great!
> > 
> > Our CI did some validations and here is its report:
> > 
> > - KVM Validation: normal:
> >   - Success! ✅:
> >   - Task: https://cirrus-ci.com/task/4909154062565376
> >   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt
> > 
> > - KVM Validation: debug:
> >   - Unstable: 1 failed test(s): selftest_diag 🔴:
> >   - Task: https://cirrus-ci.com/task/6035053969408000
> >   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6035053969408000/summary/summary.txt
> 
> As you can see:
> 
> ----------------------------
> (...)
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # chk 2 msk in use                                  [  ok  ]
> # chk 0 msk in use after flush                      [  ok  ]
> # check fallback                                    [  ok  ]
> # many msk socket present                           [ fail ] timeout
> while expecting 200 max 201 last 1
> # chk many msk in use                               [ fail ] expected
> 200 found 0
> # chk 0 msk in use after flush                      [  ok  ]
> ----------------------------
> 
> I guess one socket is still present after the 'check fallback': you
> probably need to modify flush_pids() to wait for the processes to be
> over, as suggested on a comment in your v5, no?
> 

In fact, I suspect that it's something else. the wait_msk_nr()
has already wait long enough for the processes to be over.
I suspect that the msk is not released after the processes
exit.

Now, I can reproduce it in my computer, about once in 10 times.
And I'll try to figure out what's happening.

> b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net <https://lore.kernel.org/all/<a href=>/">https://lore.kernel.org/all/b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net/
> 
> I don't think it is a good idea to wait for >= 200 except if it takes a
> very long time to have the previous socket terminated. If it does, maybe
> we should re-order the test or re-create the netns instead of re-using it.
> 

It can happen in a very slow qemu machine, but not possible
in a physical machine.

> 
> About the patch 3/4, note that the SIGUSR1 is probably stopping the test
> earlier than expected because the interrupt will cause some actions to
> stop but still good to check for the 'quit' variable.
> 
> 
> Also, one small detail for patch 4/4: can you add "...." at the
> beginning of the new lines you print in the selftest, similar to
> "....chk no fallback"?
> 

Okay!

Thanks!
Menglong Dong

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-11-04  3:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 11:06 [PATCH mptcp-next v6 0/4] mptcp: add statistics for mptcp socket in use menglong8.dong
2022-11-03 11:06 ` [PATCH mptcp-next v6 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen() menglong8.dong
2022-11-03 11:06 ` [PATCH mptcp-next v6 2/4] mptcp: add statistics for mptcp socket in use menglong8.dong
2022-11-03 11:06 ` [PATCH mptcp-next v6 3/4] selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1 menglong8.dong
2022-11-03 11:06 ` [PATCH mptcp-next v6 4/4] selftest: mptcp: add test for mptcp socket in use menglong8.dong
2022-11-03 13:21   ` selftest: mptcp: add test for mptcp socket in use: Tests Results MPTCP CI
2022-11-03 17:34     ` Matthieu Baerts
2022-11-04  0:04       ` Mat Martineau
2022-11-04  2:58       ` [Internet]Re: " imagedong(董梦龙)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.