| Message ID | 20241114050330.4006367-1-xue01.he@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [liburing,v2] test: add test cases for hybrid iopoll | expand |
On 11/13/24 10:03 PM, hexue wrote: > diff --git a/man/io_uring_setup.2 b/man/io_uring_setup.2 > index 2f87783..fa928fa 100644 > --- a/man/io_uring_setup.2 > +++ b/man/io_uring_setup.2 > @@ -78,7 +78,15 @@ in question. For NVMe devices, the nvme driver must be loaded with the > parameter set to the desired number of polling queues. The polling queues > will be shared appropriately between the CPUs in the system, if the number > is less than the number of online CPU threads. > - > +.TP > +.B IORING_SETUP_HYBRID_IOPOLL > +This flag must setup with This flag must be used with > +.B IORING_SETUP_IOPOLL > +flag. hybrid poll is a new Like before, skip new. Think about what happens when someone reads this in 5 years time. What does new mean? Yes it may be new now, but docs are supposed to be timeless. > +feature baed on iopoll, this could be a suboptimal solution when running based on > +on a single thread, it offers higher performance than IRQ and lower CPU > +utilization than polling. Similarly, this feature also requires the devices > +to support polling configuration. This doesn't explain how it works. I'd say something like: Hybrid io polling differs from strict polling in that it will delay a bit before doing completion side polling, to avoid wasting too much CPU. Like IOPOLL, it requires that devices support polling. > diff --git a/src/setup.c b/src/setup.c > index 073de50..d1a87aa 100644 > --- a/src/setup.c > +++ b/src/setup.c > @@ -320,6 +320,10 @@ int __io_uring_queue_init_params(unsigned entries, struct io_uring *ring, > ring->int_flags |= INT_FLAG_APP_MEM; > } > > + if ((p->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_HYBRID_IOPOLL)) == > + IORING_SETUP_HYBRID_IOPOLL) > + return -EINVAL; > + The kernel should already do this, no point duplicating it in liburing. The test bits look much better now, way simpler. I'll just need to double check that they handle EINVAL on setup properly, and EOPNOTSUPP at completion time will turn off further testing of it. Did you run it on configurations where hybrid io polling will both fail at setup time, and at runtime (eg the latter where the kernel supports it, but the device/fs does not)?
>On 11/13/24 10:03 PM, hexue wrote: >> diff --git a/man/io_uring_setup.2 b/man/io_uring_setup.2 >> index 2f87783..fa928fa 100644 ... > >This flag must be used with > >> +.B IORING_SETUP_IOPOLL >> +flag. hybrid poll is a new > >Like before, skip new. Think about what happens when someone reads this >in 5 years time. What does new mean? Yes it may be new now, but docs >are supposed to be timeless. > >> +feature baed on iopoll, this could be a suboptimal solution when running > >based on > >> +on a single thread, it offers higher performance than IRQ and lower CPU >> +utilization than polling. Similarly, this feature also requires the devices >> +to support polling configuration. > >This doesn't explain how it works. I'd say something like: > >Hybrid io polling differs from strict polling in that it will delay a >bit before doing completion side polling, to avoid wasting too much CPU. >Like IOPOLL, it requires that devices support polling. Thanks, will change the description. ... >> + return -EINVAL; >> + >The kernel should already do this, no point duplicating it in liburing. > >The test bits look much better now, way simpler. I'll just need to >double check that they handle EINVAL on setup properly, and EOPNOTSUPP >at completion time will turn off further testing of it. Did you run it >on configurations where hybrid io polling will both fail at setup time, >and at runtime (eg the latter where the kernel supports it, but the >device/fs does not)? Yes, I have run both of these error configurations. The running cases are: hybrid poll without IORING_SETUP_IOPOLL and device with incorrect queue configuration, EINVAL and EOPNOTSUPP are both identified. -- hexue
On 11/14/24 8:34 PM, hexue wrote: >> The kernel should already do this, no point duplicating it in liburing. >> >> The test bits look much better now, way simpler. I'll just need to >> double check that they handle EINVAL on setup properly, and EOPNOTSUPP >> at completion time will turn off further testing of it. Did you run it >> on configurations where hybrid io polling will both fail at setup time, >> and at runtime (eg the latter where the kernel supports it, but the >> device/fs does not)? > > Yes, I have run both of these error configurations. The running cases are: > hybrid poll without IORING_SETUP_IOPOLL and device with incorrect queue > configuration, EINVAL and EOPNOTSUPP are both identified. I figured that was the case, as the existing test case should already cover both of those cases. I'll get this applied once you send the updated version.
diff --git a/man/io_uring_setup.2 b/man/io_uring_setup.2 index 2f87783..fa928fa 100644 --- a/man/io_uring_setup.2 +++ b/man/io_uring_setup.2 @@ -78,7 +78,15 @@ in question. For NVMe devices, the nvme driver must be loaded with the parameter set to the desired number of polling queues. The polling queues will be shared appropriately between the CPUs in the system, if the number is less than the number of online CPU threads. - +.TP +.B IORING_SETUP_HYBRID_IOPOLL +This flag must setup with +.B IORING_SETUP_IOPOLL +flag. hybrid poll is a new +feature baed on iopoll, this could be a suboptimal solution when running +on a single thread, it offers higher performance than IRQ and lower CPU +utilization than polling. Similarly, this feature also requires the devices +to support polling configuration. .TP .B IORING_SETUP_SQPOLL When this flag is specified, a kernel thread is created to perform diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h index 20bc570..d16364c 100644 --- a/src/include/liburing/io_uring.h +++ b/src/include/liburing/io_uring.h @@ -200,6 +200,9 @@ enum io_uring_sqe_flags_bit { */ #define IORING_SETUP_NO_SQARRAY (1U << 16) +/* Use hybrid poll in iopoll process */ +#define IORING_SETUP_HYBRID_IOPOLL (1U << 17) + enum io_uring_op { IORING_OP_NOP, IORING_OP_READV, diff --git a/src/setup.c b/src/setup.c index 073de50..d1a87aa 100644 --- a/src/setup.c +++ b/src/setup.c @@ -320,6 +320,10 @@ int __io_uring_queue_init_params(unsigned entries, struct io_uring *ring, ring->int_flags |= INT_FLAG_APP_MEM; } + if ((p->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_HYBRID_IOPOLL)) == + IORING_SETUP_HYBRID_IOPOLL) + return -EINVAL; + fd = __sys_io_uring_setup(entries, p); if (fd < 0) { if ((p->flags & IORING_SETUP_NO_MMAP) && diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c index f18a186..8604c42 100644 --- a/test/io_uring_passthrough.c +++ b/test/io_uring_passthrough.c @@ -254,7 +254,7 @@ err: } static int test_io(const char *file, int tc, int read, int sqthread, - int fixed, int nonvec) + int fixed, int nonvec, int hybrid) { struct io_uring ring; int ret, ring_flags = 0; @@ -265,6 +265,9 @@ static int test_io(const char *file, int tc, int read, int sqthread, if (sqthread) ring_flags |= IORING_SETUP_SQPOLL; + if (hybrid) + ring_flags |= IORING_SETUP_IOPOLL | IORING_SETUP_HYBRID_IOPOLL; + ret = t_create_ring(64, &ring, ring_flags); if (ret == T_SETUP_SKIP) return 0; @@ -449,18 +452,19 @@ int main(int argc, char *argv[]) vecs = t_create_buffers(BUFFERS, BS); - for (i = 0; i < 16; i++) { + for (i = 0; i < 32; i++) { int read = (i & 1) != 0; int sqthread = (i & 2) != 0; int fixed = (i & 4) != 0; int nonvec = (i & 8) != 0; + int hybrid = (i & 16) != 0; - ret = test_io(fname, i, read, sqthread, fixed, nonvec); + ret = test_io(fname, i, read, sqthread, fixed, nonvec, hybrid); if (no_pt) break; if (ret) { - fprintf(stderr, "test_io failed %d/%d/%d/%d\n", - read, sqthread, fixed, nonvec); + fprintf(stderr, "test_io failed %d/%d/%d/%d%d\n", + read, sqthread, fixed, nonvec, hybrid); goto err; } } diff --git a/test/iopoll.c b/test/iopoll.c index 2e0f7ea..0d7bd77 100644 --- a/test/iopoll.c +++ b/test/iopoll.c @@ -351,7 +351,7 @@ ok: } static int test_io(const char *file, int write, int sqthread, int fixed, - int buf_select, int defer) + int hybrid, int buf_select, int defer) { struct io_uring ring; int ret, ring_flags = IORING_SETUP_IOPOLL; @@ -363,6 +363,9 @@ static int test_io(const char *file, int write, int sqthread, int fixed, ring_flags |= IORING_SETUP_SINGLE_ISSUER | IORING_SETUP_DEFER_TASKRUN; + if (hybrid) + ring_flags |= IORING_SETUP_HYBRID_IOPOLL; + ret = t_create_ring(64, &ring, ring_flags); if (ret == T_SETUP_SKIP) return 0; @@ -418,22 +421,23 @@ int main(int argc, char *argv[]) vecs = t_create_buffers(BUFFERS, BS); - nr = 32; + nr = 64; if (no_buf_select) - nr = 8; - else if (!t_probe_defer_taskrun()) nr = 16; + else if (!t_probe_defer_taskrun()) + nr = 32; for (i = 0; i < nr; i++) { int write = (i & 1) != 0; int sqthread = (i & 2) != 0; int fixed = (i & 4) != 0; - int buf_select = (i & 8) != 0; - int defer = (i & 16) != 0; + int hybrid = (i & 8) != 0; + int buf_select = (i & 16) != 0; + int defer = (i & 32) != 0; - ret = test_io(fname, write, sqthread, fixed, buf_select, defer); + ret = test_io(fname, write, sqthread, fixed, hybrid, buf_select, defer); if (ret) { - fprintf(stderr, "test_io failed %d/%d/%d/%d/%d\n", - write, sqthread, fixed, buf_select, defer); + fprintf(stderr, "test_io failed %d/%d/%d/%d/%d%d\n", + write, sqthread, fixed, hybrid, buf_select, defer); goto err; } if (no_iopoll)
Add a test file for hybrid iopoll to make sure it works safe.Test case include basic read/write tests, and run in normal iopoll mode and passthrough mode respectively. -- changes since v1: - remove iopoll-hybridpoll.c - test hybrid poll with exsiting iopoll and io_uring_passthrough - add a misconfiguration check Signed-off-by: hexue <xue01.he@samsung.com> --- man/io_uring_setup.2 | 10 +++++++++- src/include/liburing/io_uring.h | 3 +++ src/setup.c | 4 ++++ test/io_uring_passthrough.c | 14 +++++++++----- test/iopoll.c | 22 +++++++++++++--------- 5 files changed, 38 insertions(+), 15 deletions(-)