[PATCH liburing 0/3] multishot accept test fix and clean

[PATCH liburing 0/3] multishot accept test fix and clean

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

The multishot accept test is skipped, patch 1 fixes this. After this
it is still broken, patch 2 fixes it. And patch 3 is code clean.

Donald Hunter (1):
  Fix incorrect close in test for multishot accept

Hao Xu (2):
  test/accept: fix minus one error when calculating multishot_mask
  test/accept: clean code of accept test

 test/accept.c | 76 ++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

-- 
2.25.1

[PATCH 1/3] Fix incorrect close in test for multishot accept

[Hao Xu]

From: Donald Hunter <donald.hunter@gmail.com>

This fixes a bug in accept_conn handling in the accept tests that caused it
to incorrectly skip the multishot tests and also lose the warning message
to a closed stdout. This can be seen in the strace output below.

close(1)                                = 0
io_uring_setup(32, { ...
...
write(1, "Fixed Multishot Accept not suppo"..., 47) = -1 EINVAL

Unfortunately this exposes a a bug with gcc -O2 where multishot_mask logic
gets optimized incorrectly and "Fixed Multishot Accept misses events" is
wrongly reported. I am investigating this separately.

Fixes: 66cf84527c34 ("test/accept.c: add test for multishot mode accept")
Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 test/accept.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/test/accept.c b/test/accept.c
index 4e2f58767b3c..c6654baa3925 100644
--- a/test/accept.c
+++ b/test/accept.c
@@ -103,7 +103,7 @@ static void queue_accept_conn(struct io_uring *ring, int fd,
 	}
 }
 
-static int accept_conn(struct io_uring *ring, int fixed_idx)
+static int accept_conn(struct io_uring *ring, int fixed_idx, bool multishot)
 {
 	struct io_uring_cqe *cqe;
 	int ret;
@@ -115,8 +115,10 @@ static int accept_conn(struct io_uring *ring, int fixed_idx)
 
 	if (fixed_idx >= 0) {
 		if (ret > 0) {
-			close(ret);
-			return -EINVAL;
+			if (!multishot) {
+				close(ret);
+				return -EINVAL;
+			}
 		} else if (!ret) {
 			ret = fixed_idx;
 		}
@@ -208,7 +210,7 @@ static int test_loop(struct io_uring *ring,
 		queue_accept_conn(ring, recv_s0, args);
 
 	for (i = 0; i < MAX_FDS; i++) {
-		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1);
+		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1, multishot);
 		if (s_fd[i] == -EINVAL) {
 			if (args.accept_should_error)
 				goto out;
-- 
2.25.1

[PATCH 2/3] test/accept: fix minus one error when calculating multishot_mask

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

We don't need to minus one for the s_fd[i] since the returned cqe.res
is already the fixed file table slot which is indexed from zero.

Fixes: 66cf84527c34 ("test/accept.c: add test for multishot mode accept")
Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 test/accept.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/accept.c b/test/accept.c
index c6654baa3925..b322c018c7a9 100644
--- a/test/accept.c
+++ b/test/accept.c
@@ -241,7 +241,7 @@ static int test_loop(struct io_uring *ring,
 					i, s_fd[i]);
 				goto err;
 			}
-			multishot_mask |= (1 << (s_fd[i] - 1));
+			multishot_mask |= (1U << s_fd[i]);
 		}
 		if (!multishot)
 			break;
-- 
2.25.1

[PATCH 3/3] test/accept: clean code of accept test

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

This does three things:
 - change multishot_mask from uint to uint32_t
 - change multishot_mask != UINT_MAX check to ~multishot_mask != 0

The above two avoid compiler and arch influence. Make the logic more robost.

 - other cleaning to make code clearer.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 test/accept.c | 66 +++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

diff --git a/test/accept.c b/test/accept.c
index b322c018c7a9..78eba352a943 100644
--- a/test/accept.c
+++ b/test/accept.c
@@ -41,6 +41,21 @@ struct accept_test_args {
 	int extra_loops;
 };
 
+static void close_fds(int fds[], int nr)
+{
+	int i;
+
+	for (i = 0; i < nr; i++)
+		close(fds[i]);
+}
+
+static void close_sock_fds(int s_fd[], int c_fd[], int nr, bool fixed)
+{
+	if (!fixed)
+		close_fds(s_fd, nr);
+	close_fds(c_fd, nr);
+}
+
 static void queue_send(struct io_uring *ring, int fd)
 {
 	struct io_uring_sqe *sqe;
@@ -198,19 +213,17 @@ static int test_loop(struct io_uring *ring,
 	int i, ret, s_fd[MAX_FDS], c_fd[MAX_FDS], done = 0;
 	bool fixed = args.fixed;
 	bool multishot = args.multishot;
-	unsigned int multishot_mask = 0;
+	uint32_t multishot_mask = 0;
+	int nr_fds = multishot ? MAX_FDS : 1;
 
-	for (i = 0; i < MAX_FDS; i++) {
+	for (i = 0; i < nr_fds; i++)
 		c_fd[i] = set_client_fd(addr);
-		if (!multishot)
-			break;
-	}
 
 	if (!args.queue_accept_before_connect)
 		queue_accept_conn(ring, recv_s0, args);
 
-	for (i = 0; i < MAX_FDS; i++) {
-		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1, multishot);
+	for (i = 0; i < nr_fds; i++) {
+		s_fd[i] = accept_conn(ring, fixed ? 0 : -1, multishot);
 		if (s_fd[i] == -EINVAL) {
 			if (args.accept_should_error)
 				goto out;
@@ -241,14 +254,17 @@ static int test_loop(struct io_uring *ring,
 					i, s_fd[i]);
 				goto err;
 			}
+			/*
+			 * for fixed multishot accept test, the file slots
+			 * allocated are [0, 32), this means we finally end up
+			 * with each bit of a u32 being 1.
+			 */
 			multishot_mask |= (1U << s_fd[i]);
 		}
-		if (!multishot)
-			break;
 	}
 
 	if (multishot) {
-		if (fixed && multishot_mask != UINT_MAX) {
+		if (fixed && (~multishot_mask != 0U)) {
 			fprintf(stderr, "Fixed Multishot Accept misses events\n");
 			goto err;
 		}
@@ -256,7 +272,7 @@ static int test_loop(struct io_uring *ring,
 	}
 
 	queue_send(ring, c_fd[0]);
-	queue_recv(ring, s_fd[0], args.fixed);
+	queue_recv(ring, s_fd[0], fixed);
 
 	ret = io_uring_submit_and_wait(ring, 2);
 	assert(ret != -1);
@@ -280,32 +296,10 @@ static int test_loop(struct io_uring *ring,
 	}
 
 out:
-	if (!args.fixed) {
-		for (i = 0; i < MAX_FDS; i++) {
-			close(s_fd[i]);
-			if (!multishot)
-				break;
-		}
-	}
-	for (i = 0; i < MAX_FDS; i++) {
-		close(c_fd[i]);
-		if (!multishot)
-			break;
-	}
+	close_sock_fds(s_fd, c_fd, nr_fds, fixed);
 	return 0;
 err:
-	if (!args.fixed) {
-		for (i = 0; i < MAX_FDS; i++) {
-			close(s_fd[i]);
-			if (!multishot)
-				break;
-		}
-	}
-	for (i = 0; i < MAX_FDS; i++) {
-		close(c_fd[i]);
-		if (!multishot)
-			break;
-	}
+	close_sock_fds(s_fd, c_fd, nr_fds, fixed);
 	return 1;
 }
 
@@ -627,7 +621,7 @@ static int test_accept_fixed(void)
 static int test_multishot_fixed_accept(void)
 {
 	struct io_uring m_io_uring;
-	int ret, fd[100];
+	int ret, fd[MAX_FDS];
 	struct accept_test_args args = {
 		.fixed = true,
 		.multishot = true
-- 
2.25.1

Re: [PATCH liburing 0/3] multishot accept test fix and clean

[Hao Xu]

On 6/17/22 22:36, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> The multishot accept test is skipped, patch 1 fixes this. After this
> it is still broken, patch 2 fixes it. And patch 3 is code clean.
> 
> Donald Hunter (1):
>    Fix incorrect close in test for multishot accept
> 
> Hao Xu (2):
>    test/accept: fix minus one error when calculating multishot_mask
>    test/accept: clean code of accept test
> 
>   test/accept.c | 76 ++++++++++++++++++++++++---------------------------
>   1 file changed, 36 insertions(+), 40 deletions(-)
> 

Cced Donald Hunter

Re: [PATCH liburing 0/3] multishot accept test fix and clean

[Jens Axboe]

On Fri, 17 Jun 2022 22:36:00 +0800, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> The multishot accept test is skipped, patch 1 fixes this. After this
> it is still broken, patch 2 fixes it. And patch 3 is code clean.
> 
> Donald Hunter (1):
>   Fix incorrect close in test for multishot accept
> 
> [...]

Applied, thanks!

[1/3] Fix incorrect close in test for multishot accept
      commit: c8909b9e1963f4651927f8f46d1d0420d28bcd58
[2/3] test/accept: fix minus one error when calculating multishot_mask
      commit: d0d9b70d4861bae77b86c05f6925e991cb6c32f1
[3/3] test/accept: clean code of accept test
      commit: 6cc371d3fe076bcad549dbc0e1a95da8fc8085f2

Best regards,
-- 
Jens Axboe