[PATCH mptcp-next 0/4] add io thread mode tests

[PATCH mptcp-next 0/4] add io thread mode tests

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Related to #487 (send() fails with EAGAIN in blocking IO mode). In the
comment of this issue, I added a test named mptcp_eagain_reproducer.c to
reproduce these EAGAIN errors. It uses the same thread mode to send and
receive data as MPTCP sched BPF selftests (in network_helpers.c).

It looks like this type of data transfer is not covered by MPTCP selftests,
so this patchset adds them. The code is all from mptcp_eagain_reproducer.c,
just added into mptcp_connect and mptcp_join.

It is helpful to reproduce and solve #487 issue, and can also provide MPTCP
stability testing in the future.

Geliang Tang (4):
  selftests: mptcp: add cfg_timeo for mptcp_connect
  selftests: mptcp: add io thread mode for mptcp_connect
  selftests: mptcp: enable io thread mode
  selftests: mptcp: join: add io thread tests

 .../selftests/net/mptcp/mptcp_connect.c       | 163 +++++++++++++++++-
 .../testing/selftests/net/mptcp/mptcp_join.sh |  19 ++
 2 files changed, 177 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH mptcp-next 1/4] selftests: mptcp: add cfg_timeo for mptcp_connect

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper settimeo(), which comes from BPF selftests
network_helpers.c, into MPTCP selftests tool mptcp_connect, to set
SO_RCVTIMEO and SO_SNDTIMEO options of the given socket.

Add a new mptcp_connect option "-O" to pass a timeout value and a new
cfg cfg_timeo to store this value.

Invoke this new helper in sock_connect_mptcp() when cfg_timeo is set.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/net/mptcp/mptcp_connect.c       | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4209b9569039..ce261a4bb324 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -75,6 +75,7 @@ static char *cfg_input;
 static int cfg_repeat = 1;
 static int cfg_truncate;
 static int cfg_rcv_trunc;
+static int cfg_timeo;
 
 struct cfg_cmsg_types {
 	unsigned int cmsg_enabled:1;
@@ -249,6 +250,30 @@ static void set_mptfo(int fd, int pf)
 		perror("TCP_FASTOPEN");
 }
 
+static int settimeo(int fd, int timeout_ms)
+{
+	struct timeval timeout = { .tv_sec = 3 };
+
+	if (timeout_ms > 0) {
+		timeout.tv_sec = timeout_ms / 1000;
+		timeout.tv_usec = (timeout_ms % 1000) * 1000;
+	}
+
+	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout,
+		       sizeof(timeout))) {
+		perror("set SO_RCVTIMEO");
+		return -1;
+	}
+
+	if (setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout,
+		       sizeof(timeout))) {
+		perror("set SO_SNDTIMEO");
+		return -1;
+	}
+
+	return 0;
+}
+
 static int do_ulp_so(int sock, const char *name)
 {
 	return setsockopt(sock, IPPROTO_TCP, TCP_ULP, name, strlen(name));
@@ -376,6 +401,9 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 		if (cfg_mark)
 			set_mark(sock, cfg_mark);
 
+		if (cfg_timeo)
+			settimeo(sock, cfg_timeo);
+
 		if (cfg_sockopt_types.mptfo) {
 			if (!winfo->total_len)
 				winfo->total_len = winfo->len = read(infd, winfo->buf,
@@ -1384,7 +1412,7 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "6c:f:hi:I:jlm:M:o:p:P:r:R:s:S:t:T:w:")) != -1) {
+	while ((c = getopt(argc, argv, "6c:f:hi:I:jlm:M:o:p:P:r:R:s:S:t:T:w:O:")) != -1) {
 		switch (c) {
 		case 'f':
 			cfg_truncate = atoi(optarg);
@@ -1462,6 +1490,9 @@ static void parse_opts(int argc, char **argv)
 		case 'o':
 			parse_setsock_options(optarg);
 			break;
+		case 'O':
+			cfg_timeo = strtol(optarg, NULL, 0);
+			break;
 		}
 	}
 
-- 
2.43.0

[PATCH mptcp-next 2/4] selftests: mptcp: add io thread mode for mptcp_connect

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new io mode "thread" for mptcp_connect, it creates a
new thread to send data meanwhile receiving data in main thread.

copyfd_io_thread() comes from BPF selftests network_helpers.c.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/net/mptcp/mptcp_connect.c       | 105 ++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index ce261a4bb324..477969ba9653 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -15,6 +15,7 @@
 #include <signal.h>
 #include <unistd.h>
 #include <time.h>
+#include <pthread.h>
 
 #include <sys/ioctl.h>
 #include <sys/poll.h>
@@ -24,6 +25,7 @@
 #include <sys/socket.h>
 #include <sys/types.h>
 #include <sys/mman.h>
+#include <sys/param.h>
 
 #include <netdb.h>
 #include <netinet/in.h>
@@ -49,6 +51,7 @@ enum cfg_mode {
 	CFG_MODE_POLL,
 	CFG_MODE_MMAP,
 	CFG_MODE_SENDFILE,
+	CFG_MODE_THREAD,
 };
 
 enum cfg_peek {
@@ -105,6 +108,8 @@ static struct tcp_inq_state tcp_inq;
 static struct cfg_cmsg_types cfg_cmsg_types;
 static struct cfg_sockopt_types cfg_sockopt_types;
 
+static unsigned int io_thread_total_bytes = 10 * 1024 * 1024;
+
 static void die_usage(void)
 {
 	fprintf(stderr, "Usage: mptcp_connect [-6] [-c cmsg] [-f offset] [-i file] [-I num] [-j] [-l] "
@@ -139,6 +144,11 @@ static void die_usage(void)
 	exit(1);
 }
 
+static void *ERR_PTR(long error)
+{
+	return (void *)error;
+}
+
 static void xerror(const char *fmt, ...)
 {
 	va_list ap;
@@ -938,6 +948,94 @@ static int copyfd_io_sendfile(int infd, int peerfd, int outfd,
 	return err;
 }
 
+struct io_thread_arg {
+	int		fd;
+	uint32_t	bytes;
+	int		stop;
+};
+
+static void *send_thread(void *arg)
+{
+	struct io_thread_arg *a = (struct io_thread_arg *)arg;
+	ssize_t nr_sent = 0, bytes = 0;
+	int err = 0, fd = a->fd;
+	char batch[1500];
+
+	while (bytes < a->bytes && !a->stop) {
+		nr_sent = send(fd, &batch,
+			       MIN(a->bytes - bytes, sizeof(batch)), 0);
+		if (nr_sent == -1 && errno == EINTR)
+			continue;
+		if (nr_sent == -1) {
+			err = -errno;
+			break;
+		}
+		bytes += nr_sent;
+	}
+
+	if (bytes != a->bytes) {
+		printf("send %zd expected %u\n", bytes, a->bytes);
+		if (!err)
+			err = bytes > a->bytes ? -E2BIG : -EINTR;
+	}
+
+	if (err) {
+		a->stop = 1;
+		return ERR_PTR(err);
+	}
+	return NULL;
+}
+
+static int copyfd_io_thread(int peerfd, int fd, uint32_t total_bytes)
+{
+	ssize_t nr_recv = 0, bytes = 0;
+	struct io_thread_arg arg = {
+		.fd	= fd,
+		.bytes	= total_bytes,
+		.stop	= 0,
+	};
+	pthread_t thread;
+	void *thread_ret;
+	char batch[1500];
+	int err;
+
+	err = pthread_create(&thread, NULL, send_thread, (void *)&arg);
+	if (err) {
+		printf("Failed to pthread_create\n");
+		goto done;
+	}
+
+	/* recv total_bytes */
+	while (bytes < total_bytes && !arg.stop) {
+		nr_recv = recv(peerfd, &batch,
+			       MIN(total_bytes - bytes, sizeof(batch)), 0);
+		if (nr_recv == -1 && errno == EINTR)
+			continue;
+		if (nr_recv == -1) {
+			err = -errno;
+			break;
+		}
+		bytes += nr_recv;
+	}
+
+	if (bytes != total_bytes) {
+		printf("recv %zd expected %u\n", bytes, total_bytes);
+		if (!err)
+			err = bytes > total_bytes ? -E2BIG : -EINTR;
+	}
+
+	arg.stop = 1;
+	pthread_join(thread, &thread_ret);
+	if (thread_ret) {
+		printf("Failed in thread_ret %ld\n", (long)thread_ret);
+		err = err ? : (long)thread_ret;
+	}
+
+done:
+	close(peerfd);
+	return err;
+}
+
 static int copyfd_io(int infd, int peerfd, int outfd, bool close_peerfd, struct wstate *winfo)
 {
 	bool in_closed_after_out = false;
@@ -970,6 +1068,11 @@ static int copyfd_io(int infd, int peerfd, int outfd, bool close_peerfd, struct
 					 &in_closed_after_out, winfo);
 		break;
 
+	case CFG_MODE_THREAD:
+		ret = copyfd_io_thread(peerfd, outfd,
+				       io_thread_total_bytes);
+		break;
+
 	default:
 		fprintf(stderr, "Invalid mode %d\n", cfg_mode);
 
@@ -1352,6 +1455,8 @@ int parse_mode(const char *mode)
 		return CFG_MODE_MMAP;
 	if (!strcasecmp(mode, "sendfile"))
 		return CFG_MODE_SENDFILE;
+	if (!strcasecmp(mode, "thread"))
+		return CFG_MODE_THREAD;
 
 	fprintf(stderr, "Unknown test mode: %s\n", mode);
 	fprintf(stderr, "Supported modes are:\n");
-- 
2.43.0

[PATCH mptcp-next 3/4] selftests: mptcp: enable io thread mode

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new parameter "listensock" for the client main_loop()
to support io thread mode tests. In it, invoke accept() to get the peer
socket and pass it to copyfd_io(), then to copyfd_io_thread().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/net/mptcp/mptcp_connect.c       | 25 ++++++++++++++++---
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 477969ba9653..bd61cf203501 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1372,9 +1372,9 @@ void xdisconnect(int fd, int addrlen)
 		xerror("can't disconnect: %d", errno);
 }
 
-int main_loop(void)
+int main_loop(int listensock)
 {
-	int fd = 0, ret, fd_in = 0;
+	int fd = 0, ret, fd_in = 0, peerfd = 1;
 	struct addrinfo *peer;
 	struct wstate winfo;
 
@@ -1389,6 +1389,19 @@ int main_loop(void)
 	if (fd < 0)
 		return 2;
 
+	if (cfg_mode == CFG_MODE_THREAD &&
+	    listensock >= 0) {
+		peerfd = accept(listensock, NULL, NULL);
+		while (peerfd == -1) {
+			if (errno == EINTR)
+				continue;
+			return -errno;
+		}
+
+		if (cfg_timeo)
+			settimeo(peerfd, cfg_timeo);
+	}
+
 again:
 	check_getpeername_connect(fd);
 
@@ -1407,7 +1420,7 @@ int main_loop(void)
 			xerror("can't open %s:%d", cfg_input, errno);
 	}
 
-	ret = copyfd_io(fd_in, fd, 1, 0, &winfo);
+	ret = copyfd_io(fd_in, fd, peerfd, 0, &winfo);
 	if (ret)
 		return ret;
 
@@ -1430,6 +1443,8 @@ int main_loop(void)
 		close(fd);
 	}
 
+	if (listensock >= 0)
+		close(listensock);
 	return 0;
 }
 
@@ -1630,9 +1645,11 @@ int main(int argc, char *argv[])
 			set_mark(fd, cfg_mark);
 		if (cfg_cmsg_types.cmsg_enabled)
 			apply_cmsg_types(fd, &cfg_cmsg_types);
+		if (cfg_mode == CFG_MODE_THREAD)
+			return main_loop(fd);
 
 		return main_loop_s(fd);
 	}
 
-	return main_loop();
+	return main_loop(-1);
 }
-- 
2.43.0

[PATCH mptcp-next 4/4] selftests: mptcp: join: add io thread tests

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This io thread mode is used in BPF selftests bpf_tcp_ca and MPTCP, and
it is not very stable for MPTCP sched tests. This patch adds them into
mptcp_join tests (-T) to make sure this scene is also covered.

These tests only run in ns1 for 100 times. The failed output looks like:

 001 io thread tests
 recv 2884500 expected 10485760
 send 8005500 expected 10485760
 Failed in thread_ret -11
 Test no. 12 failed.
       syn                        [FAIL] got 12 JOIN[s] syn expected 100
       synack                     [ OK ]
       ack                        [FAIL] got 12 JOIN[s] ack expected 100

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../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 01c1e0871aca..ecba07aac776 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3663,6 +3663,24 @@ endpoint_tests()
 	fi
 }
 
+io_thread_tests()
+{
+	if reset "io thread tests"; then
+		local nr=0 max=100
+		mptcp_lib_pm_nl_set_limits $ns1 8 8
+		mptcp_lib_pm_nl_add_endpoint $ns1 10.0.2.1 flags subflow
+		while [ $nr -lt $max ]; do
+			nr=$((nr + 1))
+			ip netns exec $ns1 ./mptcp_connect -l 10.0.1.1 -m "thread" -O 3000
+			if [ $? -ne 0 ]; then
+				echo "Test no. $nr failed."
+				break;
+			fi
+		done
+		chk_join_nr $max 0 $max
+	fi
+}
+
 # [$1: error message]
 usage()
 {
@@ -3711,6 +3729,7 @@ all_tests_sorted=(
 	F@fail_tests
 	u@userspace_tests
 	I@endpoint_tests
+	T@io_thread_tests
 )
 
 all_tests_args=""
-- 
2.43.0

Re: [PATCH mptcp-next 0/4] add io thread mode tests

[Matthieu Baerts]

Hi Geliang,

On 01/08/2024 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Related to #487 (send() fails with EAGAIN in blocking IO mode). In the
> comment of this issue, I added a test named mptcp_eagain_reproducer.c to
> reproduce these EAGAIN errors. It uses the same thread mode to send and
> receive data as MPTCP sched BPF selftests (in network_helpers.c).
> 
> It looks like this type of data transfer is not covered by MPTCP selftests,
> so this patchset adds them. The code is all from mptcp_eagain_reproducer.c,
> just added into mptcp_connect and mptcp_join.
> 
> It is helpful to reproduce and solve #487 issue, and can also provide MPTCP
> stability testing in the future.

Good idea to have this covered!

I guess we need to wait for a fix before accepting this series, right?

By chance, did you try to reproduce the issue with packetdrill? That
might be quicker and more reliable to reproduce the issue, no?

I have some comments, please see my other emails.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 1/4] selftests: mptcp: add cfg_timeo for mptcp_connect

[Matthieu Baerts]

Hi Geliang,

On 01/08/2024 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper settimeo(), which comes from BPF selftests
> network_helpers.c, into MPTCP selftests tool mptcp_connect, to set
> SO_RCVTIMEO and SO_SNDTIMEO options of the given socket.

Can you add a (very) brief description explaining *why* it is
interesting to set these options in our tests?

(e.g. mention why the 'timeout' command we use when launching
'mptcp_connect' is not enough)

> Add a new mptcp_connect option "-O" to pass a timeout value and a new
> cfg cfg_timeo to store this value.

Please also update the 'usage()' helper with this new option.

> Invoke this new helper in sock_connect_mptcp() when cfg_timeo is set.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 2/4] selftests: mptcp: add io thread mode for mptcp_connect

[Matthieu Baerts]

Hi Geliang,

On 01/08/2024 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new io mode "thread" for mptcp_connect, it creates a
> new thread to send data meanwhile receiving data in main thread.

Please add the reason why it is interesting to do that, something
similar to what you said in your cover-letter: not covered, bugs have
been found, link to #487.

> copyfd_io_thread() comes from BPF selftests network_helpers.c.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/mptcp_connect.c       | 105 ++++++++++++++++++
>  1 file changed, 105 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index ce261a4bb324..477969ba9653 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c

(...)

> @@ -105,6 +108,8 @@ static struct tcp_inq_state tcp_inq;
>  static struct cfg_cmsg_types cfg_cmsg_types;
>  static struct cfg_sockopt_types cfg_sockopt_types;
>  
> +static unsigned int io_thread_total_bytes = 10 * 1024 * 1024;

Is it enough to always reproduce the bug? (I didn't check)
Did you not say that if it was bigger, it would trigger the bug
quicker/directly?

> +
>  static void die_usage(void)
>  {
>  	fprintf(stderr, "Usage: mptcp_connect [-6] [-c cmsg] [-f offset] [-i file] [-I num] [-j] [-l] "

Please add your new option here.

> @@ -139,6 +144,11 @@ static void die_usage(void)
>  	exit(1);
>  }
>  
> +static void *ERR_PTR(long error)
> +{
> +	return (void *)error;
> +}
> +
>  static void xerror(const char *fmt, ...)
>  {
>  	va_list ap;
> @@ -938,6 +948,94 @@ static int copyfd_io_sendfile(int infd, int peerfd, int outfd,
>  	return err;
>  }
>  
> +struct io_thread_arg {
> +	int		fd;
> +	uint32_t	bytes;
> +	int		stop;
> +};
> +
> +static void *send_thread(void *arg)
> +{
> +	struct io_thread_arg *a = (struct io_thread_arg *)arg;
> +	ssize_t nr_sent = 0, bytes = 0;
> +	int err = 0, fd = a->fd;
> +	char batch[1500];
> +
> +	while (bytes < a->bytes && !a->stop) {
> +		nr_sent = send(fd, &batch,
> +			       MIN(a->bytes - bytes, sizeof(batch)), 0);
> +		if (nr_sent == -1 && errno == EINTR)
> +			continue;
> +		if (nr_sent == -1) {
> +			err = -errno;
> +			break;
> +		}
> +		bytes += nr_sent;
> +	}
> +
> +	if (bytes != a->bytes) {
> +		printf("send %zd expected %u\n", bytes, a->bytes);

Should it be printed to stderr?
(or use xerror()? But it will exit during the transfer, I guess we don't
want that?)

> +		if (!err)
> +			err = bytes > a->bytes ? -E2BIG : -EINTR;
> +	}
> +
> +	if (err) {
> +		a->stop = 1;
> +		return ERR_PTR(err);
> +	}
> +	return NULL;
> +}
> +
> +static int copyfd_io_thread(int peerfd, int fd, uint32_t total_bytes)
> +{
> +	ssize_t nr_recv = 0, bytes = 0;
> +	struct io_thread_arg arg = {
> +		.fd	= fd,
> +		.bytes	= total_bytes,
> +		.stop	= 0,
> +	};
> +	pthread_t thread;
> +	void *thread_ret;
> +	char batch[1500];
> +	int err;
> +
> +	err = pthread_create(&thread, NULL, send_thread, (void *)&arg);
> +	if (err) {
> +		printf("Failed to pthread_create\n");

Same here for stderr, same below, there are 2 others.

> +		goto done;
> +	}
> +
> +	/* recv total_bytes */
> +	while (bytes < total_bytes && !arg.stop) {
> +		nr_recv = recv(peerfd, &batch,
> +			       MIN(total_bytes - bytes, sizeof(batch)), 0);
> +		if (nr_recv == -1 && errno == EINTR)
> +			continue;
> +		if (nr_recv == -1) {
> +			err = -errno;
> +			break;
> +		}
> +		bytes += nr_recv;
> +	}
> +
> +	if (bytes != total_bytes) {
> +		printf("recv %zd expected %u\n", bytes, total_bytes);
> +		if (!err)
> +			err = bytes > total_bytes ? -E2BIG : -EINTR;
> +	}
> +
> +	arg.stop = 1;
> +	pthread_join(thread, &thread_ret);
> +	if (thread_ret) {
> +		printf("Failed in thread_ret %ld\n", (long)thread_ret);
> +		err = err ? : (long)thread_ret;
> +	}
> +
> +done:
> +	close(peerfd);
> +	return err;
> +}
> +
>  static int copyfd_io(int infd, int peerfd, int outfd, bool close_peerfd, struct wstate *winfo)
>  {
>  	bool in_closed_after_out = false;
> @@ -970,6 +1068,11 @@ static int copyfd_io(int infd, int peerfd, int outfd, bool close_peerfd, struct
>  					 &in_closed_after_out, winfo);
>  		break;
>  
> +	case CFG_MODE_THREAD:
> +		ret = copyfd_io_thread(peerfd, outfd,
> +				       io_thread_total_bytes);
> +		break;
> +
>  	default:
>  		fprintf(stderr, "Invalid mode %d\n", cfg_mode);
>  
> @@ -1352,6 +1455,8 @@ int parse_mode(const char *mode)
>  		return CFG_MODE_MMAP;
>  	if (!strcasecmp(mode, "sendfile"))
>  		return CFG_MODE_SENDFILE;
> +	if (!strcasecmp(mode, "thread"))
> +		return CFG_MODE_THREAD;
>  
>  	fprintf(stderr, "Unknown test mode: %s\n", mode);
>  	fprintf(stderr, "Supported modes are:\n");

Can you document the new mode here as well please?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 3/4] selftests: mptcp: enable io thread mode

[Matthieu Baerts]

Hi Geliang,

On 01/08/2024 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch adds a new parameter "listensock" for the client main_loop()
> to support io thread mode tests. In it, invoke accept() to get the peer
> socket and pass it to copyfd_io(), then to copyfd_io_thread().

Please add the reason why this is needed.

Also, it is not clear that this will cause the same app to send and
receives data. Is it really required to reproduce the bug? Can we not do
the usual client-server thing? e.g.

  ./mptcp_connect.sh -m thread

Or at least an explicit option.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/mptcp_connect.c       | 25 ++++++++++++++++---
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 477969ba9653..bd61cf203501 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1372,9 +1372,9 @@ void xdisconnect(int fd, int addrlen)
>  		xerror("can't disconnect: %d", errno);
>  }
>  
> -int main_loop(void)
> +int main_loop(int listensock)
>  {
> -	int fd = 0, ret, fd_in = 0;
> +	int fd = 0, ret, fd_in = 0, peerfd = 1;
>  	struct addrinfo *peer;
>  	struct wstate winfo;
>  
> @@ -1389,6 +1389,19 @@ int main_loop(void)
>  	if (fd < 0)
>  		return 2;
>  
> +	if (cfg_mode == CFG_MODE_THREAD &&
> +	    listensock >= 0) {

(detail: can it not go to the previous line?)

> +		peerfd = accept(listensock, NULL, NULL);
> +		while (peerfd == -1) {
> +			if (errno == EINTR)
> +				continue;

Mmh, do you not need to call 'accept()' again?

> +			return -errno;
> +		}
> +
> +		if (cfg_timeo)
> +			settimeo(peerfd, cfg_timeo);
> +	}
> +
>  again:
>  	check_getpeername_connect(fd);
>  
> @@ -1407,7 +1420,7 @@ int main_loop(void)
>  			xerror("can't open %s:%d", cfg_input, errno);
>  	}
>  
> -	ret = copyfd_io(fd_in, fd, 1, 0, &winfo);
> +	ret = copyfd_io(fd_in, fd, peerfd, 0, &winfo);
>  	if (ret)
>  		return ret;
>  
> @@ -1430,6 +1443,8 @@ int main_loop(void)
>  		close(fd);
>  	}
>  
> +	if (listensock >= 0)
> +		close(listensock);
>  	return 0;
>  }
>  
> @@ -1630,9 +1645,11 @@ int main(int argc, char *argv[])
>  			set_mark(fd, cfg_mark);
>  		if (cfg_cmsg_types.cmsg_enabled)
>  			apply_cmsg_types(fd, &cfg_cmsg_types);
> +		if (cfg_mode == CFG_MODE_THREAD)
> +			return main_loop(fd);
>  
>  		return main_loop_s(fd);
>  	}
>  
> -	return main_loop();
> +	return main_loop(-1);
>  }

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 4/4] selftests: mptcp: join: add io thread tests

[Matthieu Baerts]

Hi Geliang,

On 01/08/2024 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This io thread mode is used in BPF selftests bpf_tcp_ca and MPTCP, and
> it is not very stable for MPTCP sched tests. This patch adds them into
> mptcp_join tests (-T) to make sure this scene is also covered.

Do you need to have multiple subflows to reproduce the issue? Can we not
have this test in mptcp_connect.sh instead?

For example, would it help to modify the CI to run:

  ./mptcp_connect.sh -m thread


> 
> These tests only run in ns1 for 100 times. The failed output looks like:
> 
>  001 io thread tests
>  recv 2884500 expected 10485760
>  send 8005500 expected 10485760
>  Failed in thread_ret -11
>  Test no. 12 failed.
>        syn                        [FAIL] got 12 JOIN[s] syn expected 100
>        synack                     [ OK ]
>        ack                        [FAIL] got 12 JOIN[s] ack expected 100
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../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 01c1e0871aca..ecba07aac776 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3663,6 +3663,24 @@ endpoint_tests()
>  	fi
>  }
>  
> +io_thread_tests()
> +{
> +	if reset "io thread tests"; then
> +		local nr=0 max=100
> +		mptcp_lib_pm_nl_set_limits $ns1 8 8
> +		mptcp_lib_pm_nl_add_endpoint $ns1 10.0.2.1 flags subflow
> +		while [ $nr -lt $max ]; do
> +			nr=$((nr + 1))
> +			ip netns exec $ns1 ./mptcp_connect -l 10.0.1.1 -m "thread" -O 3000

When you read this, it doesn't look obvious it is going to send and
receive data with itself. Can we not have a client and server? If not,
at least an explicit option, not making '-m "thread"' doing that.

> +			if [ $? -ne 0 ]; then

Shellcheck will not like that I guess:

  if ! ip netns exec (...); then

> +				echo "Test no. $nr failed."
> +				break;
> +			fi
> +		done
> +		chk_join_nr $max 0 $max
> +	fi
> +}
> +
>  # [$1: error message]
>  usage()
>  {
> @@ -3711,6 +3729,7 @@ all_tests_sorted=(
>  	F@fail_tests
>  	u@userspace_tests
>  	I@endpoint_tests
> +	T@io_thread_tests
>  )
>  
>  all_tests_args=""

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 0/4] add io thread mode tests

[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: Success! ✅
- KVM Validation: debug: Unstable: 2 failed test(s): mptcp_connect_mmap selftest_mptcp_join 🔴
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10195963580

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

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