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

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

[Stefano Garzarella]

Hi Jens,
I wrote the test case for epoll.

Since it fails also without sqpoll (Linux 5.4.13-201.fc31.x86_64),
can you take a look to understand if the test is wrong?

Tomorrow I'll travel, but on Wednesday I'll try this test with the patch
that I sent and also with the upstream kernel.

Thanks,
Stefano

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

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

-- 
2.24.1

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

[Stefano Garzarella]

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 .gitignore    |   1 +
 test/Makefile |   5 +-
 test/epoll.c  | 307 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100644 test/epoll.c

diff --git a/.gitignore b/.gitignore
index fdb4b32..76170c9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,6 +36,7 @@
 /test/d77a67ed5f27-test
 /test/defer
 /test/eeed8b54e0df-test
+/test/epoll
 /test/fc2a85cb02ef-test
 /test/file-register
 /test/fixed-link
diff --git a/test/Makefile b/test/Makefile
index efdc3aa..773ba0e 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
+		short-read openat2 probe shared-wq epoll
 
 include ../Makefile.quiet
 
@@ -45,7 +45,7 @@ test_srcs := poll.c poll-cancel.c ring-leak.c fsync.c io_uring_setup.c \
 	b5837bd5311d-test.c accept-test.c d77a67ed5f27-test.c connect.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
+	madvise.c short-read.c openat2.c probe.c shared-wq.c epoll.c
 
 test_objs := $(patsubst %.c,%.ol,$(test_srcs))
 
@@ -56,6 +56,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..d082f21
--- /dev/null
+++ b/test/epoll.c
@@ -0,0 +1,307 @@
+/*
+ * Description: test io_uring poll handling using a pipe
+ *
+ *              Three threads involved:
+ *              - producer: fills SQ with write requests for the pipe
+ *              - freer: consume CQ, freeing the buffers that producer allocates
+ *              - consumer: read() blocking on the pipe
+ *
+ */
+#include <errno.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 	2
+#define BUF_SIZE        16
+#define ITERATIONS      100
+
+struct thread_data {
+	struct io_uring *ring;
+	int pipe_read;
+	int pipe_write;
+	bool sqpoll;
+};
+
+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;
+	int fd, ret, iter = 0;
+	void *th_ret = (void *)1;
+
+	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 (iter < ITERATIONS) {
+		bool submit = false;
+
+		ret = epoll_wait(fd, &ev, 1, -1);
+		if (ret < 0) {
+			perror("epoll_wait");
+			goto ret;
+		}
+
+		while (iter < ITERATIONS &&
+		       (sqe = io_uring_get_sqe(td->ring))) {
+			struct iovec *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);
+			iter++;
+			submit = true;
+		}
+
+		if (!submit)
+			continue;
+
+		ret = io_uring_submit(td->ring);
+		if (ret <= 0) {
+			fprintf(stderr, "child: sqe submit failed - ret: %d\n",
+				ret);
+			goto ret;
+		}
+	}
+
+	printf("Successfully submitted %d requests\n", iter);
+
+	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, iter = 0;
+	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 (iter < ITERATIONS) {
+		ret = epoll_wait(fd, &ev, 1, -1);
+		if (ret < 0) {
+			perror("epoll_wait");
+			goto ret;
+		}
+
+		while (iter < ITERATIONS) {
+			struct iovec *vec;
+
+			ret = io_uring_peek_cqe(td->ring, &cqe);
+			if (ret) {
+				if (ret != -EAGAIN) {
+					goto ret;
+				}
+				break;
+			}
+
+			vec = io_uring_cqe_get_data(cqe);
+			free_vec(vec);
+			io_uring_cqe_seen(td->ring, cqe);
+			iter++;
+		}
+	}
+
+	printf("Successfully completed %d requests\n", iter);
+
+	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)
+{
+	int ret, pipe1[2],flags = 0;
+	struct thread_data td;
+	pthread_t threads[3];
+	struct io_uring ring;
+	void *ret_th[3];
+
+	if (pipe(pipe1) != 0) {
+		perror("pipe");
+		return 1;
+	}
+
+	td.sqpoll = sqpoll;
+	td.pipe_read = pipe1[0];
+	td.pipe_write = pipe1[1];
+
+	if (td.sqpoll)
+		flags |= IORING_SETUP_SQPOLL;
+
+	ret = io_uring_queue_init(1, &ring, flags);
+	if (ret) {
+		fprintf(stderr, "ring setup failed\n");
+		return 1;
+	}
+
+	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);
+			return 1;
+		}
+
+		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");
+		return 1;
+	}
+
+	close(pipe1[0]);
+	close(pipe1[1]);
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction act;
+	int ret, no_sqthread = 0;
+
+	memset(&act, 0, sizeof(act));
+	act.sa_handler = sig_alrm;
+	act.sa_flags = SA_RESTART;
+	sigaction(SIGALRM, &act, NULL);
+	alarm(TIMEOUT);
+
+	if (geteuid()) {
+		no_sqthread = 1;
+	}
+
+	if (no_sqthread) {
+		printf("test_epoll_sqpoll: skipped, not root\n");
+	} else {
+		ret = do_test_epoll(true);
+		if (ret) {
+			fprintf(stderr, "test_epoll_sqpoll failed\n");
+			return ret;
+		}
+	}
+
+	ret = do_test_epoll(false);
+	if (ret) {
+		fprintf(stderr, "test_epoll failed\n");
+		return ret;
+	}
+
+	return 0;
+}
-- 
2.24.1

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

[Jens Axboe]

On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> Hi Jens,
> I wrote the test case for epoll.
> 
> Since it fails also without sqpoll (Linux 5.4.13-201.fc31.x86_64),
> can you take a look to understand if the test is wrong?
> 
> Tomorrow I'll travel, but on Wednesday I'll try this test with the patch
> that I sent and also with the upstream kernel.

I'll take a look, but your patches are coming through garbled and don't
apply.

-- 
Jens Axboe

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

[Jens Axboe]

On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

You're not reaping CQ events, and hence you overflow the ring. Once
overflown, an attempt to submit new IO will returns in a -16/-EBUSY
return value. This is io_uring telling you that it won't submit more
IO until you've emptied the completion ring so io_uring can flush
the overflown entries to the ring.

-- 
Jens Axboe

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

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 09:26:41AM -0700, Jens Axboe wrote:
> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> > Hi Jens,
> > I wrote the test case for epoll.
> > 
> > Since it fails also without sqpoll (Linux 5.4.13-201.fc31.x86_64),
> > can you take a look to understand if the test is wrong?
> > 
> > Tomorrow I'll travel, but on Wednesday I'll try this test with the patch
> > that I sent and also with the upstream kernel.
> 
> I'll take a look, but your patches are coming through garbled and don't
> apply.

Weird, I'm using git-publish as usual. I tried to download the patch
received from the ML, and I tried to reapply and it seams to work here.

Which kind of issue do you have? (just to fix my setup)

Anyway I pushed my tree here:
    https://github.com/stefano-garzarella/liburing.git epoll

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

[Jens Axboe]

On 1/27/20 11:00 AM, Stefano Garzarella wrote:
> On Mon, Jan 27, 2020 at 09:26:41AM -0700, Jens Axboe wrote:
>> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
>>> Hi Jens,
>>> I wrote the test case for epoll.
>>>
>>> Since it fails also without sqpoll (Linux 5.4.13-201.fc31.x86_64),
>>> can you take a look to understand if the test is wrong?
>>>
>>> Tomorrow I'll travel, but on Wednesday I'll try this test with the patch
>>> that I sent and also with the upstream kernel.
>>
>> I'll take a look, but your patches are coming through garbled and don't
>> apply.
> 
> Weird, I'm using git-publish as usual. I tried to download the patch
> received from the ML, and I tried to reapply and it seams to work here.
> 
> Which kind of issue do you have? (just to fix my setup)

First I grabbed it from email, and I get the usual =3D (instead of =)
and =20 instead of a space. Longer lines also broken up, with an = at
the end.

Then I grabbed it from the lore io-uring archive, but it was the exact
same thing.

> Anyway I pushed my tree here:
>     https://github.com/stefano-garzarella/liburing.git epoll

As per other email, I think you're having some coordination issues
with the reaping and submitting side being separated. If the reaper
isn't keeping up, you'll get the -EBUSY problem I saw. I'm assuming
that's the failure case you are also seeing, you didn't actually
mention how it fails for you?

-- 
Jens Axboe

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

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 09:32:43AM -0700, Jens Axboe wrote:
> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> You're not reaping CQ events, and hence you overflow the ring. Once
> overflown, an attempt to submit new IO will returns in a -16/-EBUSY
> return value. This is io_uring telling you that it won't submit more
> IO until you've emptied the completion ring so io_uring can flush
> the overflown entries to the ring.

How can I reaping CQ events? (I was hoping the epoll would help me with that)

What I'm seeing is that the producer (EPOLLOUT) can fill the SQ without issues,
the consumer (read()) is receiving all the buffers produced, but the thread
that frees the buffers (EPOLLIN) is not woken up.

I tried to set a timeout to the epoll_wait(), but the io_uring_peek_cqe()
returns -EAGAIN.

If I'm using a ring with 16 entries, it seems to work better, but
sometimes I lose events and the thread that frees the buffer doesn't wake up.

Maybe I'm missing something...

Thanks,
Stefano

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

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 11:07:41AM -0700, Jens Axboe wrote:
> On 1/27/20 11:00 AM, Stefano Garzarella wrote:
> > On Mon, Jan 27, 2020 at 09:26:41AM -0700, Jens Axboe wrote:
> >> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> >>> Hi Jens,
> >>> I wrote the test case for epoll.
> >>>
> >>> Since it fails also without sqpoll (Linux 5.4.13-201.fc31.x86_64),
> >>> can you take a look to understand if the test is wrong?
> >>>
> >>> Tomorrow I'll travel, but on Wednesday I'll try this test with the patch
> >>> that I sent and also with the upstream kernel.
> >>
> >> I'll take a look, but your patches are coming through garbled and don't
> >> apply.
> > 
> > Weird, I'm using git-publish as usual. I tried to download the patch
> > received from the ML, and I tried to reapply and it seams to work here.
> > 
> > Which kind of issue do you have? (just to fix my setup)
> 
> First I grabbed it from email, and I get the usual =3D (instead of =)
> and =20 instead of a space. Longer lines also broken up, with an = at
> the end.
> 
> Then I grabbed it from the lore io-uring archive, but it was the exact
> same thing.

I saw! I'll try to fix my setup.
The strange thing is that my git (v2.24.1) is able to apply that
malformed patch!

> 
> > Anyway I pushed my tree here:
> >     https://github.com/stefano-garzarella/liburing.git epoll
> 
> As per other email, I think you're having some coordination issues
> with the reaping and submitting side being separated. If the reaper
> isn't keeping up, you'll get the -EBUSY problem I saw. I'm assuming
> that's the failure case you are also seeing, you didn't actually
> mention how it fails for you?

My fault, I sent more information on the issue that I'm seeing.

Thanks,
Stefano

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

[Jens Axboe]

On 1/27/20 11:25 AM, Stefano Garzarella wrote:
> On Mon, Jan 27, 2020 at 09:32:43AM -0700, Jens Axboe wrote:
>> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>
>> You're not reaping CQ events, and hence you overflow the ring. Once
>> overflown, an attempt to submit new IO will returns in a -16/-EBUSY
>> return value. This is io_uring telling you that it won't submit more
>> IO until you've emptied the completion ring so io_uring can flush
>> the overflown entries to the ring.
> 
> How can I reaping CQ events? (I was hoping the epoll would help me with that)
> 
> What I'm seeing is that the producer (EPOLLOUT) can fill the SQ without issues,
> the consumer (read()) is receiving all the buffers produced, but the thread
> that frees the buffers (EPOLLIN) is not woken up.
> 
> I tried to set a timeout to the epoll_wait(), but the io_uring_peek_cqe()
> returns -EAGAIN.
> 
> If I'm using a ring with 16 entries, it seems to work better, but
> sometimes I lose events and the thread that frees the buffer doesn't wake up.
> 
> Maybe I'm missing something...

OK, so that helps in terms of understanding the issue you are seeing with
it. I'll take a look at this, but it'll probably be a few days. You can
try and enable tracing, I see events completed just fine. Maybe a race
with your epoll wait and event reaping?

-- 
Jens Axboe

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

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 11:46:34AM -0700, Jens Axboe wrote:
> On 1/27/20 11:25 AM, Stefano Garzarella wrote:
> > On Mon, Jan 27, 2020 at 09:32:43AM -0700, Jens Axboe wrote:
> >> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >>
> >> You're not reaping CQ events, and hence you overflow the ring. Once
> >> overflown, an attempt to submit new IO will returns in a -16/-EBUSY
> >> return value. This is io_uring telling you that it won't submit more
> >> IO until you've emptied the completion ring so io_uring can flush
> >> the overflown entries to the ring.
> > 
> > How can I reaping CQ events? (I was hoping the epoll would help me with that)
> > 
> > What I'm seeing is that the producer (EPOLLOUT) can fill the SQ without issues,
> > the consumer (read()) is receiving all the buffers produced, but the thread
> > that frees the buffers (EPOLLIN) is not woken up.
> > 
> > I tried to set a timeout to the epoll_wait(), but the io_uring_peek_cqe()
> > returns -EAGAIN.
> > 
> > If I'm using a ring with 16 entries, it seems to work better, but
> > sometimes I lose events and the thread that frees the buffer doesn't wake up.
> > 
> > Maybe I'm missing something...
> 
> OK, so that helps in terms of understanding the issue you are seeing with
> it. I'll take a look at this, but it'll probably be a few days. You can

Sure, take your time!

> try and enable tracing, I see events completed just fine. Maybe a race
> with your epoll wait and event reaping?

Could be. I'll try to investigate better enabling the tracing!

Thanks,
Stefano

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

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 11:46:34AM -0700, Jens Axboe wrote:
> On 1/27/20 11:25 AM, Stefano Garzarella wrote:
> > On Mon, Jan 27, 2020 at 09:32:43AM -0700, Jens Axboe wrote:
> >> On 1/27/20 9:17 AM, Stefano Garzarella wrote:
> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >>
> >> You're not reaping CQ events, and hence you overflow the ring. Once
> >> overflown, an attempt to submit new IO will returns in a -16/-EBUSY
> >> return value. This is io_uring telling you that it won't submit more
> >> IO until you've emptied the completion ring so io_uring can flush
> >> the overflown entries to the ring.
> > 
> > How can I reaping CQ events? (I was hoping the epoll would help me with that)
> > 
> > What I'm seeing is that the producer (EPOLLOUT) can fill the SQ without issues,
> > the consumer (read()) is receiving all the buffers produced, but the thread
> > that frees the buffers (EPOLLIN) is not woken up.
> > 
> > I tried to set a timeout to the epoll_wait(), but the io_uring_peek_cqe()
> > returns -EAGAIN.
> > 
> > If I'm using a ring with 16 entries, it seems to work better, but
> > sometimes I lose events and the thread that frees the buffer doesn't wake up.
> > 
> > Maybe I'm missing something...
> 
> OK, so that helps in terms of understanding the issue you are seeing with
> it. I'll take a look at this, but it'll probably be a few days. You can
> try and enable tracing, I see events completed just fine. Maybe a race
> with your epoll wait and event reaping?

(discard previous email wrongly sent by my phone, sorry for the noise)

Okay, the issue was that my kernel doesn’t support IORING_FEAT_NODROP,
so for this reason I missed the CQ events.

Avoiding the CQ overflow keeping this condition true
(submitted - completed < n_entries), solves the issue.

I’ll try with a mainline kernel, handling also the -EBUSY returned by
io_uring_submit().

Thanks,
Stefano