[PATCH bpf-next v4 0/3] export send_recv_data

[PATCH bpf-next v4 0/3] export send_recv_data

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v4:
 - fix a bug in v3, it should be 'if (err)', not 'if (!err)'.
 - move "selftests/bpf: Use log_err in network_helpers" out of this
   series.

v3:
 - add two more patches.
 - use log_err instead of ASSERT in v3.
 - let send_recv_data return int as Martin suggested.

v2:

Address Martin's comments for v1 (thanks.)
 - drop patch 1, "export send_byte helper".
 - drop "WRITE_ONCE(arg.stop, 0)".
 - rebased.

send_recv_data will be re-used in MPTCP bpf tests, but not included
in this set because it depends on other patches that have not been
in the bpf-next yet. It will be sent as another set soon.

Geliang Tang (3):
  selftests/bpf: Add struct send_recv_arg
  selftests/bpf: Export send_recv_data helper
  selftests/bpf: Support nonblock for send_recv_data

 tools/testing/selftests/bpf/network_helpers.c | 103 ++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |   1 +
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     |  71 +-----------
 3 files changed, 105 insertions(+), 70 deletions(-)

-- 
2.40.1

[PATCH bpf-next v4 1/3] selftests/bpf: Add struct send_recv_arg

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Avoid setting total_bytes and stop as global variables, this patch adds
a new struct named send_recv_arg to pass arguments between threads. Put
these two variables together with fd into this struct and pass it to
server thread, so that server thread can access these two variables without
setting them as global ones.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     | 34 ++++++++++++-------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 077b107130f6..64f172f02a9a 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -21,7 +21,6 @@
 
 static const unsigned int total_bytes = 10 * 1024 * 1024;
 static int expected_stg = 0xeB9F;
-static int stop;
 
 static int settcpca(int fd, const char *tcp_ca)
 {
@@ -34,13 +33,20 @@ static int settcpca(int fd, const char *tcp_ca)
 	return 0;
 }
 
+struct send_recv_arg {
+	int		fd;
+	uint32_t	bytes;
+	int		stop;
+};
+
 static void *server(void *arg)
 {
-	int lfd = (int)(long)arg, err = 0, fd;
+	struct send_recv_arg *a = (struct send_recv_arg *)arg;
 	ssize_t nr_sent = 0, bytes = 0;
 	char batch[1500];
+	int err = 0, fd;
 
-	fd = accept(lfd, NULL, NULL);
+	fd = accept(a->fd, NULL, NULL);
 	while (fd == -1) {
 		if (errno == EINTR)
 			continue;
@@ -53,9 +59,9 @@ static void *server(void *arg)
 		goto done;
 	}
 
-	while (bytes < total_bytes && !READ_ONCE(stop)) {
+	while (bytes < a->bytes && !READ_ONCE(a->stop)) {
 		nr_sent = send(fd, &batch,
-			       MIN(total_bytes - bytes, sizeof(batch)), 0);
+			       MIN(a->bytes - bytes, sizeof(batch)), 0);
 		if (nr_sent == -1 && errno == EINTR)
 			continue;
 		if (nr_sent == -1) {
@@ -65,13 +71,13 @@ static void *server(void *arg)
 		bytes += nr_sent;
 	}
 
-	ASSERT_EQ(bytes, total_bytes, "send");
+	ASSERT_EQ(bytes, a->bytes, "send");
 
 done:
 	if (fd >= 0)
 		close(fd);
 	if (err) {
-		WRITE_ONCE(stop, 1);
+		WRITE_ONCE(a->stop, 1);
 		return ERR_PTR(err);
 	}
 	return NULL;
@@ -80,18 +86,22 @@ static void *server(void *arg)
 static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
 {
 	ssize_t nr_recv = 0, bytes = 0;
+	struct send_recv_arg arg = {
+		.bytes	= total_bytes,
+		.stop	= 0,
+	};
 	int lfd = -1, fd = -1;
 	pthread_t srv_thread;
 	void *thread_ret;
 	char batch[1500];
 	int err;
 
-	WRITE_ONCE(stop, 0);
-
 	lfd = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
 	if (!ASSERT_NEQ(lfd, -1, "socket"))
 		return;
 
+	arg.fd = lfd;
+
 	fd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (!ASSERT_NEQ(fd, -1, "socket")) {
 		close(lfd);
@@ -123,12 +133,12 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
 			goto done;
 	}
 
-	err = pthread_create(&srv_thread, NULL, server, (void *)(long)lfd);
+	err = pthread_create(&srv_thread, NULL, server, (void *)&arg);
 	if (!ASSERT_OK(err, "pthread_create"))
 		goto done;
 
 	/* recv total_bytes */
-	while (bytes < total_bytes && !READ_ONCE(stop)) {
+	while (bytes < total_bytes && !READ_ONCE(arg.stop)) {
 		nr_recv = recv(fd, &batch,
 			       MIN(total_bytes - bytes, sizeof(batch)), 0);
 		if (nr_recv == -1 && errno == EINTR)
@@ -140,7 +150,7 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
 
 	ASSERT_EQ(bytes, total_bytes, "recv");
 
-	WRITE_ONCE(stop, 1);
+	WRITE_ONCE(arg.stop, 1);
 	pthread_join(srv_thread, &thread_ret);
 	ASSERT_OK(IS_ERR(thread_ret), "thread_ret");
 
-- 
2.40.1

[PATCH bpf-next v4 2/3] selftests/bpf: Export send_recv_data helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch extracts the code to send and receive data into a new
helper named send_recv_data() in network_helpers.c and export it
in network_helpers.h.

This helper will be used for MPTCP BPF selftests.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/network_helpers.c | 96 +++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |  1 +
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     | 81 +---------------
 3 files changed, 98 insertions(+), 80 deletions(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 04175e16195a..137cd18ef3f2 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -545,3 +545,99 @@ int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
 	close(sockfd);
 	return 0;
 }
+
+struct send_recv_arg {
+	int		fd;
+	uint32_t	bytes;
+	int		stop;
+};
+
+static void *send_recv_server(void *arg)
+{
+	struct send_recv_arg *a = (struct send_recv_arg *)arg;
+	ssize_t nr_sent = 0, bytes = 0;
+	char batch[1500];
+	int err = 0, fd;
+
+	fd = accept(a->fd, NULL, NULL);
+	while (fd == -1) {
+		if (errno == EINTR)
+			continue;
+		err = -errno;
+		goto done;
+	}
+
+	if (settimeo(fd, 0)) {
+		err = -errno;
+		goto done;
+	}
+
+	while (bytes < a->bytes && !READ_ONCE(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)
+		log_err("send");
+
+done:
+	if (fd >= 0)
+		close(fd);
+	if (err) {
+		WRITE_ONCE(a->stop, 1);
+		return ERR_PTR(err);
+	}
+	return NULL;
+}
+
+int send_recv_data(int lfd, int fd, uint32_t total_bytes)
+{
+	ssize_t nr_recv = 0, bytes = 0;
+	struct send_recv_arg arg = {
+		.fd	= lfd,
+		.bytes	= total_bytes,
+		.stop	= 0,
+	};
+	pthread_t srv_thread;
+	void *thread_ret;
+	char batch[1500];
+	int err;
+
+	err = pthread_create(&srv_thread, NULL, send_recv_server, (void *)&arg);
+	if (err) {
+		log_err("pthread_create");
+		return err;
+	}
+
+	/* recv total_bytes */
+	while (bytes < total_bytes && !READ_ONCE(arg.stop)) {
+		nr_recv = recv(fd, &batch,
+			       MIN(total_bytes - bytes, sizeof(batch)), 0);
+		if (nr_recv == -1 && errno == EINTR)
+			continue;
+		if (nr_recv == -1)
+			break;
+		bytes += nr_recv;
+	}
+
+	if (bytes != total_bytes) {
+		log_err("recv");
+		return -1;
+	}
+
+	WRITE_ONCE(arg.stop, 1);
+	pthread_join(srv_thread, &thread_ret);
+	if (IS_ERR(thread_ret)) {
+		log_err("thread_ret");
+		return -1;
+	}
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 6457445cc6e2..70f4e4c92733 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -76,6 +76,7 @@ struct nstoken;
  */
 struct nstoken *open_netns(const char *name);
 void close_netns(struct nstoken *token);
+int send_recv_data(int lfd, int fd, uint32_t total_bytes);
 
 static __u16 csum_fold(__u32 csum)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 64f172f02a9a..907bac46c774 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -33,75 +33,15 @@ static int settcpca(int fd, const char *tcp_ca)
 	return 0;
 }
 
-struct send_recv_arg {
-	int		fd;
-	uint32_t	bytes;
-	int		stop;
-};
-
-static void *server(void *arg)
-{
-	struct send_recv_arg *a = (struct send_recv_arg *)arg;
-	ssize_t nr_sent = 0, bytes = 0;
-	char batch[1500];
-	int err = 0, fd;
-
-	fd = accept(a->fd, NULL, NULL);
-	while (fd == -1) {
-		if (errno == EINTR)
-			continue;
-		err = -errno;
-		goto done;
-	}
-
-	if (settimeo(fd, 0)) {
-		err = -errno;
-		goto done;
-	}
-
-	while (bytes < a->bytes && !READ_ONCE(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;
-	}
-
-	ASSERT_EQ(bytes, a->bytes, "send");
-
-done:
-	if (fd >= 0)
-		close(fd);
-	if (err) {
-		WRITE_ONCE(a->stop, 1);
-		return ERR_PTR(err);
-	}
-	return NULL;
-}
-
 static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
 {
-	ssize_t nr_recv = 0, bytes = 0;
-	struct send_recv_arg arg = {
-		.bytes	= total_bytes,
-		.stop	= 0,
-	};
 	int lfd = -1, fd = -1;
-	pthread_t srv_thread;
-	void *thread_ret;
-	char batch[1500];
 	int err;
 
 	lfd = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
 	if (!ASSERT_NEQ(lfd, -1, "socket"))
 		return;
 
-	arg.fd = lfd;
-
 	fd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (!ASSERT_NEQ(fd, -1, "socket")) {
 		close(lfd);
@@ -133,26 +73,7 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
 			goto done;
 	}
 
-	err = pthread_create(&srv_thread, NULL, server, (void *)&arg);
-	if (!ASSERT_OK(err, "pthread_create"))
-		goto done;
-
-	/* recv total_bytes */
-	while (bytes < total_bytes && !READ_ONCE(arg.stop)) {
-		nr_recv = recv(fd, &batch,
-			       MIN(total_bytes - bytes, sizeof(batch)), 0);
-		if (nr_recv == -1 && errno == EINTR)
-			continue;
-		if (nr_recv == -1)
-			break;
-		bytes += nr_recv;
-	}
-
-	ASSERT_EQ(bytes, total_bytes, "recv");
-
-	WRITE_ONCE(arg.stop, 1);
-	pthread_join(srv_thread, &thread_ret);
-	ASSERT_OK(IS_ERR(thread_ret), "thread_ret");
+	ASSERT_OK(send_recv_data(lfd, fd, total_bytes), "send_recv_data");
 
 done:
 	close(lfd);
-- 
2.40.1

[PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Some tests, such as the MPTCP bpf tests, require send_recv_data helper
to run in nonblock mode.

This patch adds nonblock support for send_recv_data(). Check if it is
currently in nonblock mode, and if so, ignore EWOULDBLOCK to continue
sending and receiving.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 137cd18ef3f2..ca16ef2b648e 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -555,6 +555,7 @@ struct send_recv_arg {
 static void *send_recv_server(void *arg)
 {
 	struct send_recv_arg *a = (struct send_recv_arg *)arg;
+	int flags = fcntl(a->fd, F_GETFL);
 	ssize_t nr_sent = 0, bytes = 0;
 	char batch[1500];
 	int err = 0, fd;
@@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
 		if (nr_sent == -1 && errno == EINTR)
 			continue;
 		if (nr_sent == -1) {
+			if (flags & O_NONBLOCK && errno == EWOULDBLOCK)
+				continue;
 			err = -errno;
 			break;
 		}
@@ -599,6 +602,7 @@ static void *send_recv_server(void *arg)
 
 int send_recv_data(int lfd, int fd, uint32_t total_bytes)
 {
+	int flags = fcntl(lfd, F_GETFL);
 	ssize_t nr_recv = 0, bytes = 0;
 	struct send_recv_arg arg = {
 		.fd	= lfd,
@@ -622,8 +626,11 @@ int send_recv_data(int lfd, int fd, uint32_t total_bytes)
 			       MIN(total_bytes - bytes, sizeof(batch)), 0);
 		if (nr_recv == -1 && errno == EINTR)
 			continue;
-		if (nr_recv == -1)
+		if (nr_recv == -1) {
+			if (flags & O_NONBLOCK && errno == EWOULDBLOCK)
+				continue;
 			break;
+		}
 		bytes += nr_recv;
 	}
 
-- 
2.40.1

Re: [PATCH bpf-next v4 0/3] export send_recv_data

[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: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8635152681

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/544eee7fd63f
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=843088


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)

Re: [PATCH bpf-next v4 2/3] selftests/bpf: Export send_recv_data helper

[Martin KaFai Lau]

On 4/9/24 11:13 PM, Geliang Tang wrote:
> +int send_recv_data(int lfd, int fd, uint32_t total_bytes)
> +{
> +	ssize_t nr_recv = 0, bytes = 0;
> +	struct send_recv_arg arg = {
> +		.fd	= lfd,
> +		.bytes	= total_bytes,
> +		.stop	= 0,
> +	};
> +	pthread_t srv_thread;
> +	void *thread_ret;
> +	char batch[1500];
> +	int err;
> +
> +	err = pthread_create(&srv_thread, NULL, send_recv_server, (void *)&arg);
> +	if (err) {
> +		log_err("pthread_create");
> +		return err;
> +	}
> +
> +	/* recv total_bytes */
> +	while (bytes < total_bytes && !READ_ONCE(arg.stop)) {
> +		nr_recv = recv(fd, &batch,
> +			       MIN(total_bytes - bytes, sizeof(batch)), 0);
> +		if (nr_recv == -1 && errno == EINTR)
> +			continue;
> +		if (nr_recv == -1)
> +			break;
> +		bytes += nr_recv;
> +	}
> +
> +	if (bytes != total_bytes) {
> +		log_err("recv");
> +		return -1;

This is still not right. It needs to write arg.stop and do pthread_join().

pw-bot: cr

> +	}
> +
> +	WRITE_ONCE(arg.stop, 1);
> +	pthread_join(srv_thread, &thread_ret);
> +	if (IS_ERR(thread_ret)) {
> +		log_err("thread_ret");
> +		return -1;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Martin KaFai Lau]

On 4/9/24 11:13 PM, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Some tests, such as the MPTCP bpf tests, require send_recv_data helper
> to run in nonblock mode.
> 
> This patch adds nonblock support for send_recv_data(). Check if it is
> currently in nonblock mode, and if so, ignore EWOULDBLOCK to continue
> sending and receiving.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 137cd18ef3f2..ca16ef2b648e 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -555,6 +555,7 @@ struct send_recv_arg {
>   static void *send_recv_server(void *arg)
>   {
>   	struct send_recv_arg *a = (struct send_recv_arg *)arg;
> +	int flags = fcntl(a->fd, F_GETFL);
>   	ssize_t nr_sent = 0, bytes = 0;
>   	char batch[1500];
>   	int err = 0, fd;
> @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
>   		if (nr_sent == -1 && errno == EINTR)
>   			continue;
>   		if (nr_sent == -1) {
> +			if (flags & O_NONBLOCK && errno == EWOULDBLOCK)

I still don't see why it needs to be a non blocking IO. mptcp should work
with blocking IO also, no? Does it really need non blocking IO to make
mptcp test work? I would rather stay with blocking IO in selftest as much as
possible for simplicity reason.

I am afraid the root cause of the EAGAIN thread has not been figured out yet:
https://lore.kernel.org/all/b3943f9a8bf595212b00e96ba850bf32893312cc.camel@kernel.org/

Lets drop patch 3 until it is understood why mptcp needs EAGAIN or non-blocking IO.
It feels like there is some flakiness and it should be understood and avoided.

Other than the comment in patch 2, the first two patches lgtm. Please respin with
the first two patches.

> +				continue;
>   			err = -errno;
>   			break;
>   		}
> @@ -599,6 +602,7 @@ static void *send_recv_server(void *arg)
>   
>   int send_recv_data(int lfd, int fd, uint32_t total_bytes)
>   {
> +	int flags = fcntl(lfd, F_GETFL);
>   	ssize_t nr_recv = 0, bytes = 0;
>   	struct send_recv_arg arg = {
>   		.fd	= lfd,
> @@ -622,8 +626,11 @@ int send_recv_data(int lfd, int fd, uint32_t total_bytes)
>   			       MIN(total_bytes - bytes, sizeof(batch)), 0);
>   		if (nr_recv == -1 && errno == EINTR)
>   			continue;
> -		if (nr_recv == -1)
> +		if (nr_recv == -1) {
> +			if (flags & O_NONBLOCK && errno == EWOULDBLOCK)
> +				continue;
>   			break;
> +		}
>   		bytes += nr_recv;
>   	}
>   

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

mptcp-only

Hi Matt & Mat,

On Wed, 2024-04-10 at 14:34 -0700, Martin KaFai Lau wrote:
> On 4/9/24 11:13 PM, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Some tests, such as the MPTCP bpf tests, require send_recv_data
> > helper
> > to run in nonblock mode.
> > 
> > This patch adds nonblock support for send_recv_data(). Check if it
> > is
> > currently in nonblock mode, and if so, ignore EWOULDBLOCK to
> > continue
> > sending and receiving.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/network_helpers.c
> > b/tools/testing/selftests/bpf/network_helpers.c
> > index 137cd18ef3f2..ca16ef2b648e 100644
> > --- a/tools/testing/selftests/bpf/network_helpers.c
> > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > @@ -555,6 +555,7 @@ struct send_recv_arg {
> >   static void *send_recv_server(void *arg)
> >   {
> >   	struct send_recv_arg *a = (struct send_recv_arg *)arg;
> > +	int flags = fcntl(a->fd, F_GETFL);
> >   	ssize_t nr_sent = 0, bytes = 0;
> >   	char batch[1500];
> >   	int err = 0, fd;
> > @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
> >   		if (nr_sent == -1 && errno == EINTR)
> >   			continue;
> >   		if (nr_sent == -1) {
> > +			if (flags & O_NONBLOCK && errno ==
> > EWOULDBLOCK)
> 
> I still don't see why it needs to be a non blocking IO. mptcp should
> work
> with blocking IO also, no? Does it really need non blocking IO to
> make
> mptcp test work? I would rather stay with blocking IO in selftest as
> much as
> possible for simplicity reason.

I need some help here.

This issue is reported by Matt in "CI: MPTCP BPF tests are now
validated", and my fixes ([1] and this patch) aren't accepted by
Martin. Is it normal to get EAGAINs in this case? Please give some
suggestions.

[1]
https://patchwork.kernel.org/project/mptcp/patch/311e074a3ca0465bdc5e4c2283e334bae5ccd306.1711296000.git.tanggeliang@kylinos.cn/

Thanks,
-Geliang

> 
> I am afraid the root cause of the EAGAIN thread has not been figured
> out yet:
> https://lore.kernel.org/all/b3943f9a8bf595212b00e96ba850bf32893312cc.camel@kernel.org/
> 
> Lets drop patch 3 until it is understood why mptcp needs EAGAIN or
> non-blocking IO.
> It feels like there is some flakiness and it should be understood and
> avoided.
> 
> Other than the comment in patch 2, the first two patches lgtm. Please
> respin with
> the first two patches.
> 
> > +				continue;
> >   			err = -errno;
> >   			break;
> >   		}
> > @@ -599,6 +602,7 @@ static void *send_recv_server(void *arg)
> >   
> >   int send_recv_data(int lfd, int fd, uint32_t total_bytes)
> >   {
> > +	int flags = fcntl(lfd, F_GETFL);
> >   	ssize_t nr_recv = 0, bytes = 0;
> >   	struct send_recv_arg arg = {
> >   		.fd	= lfd,
> > @@ -622,8 +626,11 @@ int send_recv_data(int lfd, int fd, uint32_t
> > total_bytes)
> >   			       MIN(total_bytes - bytes,
> > sizeof(batch)), 0);
> >   		if (nr_recv == -1 && errno == EINTR)
> >   			continue;
> > -		if (nr_recv == -1)
> > +		if (nr_recv == -1) {
> > +			if (flags & O_NONBLOCK && errno ==
> > EWOULDBLOCK)
> > +				continue;
> >   			break;
> > +		}
> >   		bytes += nr_recv;
> >   	}
> >   
> 
> 

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

On Thu, 2024-04-11 at 14:52 +0800, Geliang Tang wrote:
> mptcp-only
> 
> Hi Matt & Mat,
> 
> On Wed, 2024-04-10 at 14:34 -0700, Martin KaFai Lau wrote:
> > On 4/9/24 11:13 PM, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > Some tests, such as the MPTCP bpf tests, require send_recv_data
> > > helper
> > > to run in nonblock mode.
> > > 
> > > This patch adds nonblock support for send_recv_data(). Check if
> > > it
> > > is
> > > currently in nonblock mode, and if so, ignore EWOULDBLOCK to
> > > continue
> > > sending and receiving.
> > > 
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> > >   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/network_helpers.c
> > > b/tools/testing/selftests/bpf/network_helpers.c
> > > index 137cd18ef3f2..ca16ef2b648e 100644
> > > --- a/tools/testing/selftests/bpf/network_helpers.c
> > > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > > @@ -555,6 +555,7 @@ struct send_recv_arg {
> > >   static void *send_recv_server(void *arg)
> > >   {
> > >   	struct send_recv_arg *a = (struct send_recv_arg *)arg;
> > > +	int flags = fcntl(a->fd, F_GETFL);
> > >   	ssize_t nr_sent = 0, bytes = 0;
> > >   	char batch[1500];
> > >   	int err = 0, fd;
> > > @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
> > >   		if (nr_sent == -1 && errno == EINTR)
> > >   			continue;
> > >   		if (nr_sent == -1) {
> > > +			if (flags & O_NONBLOCK && errno ==
> > > EWOULDBLOCK)
> > 
> > I still don't see why it needs to be a non blocking IO. mptcp
> > should
> > work
> > with blocking IO also, no? Does it really need non blocking IO to
> > make
> > mptcp test work? I would rather stay with blocking IO in selftest
> > as
> > much as
> > possible for simplicity reason.
> 
> I need some help here.
> 
> This issue is reported by Matt in "CI: MPTCP BPF tests are now
> validated", and my fixes ([1] and this patch) aren't accepted by
> Martin. Is it normal to get EAGAINs in this case? Please give some
> suggestions.

It fails in mptcp_sendmsg()

1898 wait_for_memory:
1899                 set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
1900                 __mptcp_push_pending(sk, msg->msg_flags);
1901                 ret = sk_stream_wait_memory(sk, &timeo);
1902                 if (ret)
1903                         goto do_error;
1904         }

sk_stream_wait_memory() fails in the case, and
tcp_rtx_and_write_queues_empty(sk) is true.

I added a issue #487 to trace this issue.

Thanks,
-Geliang

> 
> [1]
> https://patchwork.kernel.org/project/mptcp/patch/311e074a3ca0465bdc5e4c2283e334bae5ccd306.1711296000.git.tanggeliang@kylinos.cn/
> 
> Thanks,
> -Geliang
> 
> > 
> > I am afraid the root cause of the EAGAIN thread has not been
> > figured
> > out yet:
> > https://lore.kernel.org/all/b3943f9a8bf595212b00e96ba850bf32893312cc.camel@kernel.org/
> > 
> > Lets drop patch 3 until it is understood why mptcp needs EAGAIN or
> > non-blocking IO.
> > It feels like there is some flakiness and it should be understood
> > and
> > avoided.
> > 
> > Other than the comment in patch 2, the first two patches lgtm.
> > Please
> > respin with
> > the first two patches.
> > 
> > > +				continue;
> > >   			err = -errno;
> > >   			break;
> > >   		}
> > > @@ -599,6 +602,7 @@ static void *send_recv_server(void *arg)
> > >   
> > >   int send_recv_data(int lfd, int fd, uint32_t total_bytes)
> > >   {
> > > +	int flags = fcntl(lfd, F_GETFL);
> > >   	ssize_t nr_recv = 0, bytes = 0;
> > >   	struct send_recv_arg arg = {
> > >   		.fd	= lfd,
> > > @@ -622,8 +626,11 @@ int send_recv_data(int lfd, int fd, uint32_t
> > > total_bytes)
> > >   			       MIN(total_bytes - bytes,
> > > sizeof(batch)), 0);
> > >   		if (nr_recv == -1 && errno == EINTR)
> > >   			continue;
> > > -		if (nr_recv == -1)
> > > +		if (nr_recv == -1) {
> > > +			if (flags & O_NONBLOCK && errno ==
> > > EWOULDBLOCK)
> > > +				continue;
> > >   			break;
> > > +		}
> > >   		bytes += nr_recv;
> > >   	}
> > >   
> > 
> > 
> 

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Matthieu Baerts]

Hi Geliang,

On 22/04/2024 08:50, Geliang Tang wrote:
> On Thu, 2024-04-11 at 14:52 +0800, Geliang Tang wrote:
>> mptcp-only
>>
>> Hi Matt & Mat,
>>
>> On Wed, 2024-04-10 at 14:34 -0700, Martin KaFai Lau wrote:
>>> On 4/9/24 11:13 PM, Geliang Tang wrote:
>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>
>>>> Some tests, such as the MPTCP bpf tests, require send_recv_data
>>>> helper
>>>> to run in nonblock mode.
>>>>
>>>> This patch adds nonblock support for send_recv_data(). Check if
>>>> it
>>>> is
>>>> currently in nonblock mode, and if so, ignore EWOULDBLOCK to
>>>> continue
>>>> sending and receiving.
>>>>
>>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>>> ---
>>>>   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/network_helpers.c
>>>> b/tools/testing/selftests/bpf/network_helpers.c
>>>> index 137cd18ef3f2..ca16ef2b648e 100644
>>>> --- a/tools/testing/selftests/bpf/network_helpers.c
>>>> +++ b/tools/testing/selftests/bpf/network_helpers.c
>>>> @@ -555,6 +555,7 @@ struct send_recv_arg {
>>>>   static void *send_recv_server(void *arg)
>>>>   {
>>>>   	struct send_recv_arg *a = (struct send_recv_arg *)arg;
>>>> +	int flags = fcntl(a->fd, F_GETFL);
>>>>   	ssize_t nr_sent = 0, bytes = 0;
>>>>   	char batch[1500];
>>>>   	int err = 0, fd;
>>>> @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
>>>>   		if (nr_sent == -1 && errno == EINTR)
>>>>   			continue;
>>>>   		if (nr_sent == -1) {
>>>> +			if (flags & O_NONBLOCK && errno ==
>>>> EWOULDBLOCK)
>>>
>>> I still don't see why it needs to be a non blocking IO. mptcp
>>> should
>>> work
>>> with blocking IO also, no? Does it really need non blocking IO to
>>> make
>>> mptcp test work? I would rather stay with blocking IO in selftest
>>> as
>>> much as
>>> possible for simplicity reason.
>>
>> I need some help here.
>>
>> This issue is reported by Matt in "CI: MPTCP BPF tests are now
>> validated", and my fixes ([1] and this patch) aren't accepted by
>> Martin. Is it normal to get EAGAINs in this case? Please give some
>> suggestions.

Thank you for the notification, I missed your previous question.

Regarding Martin's comment about O_NONBLOCK, I understand that with the
current BPF selftests -- so excluding the ones linked to the MPTCP
schedulers -- it is not needed to support O_NONBLOCK.

It looks like it is too early to do such modifications (patch 3), and
probably better to do MPTCP specific modifications only in our tree for
the moment. When upstreaming the new MPTCP BPF selftests, it will be
clearer for BPF maintainers what are our requirements, and such
modifications would make more sense, no?

> It fails in mptcp_sendmsg()
> 
> 1898 wait_for_memory:
> 1899                 set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> 1900                 __mptcp_push_pending(sk, msg->msg_flags);
> 1901                 ret = sk_stream_wait_memory(sk, &timeo);
> 1902                 if (ret)
> 1903                         goto do_error;
> 1904         }
> 
> sk_stream_wait_memory() fails in the case, and
> tcp_rtx_and_write_queues_empty(sk) is true.

My understanding is that with the current BPF selftests -- so excluding
the ones linked to the MPTCP schedulers here as well -- it looks
unlikely to have a EAGAIN errno, because only blocking IO is being used,
right?

I might be wrong, but with MPTCP schedulers, it is different: with
non-blocking IO, EAGAIN can be seen, and this case needs to be handled.
Actively waiting by retrying directly in the loop in case of EAGAIN is
not recommended, but probably OK for the tests (still, might be good to
add a comment to recommend polling instead). So here as well, such
modifications can stay in our tree for the moment.

Also, what is not clear to me is where we set the socket as the
non-blocking one. Is it only done for MPTCP servers/clients?

> I added a issue #487 to trace this issue.

Thanks!

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

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

Hi Matt,

On Mon, 2024-04-22 at 11:45 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/04/2024 08:50, Geliang Tang wrote:
> > On Thu, 2024-04-11 at 14:52 +0800, Geliang Tang wrote:
> > > mptcp-only
> > > 
> > > Hi Matt & Mat,
> > > 
> > > On Wed, 2024-04-10 at 14:34 -0700, Martin KaFai Lau wrote:
> > > > On 4/9/24 11:13 PM, Geliang Tang wrote:
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > Some tests, such as the MPTCP bpf tests, require
> > > > > send_recv_data
> > > > > helper
> > > > > to run in nonblock mode.
> > > > > 
> > > > > This patch adds nonblock support for send_recv_data(). Check
> > > > > if
> > > > > it
> > > > > is
> > > > > currently in nonblock mode, and if so, ignore EWOULDBLOCK to
> > > > > continue
> > > > > sending and receiving.
> > > > > 
> > > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > ---
> > > > >   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
> > > > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/bpf/network_helpers.c
> > > > > b/tools/testing/selftests/bpf/network_helpers.c
> > > > > index 137cd18ef3f2..ca16ef2b648e 100644
> > > > > --- a/tools/testing/selftests/bpf/network_helpers.c
> > > > > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > > > > @@ -555,6 +555,7 @@ struct send_recv_arg {
> > > > >   static void *send_recv_server(void *arg)
> > > > >   {
> > > > >   	struct send_recv_arg *a = (struct send_recv_arg
> > > > > *)arg;
> > > > > +	int flags = fcntl(a->fd, F_GETFL);
> > > > >   	ssize_t nr_sent = 0, bytes = 0;
> > > > >   	char batch[1500];
> > > > >   	int err = 0, fd;
> > > > > @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
> > > > >   		if (nr_sent == -1 && errno == EINTR)
> > > > >   			continue;
> > > > >   		if (nr_sent == -1) {
> > > > > +			if (flags & O_NONBLOCK && errno ==
> > > > > EWOULDBLOCK)
> > > > 
> > > > I still don't see why it needs to be a non blocking IO. mptcp
> > > > should
> > > > work
> > > > with blocking IO also, no? Does it really need non blocking IO
> > > > to
> > > > make
> > > > mptcp test work? I would rather stay with blocking IO in
> > > > selftest
> > > > as
> > > > much as
> > > > possible for simplicity reason.
> > > 
> > > I need some help here.
> > > 
> > > This issue is reported by Matt in "CI: MPTCP BPF tests are now
> > > validated", and my fixes ([1] and this patch) aren't accepted by
> > > Martin. Is it normal to get EAGAINs in this case? Please give
> > > some
> > > suggestions.
> 
> Thank you for the notification, I missed your previous question.
> 
> Regarding Martin's comment about O_NONBLOCK, I understand that with
> the
> current BPF selftests -- so excluding the ones linked to the MPTCP
> schedulers -- it is not needed to support O_NONBLOCK.
> 
> It looks like it is too early to do such modifications (patch 3), and
> probably better to do MPTCP specific modifications only in our tree
> for
> the moment. When upstreaming the new MPTCP BPF selftests, it will be
> clearer for BPF maintainers what are our requirements, and such
> modifications would make more sense, no?
> 
> > It fails in mptcp_sendmsg()
> > 
> > 1898 wait_for_memory:
> > 1899                 set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> > 1900                 __mptcp_push_pending(sk, msg->msg_flags);
> > 1901                 ret = sk_stream_wait_memory(sk, &timeo);
> > 1902                 if (ret)
> > 1903                         goto do_error;
> > 1904         }
> > 
> > sk_stream_wait_memory() fails in the case, and
> > tcp_rtx_and_write_queues_empty(sk) is true.
> 
> My understanding is that with the current BPF selftests -- so
> excluding
> the ones linked to the MPTCP schedulers here as well -- it looks
> unlikely to have a EAGAIN errno, because only blocking IO is being
> used,
> right?
> 
> I might be wrong, but with MPTCP schedulers, it is different: with
> non-blocking IO, EAGAIN can be seen, and this case needs to be
> handled.
> Actively waiting by retrying directly in the loop in case of EAGAIN
> is
> not recommended, but probably OK for the tests (still, might be good
> to
> add a comment to recommend polling instead). So here as well, such
> modifications can stay in our tree for the moment.
> 
> Also, what is not clear to me is where we set the socket as the
> non-blocking one. Is it only done for MPTCP servers/clients?

I didn't set the socket as non-block in the end.

EAGAIN will be got no matter non-block flag is set or not.

In old version like "[mptcp-next,v7,0/5] setsockopt per subflow: BPF":

https://patchwork.kernel.org/project/mptcp/cover/cover.1712571740.git.tanggeliang@kylinos.cn/

"set_nonblock" is added in patch 2, and invoked in run_subflow() in
patch 3.

Thanks,
-Geliang

> 
> > I added a issue #487 to trace this issue.
> 
> Thanks!
> 
> Cheers,
> Matt

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Matthieu Baerts]

On 22/04/2024 12:04, Geliang Tang wrote:> On Mon, 2024-04-22 at 11:45
+0200, Matthieu Baerts wrote:

(...)

>> I might be wrong, but with MPTCP schedulers, it is different: with
>> non-blocking IO, EAGAIN can be seen, and this case needs to be
>> handled.
>> Actively waiting by retrying directly in the loop in case of EAGAIN
>> is
>> not recommended, but probably OK for the tests (still, might be good
>> to
>> add a comment to recommend polling instead). So here as well, such
>> modifications can stay in our tree for the moment.
>>
>> Also, what is not clear to me is where we set the socket as the
>> non-blocking one. Is it only done for MPTCP servers/clients?
> 
> I didn't set the socket as non-block in the end.
> 
> EAGAIN will be got no matter non-block flag is set or not.

That's strange, no? When the userspace sends data with a blocking IO
socket, it should not get EAGAIN, no?

> In old version like "[mptcp-next,v7,0/5] setsockopt per subflow: BPF":
> 
> https://patchwork.kernel.org/project/mptcp/cover/cover.1712571740.git.tanggeliang@kylinos.cn/
> 
> "set_nonblock" is added in patch 2, and invoked in run_subflow() in
> patch 3.

I see, but the commit message of patch 2 doesn't explain why
set_nonblock() is needed for MPTCP. I see it is used in patch 3, but is
it really necessary? Maybe it was added for other reasons, that are no
longer valid today?

>>> I added a issue #487 to trace this issue.

Please also note that we didn't have this issue for a while. When we had
it, it is when KVM was not supported on the CI:

 $ /opt/virtme/virtme-run (...)
 Could not access KVM kernel module: No such file or directory
 qemu-system-x86_64: failed to initialize kvm: No such file or directory
 qemu-system-x86_64: falling back to tcg


Maybe we had this issue because the VM was abnormally too slow, and we
reached the 3 seconds timeout described by Martin in a previous message?
If this is due to the "abnormally slow setup", maybe we don't need to do
anything?

In other words, can you (easily) reproduce the issue on your side? With
or without KVM support?

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

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

On Mon, 2024-04-22 at 12:31 +0200, Matthieu Baerts wrote:
> On 22/04/2024 12:04, Geliang Tang wrote:> On Mon, 2024-04-22 at 11:45
> +0200, Matthieu Baerts wrote:
> 
> (...)
> 
> > > I might be wrong, but with MPTCP schedulers, it is different:
> > > with
> > > non-blocking IO, EAGAIN can be seen, and this case needs to be
> > > handled.
> > > Actively waiting by retrying directly in the loop in case of
> > > EAGAIN
> > > is
> > > not recommended, but probably OK for the tests (still, might be
> > > good
> > > to
> > > add a comment to recommend polling instead). So here as well,
> > > such
> > > modifications can stay in our tree for the moment.
> > > 
> > > Also, what is not clear to me is where we set the socket as the
> > > non-blocking one. Is it only done for MPTCP servers/clients?
> > 
> > I didn't set the socket as non-block in the end.
> > 
> > EAGAIN will be got no matter non-block flag is set or not.
> 
> That's strange, no? When the userspace sends data with a blocking IO
> socket, it should not get EAGAIN, no?
> 
> > In old version like "[mptcp-next,v7,0/5] setsockopt per subflow:
> > BPF":
> > 
> > https://patchwork.kernel.org/project/mptcp/cover/cover.1712571740.git.tanggeliang@kylinos.cn/
> > 
> > "set_nonblock" is added in patch 2, and invoked in run_subflow() in
> > patch 3.
> 
> I see, but the commit message of patch 2 doesn't explain why
> set_nonblock() is needed for MPTCP. I see it is used in patch 3, but
> is
> it really necessary? Maybe it was added for other reasons, that are
> no
> longer valid today?
> 
> > > > I added a issue #487 to trace this issue.
> 
> Please also note that we didn't have this issue for a while. When we
> had
> it, it is when KVM was not supported on the CI:
> 
>  $ /opt/virtme/virtme-run (...)
>  Could not access KVM kernel module: No such file or directory
>  qemu-system-x86_64: failed to initialize kvm: No such file or
> directory
>  qemu-system-x86_64: falling back to tcg
> 
> 
> Maybe we had this issue because the VM was abnormally too slow, and
> we
> reached the 3 seconds timeout described by Martin in a previous
> message?
> If this is due to the "abnormally slow setup", maybe we don't need to
> do
> anything?
> 
> In other words, can you (easily) reproduce the issue on your side?
> With
> or without KVM support?

It's easy to reproduce by increasing total_bytes:

+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -46,7 +46,7 @@
 #endif
 #define MPTCP_SCHED_NAME_MAX   16
 
-static const unsigned int total_bytes = 10 * 1024 * 1024;
+static const unsigned int total_bytes = 200 * 1024 * 1024;

Geliang

> 
> Cheers,
> Matt

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Matthieu Baerts]

On 22/04/2024 12:34, Geliang Tang wrote:
> On Mon, 2024-04-22 at 12:31 +0200, Matthieu Baerts wrote:
>> On 22/04/2024 12:04, Geliang Tang wrote:> On Mon, 2024-04-22 at 11:45
>> +0200, Matthieu Baerts wrote:
>>
>> (...)
>>
>>>> I might be wrong, but with MPTCP schedulers, it is different:
>>>> with
>>>> non-blocking IO, EAGAIN can be seen, and this case needs to be
>>>> handled.
>>>> Actively waiting by retrying directly in the loop in case of
>>>> EAGAIN
>>>> is
>>>> not recommended, but probably OK for the tests (still, might be
>>>> good
>>>> to
>>>> add a comment to recommend polling instead). So here as well,
>>>> such
>>>> modifications can stay in our tree for the moment.
>>>>
>>>> Also, what is not clear to me is where we set the socket as the
>>>> non-blocking one. Is it only done for MPTCP servers/clients?
>>>
>>> I didn't set the socket as non-block in the end.
>>>
>>> EAGAIN will be got no matter non-block flag is set or not.
>>
>> That's strange, no? When the userspace sends data with a blocking IO
>> socket, it should not get EAGAIN, no?
>>
>>> In old version like "[mptcp-next,v7,0/5] setsockopt per subflow:
>>> BPF":
>>>
>>> https://patchwork.kernel.org/project/mptcp/cover/cover.1712571740.git.tanggeliang@kylinos.cn/
>>>
>>> "set_nonblock" is added in patch 2, and invoked in run_subflow() in
>>> patch 3.
>>
>> I see, but the commit message of patch 2 doesn't explain why
>> set_nonblock() is needed for MPTCP. I see it is used in patch 3, but
>> is
>> it really necessary? Maybe it was added for other reasons, that are
>> no
>> longer valid today?
>>
>>>>> I added a issue #487 to trace this issue.
>>
>> Please also note that we didn't have this issue for a while. When we
>> had
>> it, it is when KVM was not supported on the CI:
>>
>>  $ /opt/virtme/virtme-run (...)
>>  Could not access KVM kernel module: No such file or directory
>>  qemu-system-x86_64: failed to initialize kvm: No such file or
>> directory
>>  qemu-system-x86_64: falling back to tcg
>>
>>
>> Maybe we had this issue because the VM was abnormally too slow, and
>> we
>> reached the 3 seconds timeout described by Martin in a previous
>> message?
>> If this is due to the "abnormally slow setup", maybe we don't need to
>> do
>> anything?
>>
>> In other words, can you (easily) reproduce the issue on your side?
>> With
>> or without KVM support?
> 
> It's easy to reproduce by increasing total_bytes:
Even without "set_nonblock()"? With what is in our tree, we don't do that.

As I mentioned above, if 'send()' stops before the end with -1 and errno
set to EAGAIN while the socket is supposed to wait for the transfer to
be over (blocking IO), I guess there is a bug somewhere, but not in the
tests, no?

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

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

On Mon, 2024-04-22 at 12:39 +0200, Matthieu Baerts wrote:
> On 22/04/2024 12:34, Geliang Tang wrote:
> > On Mon, 2024-04-22 at 12:31 +0200, Matthieu Baerts wrote:
> > > On 22/04/2024 12:04, Geliang Tang wrote:> On Mon, 2024-04-22 at
> > > 11:45
> > > +0200, Matthieu Baerts wrote:
> > > 
> > > (...)
> > > 
> > > > > I might be wrong, but with MPTCP schedulers, it is different:
> > > > > with
> > > > > non-blocking IO, EAGAIN can be seen, and this case needs to
> > > > > be
> > > > > handled.
> > > > > Actively waiting by retrying directly in the loop in case of
> > > > > EAGAIN
> > > > > is
> > > > > not recommended, but probably OK for the tests (still, might
> > > > > be
> > > > > good
> > > > > to
> > > > > add a comment to recommend polling instead). So here as well,
> > > > > such
> > > > > modifications can stay in our tree for the moment.
> > > > > 
> > > > > Also, what is not clear to me is where we set the socket as
> > > > > the
> > > > > non-blocking one. Is it only done for MPTCP servers/clients?
> > > > 
> > > > I didn't set the socket as non-block in the end.
> > > > 
> > > > EAGAIN will be got no matter non-block flag is set or not.
> > > 
> > > That's strange, no? When the userspace sends data with a blocking
> > > IO
> > > socket, it should not get EAGAIN, no?
> > > 
> > > > In old version like "[mptcp-next,v7,0/5] setsockopt per
> > > > subflow:
> > > > BPF":
> > > > 
> > > > https://patchwork.kernel.org/project/mptcp/cover/cover.1712571740.git.tanggeliang@kylinos.cn/
> > > > 
> > > > "set_nonblock" is added in patch 2, and invoked in
> > > > run_subflow() in
> > > > patch 3.
> > > 
> > > I see, but the commit message of patch 2 doesn't explain why
> > > set_nonblock() is needed for MPTCP. I see it is used in patch 3,
> > > but
> > > is
> > > it really necessary? Maybe it was added for other reasons, that
> > > are
> > > no
> > > longer valid today?
> > > 
> > > > > > I added a issue #487 to trace this issue.
> > > 
> > > Please also note that we didn't have this issue for a while. When
> > > we
> > > had
> > > it, it is when KVM was not supported on the CI:
> > > 
> > >  $ /opt/virtme/virtme-run (...)
> > >  Could not access KVM kernel module: No such file or directory
> > >  qemu-system-x86_64: failed to initialize kvm: No such file or
> > > directory
> > >  qemu-system-x86_64: falling back to tcg
> > > 
> > > 
> > > Maybe we had this issue because the VM was abnormally too slow,
> > > and
> > > we
> > > reached the 3 seconds timeout described by Martin in a previous
> > > message?
> > > If this is due to the "abnormally slow setup", maybe we don't
> > > need to
> > > do
> > > anything?
> > > 
> > > In other words, can you (easily) reproduce the issue on your
> > > side?
> > > With
> > > or without KVM support?
> > 
> > It's easy to reproduce by increasing total_bytes:
> Even without "set_nonblock()"? With what is in our tree, we don't do
> that.

Yes, without "set_nonblock". Without making any modifications, it can
be reproduced in our tree. Increasing totol_bytes can make reproduction
easier.

> 
> As I mentioned above, if 'send()' stops before the end with -1 and
> errno
> set to EAGAIN while the socket is supposed to wait for the transfer
> to
> be over (blocking IO), I guess there is a bug somewhere, but not in
> the
> tests, no?

Yes, I think it's a bug in kernel space in wait_for_memory in
mptcp_sendmsg().

Thanks,
-Geliang 

> 
> Cheers,
> Matt

Re: [PATCH bpf-next v4 3/3] selftests/bpf: Support nonblock for send_recv_data

[Geliang Tang]

On Wed, 2024-04-10 at 14:34 -0700, Martin KaFai Lau wrote:
> On 4/9/24 11:13 PM, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Some tests, such as the MPTCP bpf tests, require send_recv_data
> > helper
> > to run in nonblock mode.
> > 
> > This patch adds nonblock support for send_recv_data(). Check if it
> > is
> > currently in nonblock mode, and if so, ignore EWOULDBLOCK to
> > continue
> > sending and receiving.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >   tools/testing/selftests/bpf/network_helpers.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/network_helpers.c
> > b/tools/testing/selftests/bpf/network_helpers.c
> > index 137cd18ef3f2..ca16ef2b648e 100644
> > --- a/tools/testing/selftests/bpf/network_helpers.c
> > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > @@ -555,6 +555,7 @@ struct send_recv_arg {
> >   static void *send_recv_server(void *arg)
> >   {
> >   	struct send_recv_arg *a = (struct send_recv_arg *)arg;
> > +	int flags = fcntl(a->fd, F_GETFL);
> >   	ssize_t nr_sent = 0, bytes = 0;
> >   	char batch[1500];
> >   	int err = 0, fd;
> > @@ -578,6 +579,8 @@ static void *send_recv_server(void *arg)
> >   		if (nr_sent == -1 && errno == EINTR)
> >   			continue;
> >   		if (nr_sent == -1) {
> > +			if (flags & O_NONBLOCK && errno ==
> > EWOULDBLOCK)
> 
> I still don't see why it needs to be a non blocking IO. mptcp should
> work
> with blocking IO also, no? Does it really need non blocking IO to
> make
> mptcp test work? I would rather stay with blocking IO in selftest as
> much as
> possible for simplicity reason.
> 
> I am afraid the root cause of the EAGAIN thread has not been figured
> out yet:
> https://lore.kernel.org/all/b3943f9a8bf595212b00e96ba850bf32893312cc.camel@kernel.org/
> 
> Lets drop patch 3 until it is understood why mptcp needs EAGAIN or
> non-blocking IO.
> It feels like there is some flakiness and it should be understood and
> avoided.

Hi Martin,

I finally found the root cause of this issue. It is indeed an MPTCP
bug. It took me a long time to debug, and the fix is here:

https://patchwork.kernel.org/project/mptcp/patch/0ccc1c26d27d6ee7be22806a97983d37c6ca548c.1715053270.git.tanggeliang@kylinos.cn/

Thank you for insisting on not accepting these work around patches from
me in the user space, almost hiding a kernel bug.

-Geliang

> 
> Other than the comment in patch 2, the first two patches lgtm. Please
> respin with
> the first two patches.
> 
> > +				continue;
> >   			err = -errno;
> >   			break;
> >   		}
> > @@ -599,6 +602,7 @@ static void *send_recv_server(void *arg)
> >   
> >   int send_recv_data(int lfd, int fd, uint32_t total_bytes)
> >   {
> > +	int flags = fcntl(lfd, F_GETFL);
> >   	ssize_t nr_recv = 0, bytes = 0;
> >   	struct send_recv_arg arg = {
> >   		.fd	= lfd,
> > @@ -622,8 +626,11 @@ int send_recv_data(int lfd, int fd, uint32_t
> > total_bytes)
> >   			       MIN(total_bytes - bytes,
> > sizeof(batch)), 0);
> >   		if (nr_recv == -1 && errno == EINTR)
> >   			continue;
> > -		if (nr_recv == -1)
> > +		if (nr_recv == -1) {
> > +			if (flags & O_NONBLOCK && errno ==
> > EWOULDBLOCK)
> > +				continue;
> >   			break;
> > +		}
> >   		bytes += nr_recv;
> >   	}
> >   
> 
>