linux-kernel - Re: [PATCH liburing v2] test: add test cases for hybrid iopoll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3aada5a2-074a-45e8-882c-0302cae4c41b@kernel.dk>
Date: Thu, 14 Nov 2024 08:21:47 -0700
From: Jens Axboe <axboe@...nel.dk>
To: hexue <xue01.he@...sung.com>, asml.silence@...il.com
Cc: io-uring@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH liburing v2] test: add test cases for hybrid iopoll

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)?

-- 
Jens Axboe