[PATCH liburing v2 0/1] test: add epoll test case

[PATCH liburing v2 0/1] test: add epoll test case

[Stefano Garzarella]

Hi Jens,
this is a v2 of the epoll test.

v1 -> v2:
    - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
    - add 2 new tests to test epoll with IORING_FEAT_NODROP
    - cleanups

There are 4 sub-tests:
    1. test_epoll
    2. test_epoll_sqpoll
    3. test_epoll_nodrop
    4. test_epoll_sqpoll_nodrop

In the first 2 tests, I try to avoid to queue more requests than we have room
for in the CQ ring. These work fine, I have no faults.

In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
much as I can until I get a -EBUSY, but they often fail in this way:
the submitter manages to submit everything, the receiver receives all the
submitted bytes, but the cleaner loses completion events (I also tried to put a
timeout to epoll_wait() in the cleaner to be sure that it is not related to the
patch that I send some weeks ago, but the situation doesn't change, it's like
there is still overflow in the CQ).

Next week I'll try to investigate better which is the problem.

I hope my test make sense, otherwise let me know what is wrong.

Anyway, when I was exploring the library, I had a doubt:
- in the __io_uring_get_cqe() should we call sys_io_uring_enter() also if
  submit and wait_nr are zero, but IORING_SQ_NEED_WAKEUP is set in the
  sq.kflags?

Thanks,
Stefano

Stefano Garzarella (1):
  test: add epoll test case

 .gitignore    |   1 +
 test/Makefile |   5 +-
 test/epoll.c  | 386 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+), 2 deletions(-)
 create mode 100644 test/epoll.c

-- 
2.24.1

[PATCH liburing v2 1/1] test: add epoll test case

[Stefano Garzarella]

This patch add the epoll test case that has four sub-tests:
- test_epoll
- test_epoll_sqpoll
- test_epoll_nodrop
- test_epoll_sqpoll_nodrop

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v1 -> v2:
    - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
    - add 2 new tests to test epoll with IORING_FEAT_NODROP
    - cleanups
---
 .gitignore    |   1 +
 test/Makefile |   5 +-
 test/epoll.c  | 386 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+), 2 deletions(-)
 create mode 100644 test/epoll.c

diff --git a/.gitignore b/.gitignore
index bdff558..49903ca 100644
--- a/.gitignore
+++ b/.gitignore
@@ -37,6 +37,7 @@
 /test/d77a67ed5f27-test
 /test/defer
 /test/eeed8b54e0df-test
+/test/epoll
 /test/fadvise
 /test/fallocate
 /test/fc2a85cb02ef-test
diff --git a/test/Makefile b/test/Makefile
index a975999..3f1d2f6 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -19,7 +19,7 @@ all_targets += poll poll-cancel ring-leak fsync io_uring_setup io_uring_register
 		poll-many b5837bd5311d-test accept-test d77a67ed5f27-test \
 		connect 7ad0e4b2f83c-test submit-reuse fallocate open-close \
 		file-update statx accept-reuse poll-v-poll fadvise madvise \
-		short-read openat2 probe shared-wq personality
+		short-read openat2 probe shared-wq personality epoll
 
 include ../Makefile.quiet
 
@@ -46,7 +46,7 @@ test_srcs := poll.c poll-cancel.c ring-leak.c fsync.c io_uring_setup.c \
 	7ad0e4b2f83c-test.c submit-reuse.c fallocate.c open-close.c \
 	file-update.c statx.c accept-reuse.c poll-v-poll.c fadvise.c \
 	madvise.c short-read.c openat2.c probe.c shared-wq.c \
-	personality.c
+	personality.c epoll.c
 
 test_objs := $(patsubst %.c,%.ol,$(test_srcs))
 
@@ -57,6 +57,7 @@ poll-link: XCFLAGS = -lpthread
 accept-link: XCFLAGS = -lpthread
 submit-reuse: XCFLAGS = -lpthread
 poll-v-poll: XCFLAGS = -lpthread
+epoll: XCFLAGS = -lpthread
 
 install: $(all_targets) runtests.sh runtests-loop.sh
 	$(INSTALL) -D -d -m 755 $(datadir)/liburing-test/
diff --git a/test/epoll.c b/test/epoll.c
new file mode 100644
index 0000000..610820a
--- /dev/null
+++ b/test/epoll.c
@@ -0,0 +1,386 @@
+/*
+ * Description: test io_uring poll handling using a pipe
+ *
+ *              Three threads involved:
+ *              - producer: fills SQ with write requests for the pipe
+ *              - cleaner: consumes CQ, freeing the buffers that producer
+ *                         allocates
+ *              - consumer: read() blocking on the pipe
+ *
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include <signal.h>
+#include <sys/poll.h>
+#include <sys/wait.h>
+#include <sys/epoll.h>
+
+#include "liburing.h"
+
+#define TIMEOUT         10
+#define ITERATIONS      100
+#define RING_ENTRIES    2
+#define BUF_SIZE        2048
+#define PIPE_SIZE       4096 /* pipe capacity below the page size are silently
+			      * rounded up to the page size
+			      */
+
+enum {
+	TEST_OK = 0,
+	TEST_SKIPPED = 1,
+	TEST_FAILED = 2,
+};
+
+struct thread_data {
+	struct io_uring *ring;
+
+	volatile uint32_t submitted;
+	volatile uint32_t freed;
+	uint32_t entries;
+	int pipe_read;
+	int pipe_write;
+	bool sqpoll;
+	bool nodrop;
+};
+
+static void sig_alrm(int sig)
+{
+	fprintf(stderr, "Timed out!\n");
+	exit(1);
+}
+
+static struct iovec *alloc_vec(void)
+{
+	struct iovec *vec;
+
+	vec = malloc(sizeof(struct iovec));
+	if (!vec) {
+		perror("malloc iovec");
+		exit(1);
+	}
+	vec->iov_base = malloc(BUF_SIZE);
+	if (!vec->iov_base) {
+		perror("malloc buffer");
+		exit(1);
+	}
+	vec->iov_len = BUF_SIZE;
+
+	return vec;
+}
+
+static void free_vec(struct iovec *vec)
+{
+	free(vec->iov_base);
+	free(vec);
+}
+
+static void *do_test_epoll_produce(void *data)
+{
+	struct thread_data *td = data;
+	struct io_uring_sqe *sqe;
+	struct epoll_event ev;
+	void *th_ret = (void *)1;
+	int fd, ret;
+
+	fd = epoll_create1(0);
+	if (fd < 0) {
+		perror("epoll_create");
+		return th_ret;
+	}
+
+	ev.events = EPOLLOUT;
+	ev.data.fd = td->ring->ring_fd;
+
+	if (epoll_ctl(fd, EPOLL_CTL_ADD, td->ring->ring_fd, &ev) < 0) {
+		perror("epoll_ctrl");
+		goto ret;
+	}
+
+	while (td->submitted < ITERATIONS) {
+		bool submit = false;
+
+		ret = epoll_wait(fd, &ev, 1, -1);
+		if (ret < 0) {
+			perror("epoll_wait");
+			goto ret;
+		}
+
+		while (td->submitted < ITERATIONS) {
+			struct iovec *vec;
+
+			/*
+			 * If IORING_FEAT_NODROP is not supported, we want to
+			 * avoid the drop of completion event.
+			 */
+			if (!td->nodrop &&
+			    (td->submitted - td->freed >= td->entries))
+				break;
+
+			sqe = io_uring_get_sqe(td->ring);
+			if (!sqe)
+				break;
+
+			vec = alloc_vec();
+			io_uring_prep_writev(sqe, td->pipe_write, vec, 1, 0);
+
+			if (td->sqpoll)
+				sqe->flags |= IOSQE_FIXED_FILE;
+
+			io_uring_sqe_set_data(sqe, vec);
+			td->submitted++;
+			submit = true;
+		}
+
+		if (!submit)
+			continue;
+
+		ret = io_uring_submit(td->ring);
+		while (td->nodrop && ret == -EBUSY) {
+			usleep(10000);
+			ret = io_uring_submit(td->ring);
+		}
+		if (ret <= 0) {
+			fprintf(stderr, "io_uring_submit failed - ret: %d\n",
+				ret);
+			goto ret;
+		}
+	}
+
+	printf("Successfully submitted %d requests\n", td->submitted);
+
+	th_ret = 0;
+ret:
+	close(fd);
+	return th_ret;
+}
+
+static void *do_test_epoll_free(void *data)
+{
+	struct thread_data *td = data;
+	struct io_uring_cqe *cqe;
+	struct epoll_event ev;
+	int fd, ret;
+	void *th_ret = (void *)1;
+
+	fd = epoll_create1(0);
+	if (fd < 0) {
+		perror("epoll_create");
+		return th_ret;
+	}
+
+	ev.events = EPOLLIN;
+	ev.data.fd = td->ring->ring_fd;
+
+	if (epoll_ctl(fd, EPOLL_CTL_ADD, td->ring->ring_fd, &ev) < 0) {
+		perror("epoll_ctrl");
+		goto ret;
+	}
+
+	while (td->freed < ITERATIONS) {
+		ret = epoll_wait(fd, &ev, 1, -1);
+		if (ret < 0) {
+			perror("epoll_wait");
+			goto ret;
+		}
+
+		while (td->freed < ITERATIONS) {
+			struct iovec *vec;
+
+			ret = io_uring_peek_cqe(td->ring, &cqe);
+			if (!cqe || ret) {
+				if (ret == -EAGAIN)
+					break;
+				fprintf(stderr,
+					"io_uring_peek_cqe failed - ret: %d\n",
+					ret);
+				goto ret;
+			}
+
+			vec = io_uring_cqe_get_data(cqe);
+			io_uring_cqe_seen(td->ring, cqe);
+			free_vec(vec);
+			td->freed++;
+		}
+	}
+
+	printf("Successfully completed %d requests\n", td->freed);
+
+	th_ret = 0;
+ret:
+	close(fd);
+	return th_ret;
+}
+
+
+static void *do_test_epoll_consume(void *data)
+{
+	struct thread_data *td = data;
+	static uint8_t buf[BUF_SIZE];
+	int ret, iter = 0;
+	void *th_ret = (void *)1;
+
+	while (iter < ITERATIONS) {
+		errno = 0;
+		ret = read(td->pipe_read, &buf, BUF_SIZE);
+		if (ret != BUF_SIZE)
+			break;
+		iter++;
+	};
+
+	if (ret < 0) {
+		perror("read");
+		goto ret;
+	}
+
+	if (iter != ITERATIONS) {
+		fprintf(stderr, "Wrong iterations: %d [expected %d]\n",
+			iter, ITERATIONS);
+		goto ret;
+	}
+
+	printf("Successfully received %d messages\n", iter);
+
+	th_ret = 0;
+ret:
+	return th_ret;
+}
+
+static int do_test_epoll(bool sqpoll, bool nodrop)
+{
+	int ret = 0, pipe1[2];
+	struct io_uring_params param;
+	struct thread_data td;
+	pthread_t threads[3];
+	struct io_uring ring;
+	void *ret_th[3];
+
+	if (geteuid() && sqpoll) {
+		fprintf(stderr, "sqpoll requires root!\n");
+		return TEST_SKIPPED;
+	}
+
+	if (pipe(pipe1) != 0) {
+		perror("pipe");
+		return TEST_FAILED;
+	}
+
+	ret = fcntl(pipe1[0], F_SETPIPE_SZ, PIPE_SIZE);
+	if (ret < 0) {
+		perror("fcntl");
+		ret = TEST_FAILED;
+		goto err_pipe;
+	}
+
+	memset(&param, 0, sizeof(param));
+	memset(&td, 0, sizeof(td));
+
+	td.sqpoll = sqpoll;
+	td.nodrop = nodrop;
+	td.pipe_read = pipe1[0];
+	td.pipe_write = pipe1[1];
+	td.entries = RING_ENTRIES;
+
+	if (td.sqpoll)
+		param.flags |= IORING_SETUP_SQPOLL;
+
+	ret = io_uring_queue_init_params(td.entries, &ring, &param);
+	if (ret) {
+		fprintf(stderr, "ring setup failed\n");
+		ret = TEST_FAILED;
+	}
+
+	if (nodrop && !(param.features & IORING_FEAT_NODROP)) {
+		fprintf(stderr, "IORING_FEAT_NODROP not supported!\n");
+		ret = TEST_SKIPPED;
+		goto err_pipe;
+	}
+
+	td.ring = &ring;
+
+	if (td.sqpoll) {
+		ret = io_uring_register_files(&ring, &td.pipe_write, 1);
+		if (ret) {
+			fprintf(stderr, "file reg failed: %d\n", ret);
+			ret = TEST_FAILED;
+			goto err_uring;
+		}
+
+		td.pipe_write = 0;
+	}
+
+	pthread_create(&threads[0], NULL, do_test_epoll_produce, &td);
+	pthread_create(&threads[1], NULL, do_test_epoll_free, &td);
+	pthread_create(&threads[2], NULL, do_test_epoll_consume, &td);
+
+	pthread_join(threads[0], &ret_th[0]);
+	pthread_join(threads[1], &ret_th[1]);
+	pthread_join(threads[2], &ret_th[2]);
+
+	if (ret_th[0] || ret_th[1] || ret_th[2]) {
+		fprintf(stderr, "threads ended with errors\n");
+		ret = TEST_FAILED;
+		goto err_uring;
+	}
+
+	ret = TEST_OK;
+
+err_uring:
+	io_uring_queue_exit(&ring);
+err_pipe:
+	close(pipe1[0]);
+	close(pipe1[1]);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction act;
+	int ret;
+
+	memset(&act, 0, sizeof(act));
+	act.sa_handler = sig_alrm;
+	act.sa_flags = SA_RESTART;
+	sigaction(SIGALRM, &act, NULL);
+	alarm(TIMEOUT);
+
+	ret = do_test_epoll(false, false);
+	if (ret == TEST_SKIPPED) {
+		printf("test_epoll: skipped\n");
+	} else if (ret == TEST_FAILED) {
+		fprintf(stderr, "test_epoll failed\n");
+		return ret;
+	}
+
+	ret = do_test_epoll(true, false);
+	if (ret == TEST_SKIPPED) {
+		printf("test_epoll_sqpoll: skipped\n");
+	} else if (ret == TEST_FAILED) {
+		fprintf(stderr, "test_epoll_sqpoll failed\n");
+		return ret;
+	}
+
+	ret = do_test_epoll(false, true);
+	if (ret == TEST_SKIPPED) {
+		printf("test_epoll_nodrop: skipped\n");
+	} else if (ret == TEST_FAILED) {
+		fprintf(stderr, "test_epoll_nodrop failed\n");
+		return ret;
+	}
+
+	ret = do_test_epoll(true, true);
+	if (ret == TEST_SKIPPED) {
+		printf("test_epoll_sqpoll_nodrop: skipped\n");
+	} else if (ret == TEST_FAILED) {
+		fprintf(stderr, "test_epoll_sqpoll_nodrop failed\n");
+		return ret;
+	}
+
+	return 0;
+}
-- 
2.24.1

Re: [PATCH liburing v2 1/1] test: add epoll test case

[Jens Axboe]

On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> This patch add the epoll test case that has four sub-tests:
> - test_epoll
> - test_epoll_sqpoll
> - test_epoll_nodrop
> - test_epoll_sqpoll_nodrop

Since we have EPOLL_CTL now, any chance you could also include
a test case that uses that instead of epoll_ctl()?

-- 
Jens Axboe

Re: [PATCH liburing v2 1/1] test: add epoll test case

[Stefano Garzarella]

On Fri, Jan 31, 2020 at 08:41:49AM -0700, Jens Axboe wrote:
> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> > This patch add the epoll test case that has four sub-tests:
> > - test_epoll
> > - test_epoll_sqpoll
> > - test_epoll_nodrop
> > - test_epoll_sqpoll_nodrop
> 
> Since we have EPOLL_CTL now, any chance you could also include
> a test case that uses that instead of epoll_ctl()?

Sure, I'll add a test case for EPOLL_CTL!

Stefano

Re: [PATCH liburing v2 1/1] test: add epoll test case

[Jens Axboe]

On 2/3/20 2:04 AM, Stefano Garzarella wrote:
> On Fri, Jan 31, 2020 at 08:41:49AM -0700, Jens Axboe wrote:
>> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
>>> This patch add the epoll test case that has four sub-tests:
>>> - test_epoll
>>> - test_epoll_sqpoll
>>> - test_epoll_nodrop
>>> - test_epoll_sqpoll_nodrop
>>
>> Since we have EPOLL_CTL now, any chance you could also include
>> a test case that uses that instead of epoll_ctl()?
> 
> Sure, I'll add a test case for EPOLL_CTL!

Awesome, thanks!

-- 
Jens Axboe

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Jens Axboe]

On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> Hi Jens,
> this is a v2 of the epoll test.
> 
> v1 -> v2:
>     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
>     - add 2 new tests to test epoll with IORING_FEAT_NODROP
>     - cleanups
> 
> There are 4 sub-tests:
>     1. test_epoll
>     2. test_epoll_sqpoll
>     3. test_epoll_nodrop
>     4. test_epoll_sqpoll_nodrop
> 
> In the first 2 tests, I try to avoid to queue more requests than we have room
> for in the CQ ring. These work fine, I have no faults.

Thanks!

> In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
> much as I can until I get a -EBUSY, but they often fail in this way:
> the submitter manages to submit everything, the receiver receives all the
> submitted bytes, but the cleaner loses completion events (I also tried to put a
> timeout to epoll_wait() in the cleaner to be sure that it is not related to the
> patch that I send some weeks ago, but the situation doesn't change, it's like
> there is still overflow in the CQ).
> 
> Next week I'll try to investigate better which is the problem.

Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
you just pruned the CQ ring but didn't flush the internal side.

> I hope my test make sense, otherwise let me know what is wrong.

I'll take a look...

> Anyway, when I was exploring the library, I had a doubt:
> - in the __io_uring_get_cqe() should we call sys_io_uring_enter() also if
>   submit and wait_nr are zero, but IORING_SQ_NEED_WAKEUP is set in the
>   sq.kflags?

It's a submission side thing, the completion side shouldn't care. That
flag is only relevant if you're submitting IO with SQPOLL. Then it tells
you that the thread needs to get woken up, which you need io_uring_enter()
to do. But for just reaping completions and not needing to submit
anything new, we don't care if the thread is sleeping.

-- 
Jens Axboe

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Stefano Garzarella]

On Fri, Jan 31, 2020 at 4:39 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> > Hi Jens,
> > this is a v2 of the epoll test.
> >
> > v1 -> v2:
> >     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
> >     - add 2 new tests to test epoll with IORING_FEAT_NODROP
> >     - cleanups
> >
> > There are 4 sub-tests:
> >     1. test_epoll
> >     2. test_epoll_sqpoll
> >     3. test_epoll_nodrop
> >     4. test_epoll_sqpoll_nodrop
> >
> > In the first 2 tests, I try to avoid to queue more requests than we have room
> > for in the CQ ring. These work fine, I have no faults.
>
> Thanks!
>
> > In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
> > much as I can until I get a -EBUSY, but they often fail in this way:
> > the submitter manages to submit everything, the receiver receives all the
> > submitted bytes, but the cleaner loses completion events (I also tried to put a
> > timeout to epoll_wait() in the cleaner to be sure that it is not related to the
> > patch that I send some weeks ago, but the situation doesn't change, it's like
> > there is still overflow in the CQ).
> >
> > Next week I'll try to investigate better which is the problem.
>
> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
> you just pruned the CQ ring but didn't flush the internal side.

Yes, If I use the io_uring_wait_cqe() instead of io_uring_peek_cqe() all
the tests work great, but it is blocking and the epoll_wait() it is used
only the first time.

>
> > I hope my test make sense, otherwise let me know what is wrong.
>
> I'll take a look...

Thanks!

>
> > Anyway, when I was exploring the library, I had a doubt:
> > - in the __io_uring_get_cqe() should we call sys_io_uring_enter() also if
> >   submit and wait_nr are zero, but IORING_SQ_NEED_WAKEUP is set in the
> >   sq.kflags?
>
> It's a submission side thing, the completion side shouldn't care. That
> flag is only relevant if you're submitting IO with SQPOLL. Then it tells
> you that the thread needs to get woken up, which you need io_uring_enter()
> to do. But for just reaping completions and not needing to submit
> anything new, we don't care if the thread is sleeping.

Thank you for clarifying that,
Stefano

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Stefano Garzarella]

On Fri, Jan 31, 2020 at 4:39 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> > Hi Jens,
> > this is a v2 of the epoll test.
> >
> > v1 -> v2:
> >     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
> >     - add 2 new tests to test epoll with IORING_FEAT_NODROP
> >     - cleanups
> >
> > There are 4 sub-tests:
> >     1. test_epoll
> >     2. test_epoll_sqpoll
> >     3. test_epoll_nodrop
> >     4. test_epoll_sqpoll_nodrop
> >
> > In the first 2 tests, I try to avoid to queue more requests than we have room
> > for in the CQ ring. These work fine, I have no faults.
>
> Thanks!
>
> > In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
> > much as I can until I get a -EBUSY, but they often fail in this way:
> > the submitter manages to submit everything, the receiver receives all the
> > submitted bytes, but the cleaner loses completion events (I also tried to put a
> > timeout to epoll_wait() in the cleaner to be sure that it is not related to the
> > patch that I send some weeks ago, but the situation doesn't change, it's like
> > there is still overflow in the CQ).
> >
> > Next week I'll try to investigate better which is the problem.
>
> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
> you just pruned the CQ ring but didn't flush the internal side.

If I do io_uring_enter() with GETEVENTS set and wait_nr = 0 it solves
the issue, I think because we call io_cqring_events() that flushes the
overflow list.

At this point, should we call io_cqring_events() (that flushes the
overflow list) in io_uring_poll()?
I mean something like this:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 77f22c3da30f..2769451af89a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6301,7 +6301,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
        if (READ_ONCE(ctx->rings->sq.tail) - ctx->cached_sq_head !=
            ctx->rings->sq_ring_entries)
                mask |= EPOLLOUT | EPOLLWRNORM;
-       if (READ_ONCE(ctx->rings->cq.head) != ctx->cached_cq_tail)
+       if (!io_cqring_events(ctx, false))
                mask |= EPOLLIN | EPOLLRDNORM;

        return mask;

Thanks,
Stefano

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Jens Axboe]

On 2/6/20 10:33 AM, Stefano Garzarella wrote:
> 
> 
> On Fri, Jan 31, 2020 at 4:39 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
>>> Hi Jens,
>>> this is a v2 of the epoll test.
>>>
>>> v1 -> v2:
>>>     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
>>>     - add 2 new tests to test epoll with IORING_FEAT_NODROP
>>>     - cleanups
>>>
>>> There are 4 sub-tests:
>>>     1. test_epoll
>>>     2. test_epoll_sqpoll
>>>     3. test_epoll_nodrop
>>>     4. test_epoll_sqpoll_nodrop
>>>
>>> In the first 2 tests, I try to avoid to queue more requests than we have room
>>> for in the CQ ring. These work fine, I have no faults.
>>
>> Thanks!
>>
>>> In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
>>> much as I can until I get a -EBUSY, but they often fail in this way:
>>> the submitter manages to submit everything, the receiver receives all the
>>> submitted bytes, but the cleaner loses completion events (I also tried to put a
>>> timeout to epoll_wait() in the cleaner to be sure that it is not related to the
>>> patch that I send some weeks ago, but the situation doesn't change, it's like
>>> there is still overflow in the CQ).
>>>
>>> Next week I'll try to investigate better which is the problem.
>>
>> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
>> you just pruned the CQ ring but didn't flush the internal side.
> 
> If I do io_uring_enter() with GETEVENTS set and wait_nr = 0 it solves
> the issue, I think because we call io_cqring_events() that flushes the
> overflow list.
> 
> At this point, should we call io_cqring_events() (that flushes the
> overflow list) in io_uring_poll()?
> I mean something like this:
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 77f22c3da30f..2769451af89a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6301,7 +6301,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
>         if (READ_ONCE(ctx->rings->sq.tail) - ctx->cached_sq_head !=
>             ctx->rings->sq_ring_entries)
>                 mask |= EPOLLOUT | EPOLLWRNORM;
> -       if (READ_ONCE(ctx->rings->cq.head) != ctx->cached_cq_tail)
> +       if (!io_cqring_events(ctx, false))
>                 mask |= EPOLLIN | EPOLLRDNORM;
> 
>         return mask;

That's not a bad idea, would just have to verify that it is indeed safe
to always call the flushing variant from there.

-- 
Jens Axboe

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Jens Axboe]

On 2/6/20 12:15 PM, Jens Axboe wrote:
> On 2/6/20 10:33 AM, Stefano Garzarella wrote:
>>
>>
>> On Fri, Jan 31, 2020 at 4:39 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
>>>> Hi Jens,
>>>> this is a v2 of the epoll test.
>>>>
>>>> v1 -> v2:
>>>>     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
>>>>     - add 2 new tests to test epoll with IORING_FEAT_NODROP
>>>>     - cleanups
>>>>
>>>> There are 4 sub-tests:
>>>>     1. test_epoll
>>>>     2. test_epoll_sqpoll
>>>>     3. test_epoll_nodrop
>>>>     4. test_epoll_sqpoll_nodrop
>>>>
>>>> In the first 2 tests, I try to avoid to queue more requests than we have room
>>>> for in the CQ ring. These work fine, I have no faults.
>>>
>>> Thanks!
>>>
>>>> In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
>>>> much as I can until I get a -EBUSY, but they often fail in this way:
>>>> the submitter manages to submit everything, the receiver receives all the
>>>> submitted bytes, but the cleaner loses completion events (I also tried to put a
>>>> timeout to epoll_wait() in the cleaner to be sure that it is not related to the
>>>> patch that I send some weeks ago, but the situation doesn't change, it's like
>>>> there is still overflow in the CQ).
>>>>
>>>> Next week I'll try to investigate better which is the problem.
>>>
>>> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
>>> you just pruned the CQ ring but didn't flush the internal side.
>>
>> If I do io_uring_enter() with GETEVENTS set and wait_nr = 0 it solves
>> the issue, I think because we call io_cqring_events() that flushes the
>> overflow list.
>>
>> At this point, should we call io_cqring_events() (that flushes the
>> overflow list) in io_uring_poll()?
>> I mean something like this:
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 77f22c3da30f..2769451af89a 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6301,7 +6301,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
>>         if (READ_ONCE(ctx->rings->sq.tail) - ctx->cached_sq_head !=
>>             ctx->rings->sq_ring_entries)
>>                 mask |= EPOLLOUT | EPOLLWRNORM;
>> -       if (READ_ONCE(ctx->rings->cq.head) != ctx->cached_cq_tail)
>> +       if (!io_cqring_events(ctx, false))
>>                 mask |= EPOLLIN | EPOLLRDNORM;
>>
>>         return mask;
> 
> That's not a bad idea, would just have to verify that it is indeed safe
> to always call the flushing variant from there.

Double checked, and it should be fine. We may be invoked with
ctx->uring_lock held, but that's fine.

-- 
Jens Axboe

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Stefano Garzarella]

On Thu, Feb 6, 2020 at 8:51 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 2/6/20 12:15 PM, Jens Axboe wrote:
> > On 2/6/20 10:33 AM, Stefano Garzarella wrote:
> >>
> >>
> >> On Fri, Jan 31, 2020 at 4:39 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>
> >>> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> >>>> Hi Jens,
> >>>> this is a v2 of the epoll test.
> >>>>
> >>>> v1 -> v2:
> >>>>     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
> >>>>     - add 2 new tests to test epoll with IORING_FEAT_NODROP
> >>>>     - cleanups
> >>>>
> >>>> There are 4 sub-tests:
> >>>>     1. test_epoll
> >>>>     2. test_epoll_sqpoll
> >>>>     3. test_epoll_nodrop
> >>>>     4. test_epoll_sqpoll_nodrop
> >>>>
> >>>> In the first 2 tests, I try to avoid to queue more requests than we have room
> >>>> for in the CQ ring. These work fine, I have no faults.
> >>>
> >>> Thanks!
> >>>
> >>>> In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
> >>>> much as I can until I get a -EBUSY, but they often fail in this way:
> >>>> the submitter manages to submit everything, the receiver receives all the
> >>>> submitted bytes, but the cleaner loses completion events (I also tried to put a
> >>>> timeout to epoll_wait() in the cleaner to be sure that it is not related to the
> >>>> patch that I send some weeks ago, but the situation doesn't change, it's like
> >>>> there is still overflow in the CQ).
> >>>>
> >>>> Next week I'll try to investigate better which is the problem.
> >>>
> >>> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
> >>> you just pruned the CQ ring but didn't flush the internal side.
> >>
> >> If I do io_uring_enter() with GETEVENTS set and wait_nr = 0 it solves
> >> the issue, I think because we call io_cqring_events() that flushes the
> >> overflow list.
> >>
> >> At this point, should we call io_cqring_events() (that flushes the
> >> overflow list) in io_uring_poll()?
> >> I mean something like this:
> >>
> >> diff --git a/fs/io_uring.c b/fs/io_uring.c
> >> index 77f22c3da30f..2769451af89a 100644
> >> --- a/fs/io_uring.c
> >> +++ b/fs/io_uring.c
> >> @@ -6301,7 +6301,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
> >>         if (READ_ONCE(ctx->rings->sq.tail) - ctx->cached_sq_head !=
> >>             ctx->rings->sq_ring_entries)
> >>                 mask |= EPOLLOUT | EPOLLWRNORM;
> >> -       if (READ_ONCE(ctx->rings->cq.head) != ctx->cached_cq_tail)
> >> +       if (!io_cqring_events(ctx, false))
> >>                 mask |= EPOLLIN | EPOLLRDNORM;
> >>
> >>         return mask;
> >
> > That's not a bad idea, would just have to verify that it is indeed safe
> > to always call the flushing variant from there.
>
> Double checked, and it should be fine. We may be invoked with
> ctx->uring_lock held, but that's fine.
>

Maybe yes, I'll check better and I'll send a patch :-)

Thanks,
Stefano

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Jens Axboe]

On 2/6/20 10:33 AM, Stefano Garzarella wrote:
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 77f22c3da30f..2769451af89a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6301,7 +6301,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
>         if (READ_ONCE(ctx->rings->sq.tail) - ctx->cached_sq_head !=
>             ctx->rings->sq_ring_entries)
>                 mask |= EPOLLOUT | EPOLLWRNORM;
> -       if (READ_ONCE(ctx->rings->cq.head) != ctx->cached_cq_tail)
> +       if (!io_cqring_events(ctx, false))
>                 mask |= EPOLLIN | EPOLLRDNORM;
> 
>         return mask;

Are you going to send this as a proper patch? If so, I think you'll want
to remove the '!' negation for that check.

-- 
Jens Axboe

Re: [PATCH liburing v2 0/1] test: add epoll test case

[Stefano Garzarella]

On Fri, Jan 31, 2020 at 08:39:46AM -0700, Jens Axboe wrote:
> On 1/31/20 7:29 AM, Stefano Garzarella wrote:
> > Hi Jens,
> > this is a v2 of the epoll test.
> > 
> > v1 -> v2:
> >     - if IORING_FEAT_NODROP is not available, avoid to overflow the CQ
> >     - add 2 new tests to test epoll with IORING_FEAT_NODROP
> >     - cleanups
> > 
> > There are 4 sub-tests:
> >     1. test_epoll
> >     2. test_epoll_sqpoll
> >     3. test_epoll_nodrop
> >     4. test_epoll_sqpoll_nodrop
> > 
> > In the first 2 tests, I try to avoid to queue more requests than we have room
> > for in the CQ ring. These work fine, I have no faults.
> 
> Thanks!
> 
> > In the tests 3 and 4, if IORING_FEAT_NODROP is supported, I try to submit as
> > much as I can until I get a -EBUSY, but they often fail in this way:
> > the submitter manages to submit everything, the receiver receives all the
> > submitted bytes, but the cleaner loses completion events (I also tried to put a
> > timeout to epoll_wait() in the cleaner to be sure that it is not related to the
> > patch that I send some weeks ago, but the situation doesn't change, it's like
> > there is still overflow in the CQ).
> > 
> > Next week I'll try to investigate better which is the problem.
> 
> Does it change if you have an io_uring_enter() with GETEVENTS set? I wonder if
> you just pruned the CQ ring but didn't flush the internal side.
> 

Just an update: after the "io_uring: flush overflowed CQ events in the
io_uring_poll()" the test 3 works well.

Now the problem is the test 4 (with sqpoll). It works in most cases, but it
fails a few times in this way:
- the submitter freezes after submitting X requests
- the cleaner and the consumer see X-2 requests (2 are the entries in
  the queue)

I tried to put a timeout on the submitter's epoll and do an io_uring_submit()
to wake up the kthread (if we lose some notifications), but the problem seems
to be somewhere else. I think a race somewhere.

Any suggestion on how to debug this case?
I'll try with tracing.

Thanks,
Stefano