Re: [PATCH v2] io_uring: add IORING_ENTER_NO_IOWAIT to not set in_iowait

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/16/24 23:36, David Wei wrote:
> io_uring sets current->in_iowait when waiting for completions, which
> achieves two things:
> 
> 1. Proper accounting of the time as iowait time
> 2. Enable cpufreq optimisations, setting SCHED_CPUFREQ_IOWAIT on the rq

"achieve" is not the right word, nobody wanted 1. and it's not
"proper accounting" but rather an unfortunate side effect.

> For block IO this makes sense as high iowait can be indicative of
> issues. But for network IO especially recv, the recv side does not
> control when the completions happen.
> 
> Some user tooling attributes iowait time as CPU utilisation i.e. not
> idle, so high iowait time looks like high CPU util even though the task
> is not scheduled and the CPU is free to run other tasks. When doing
> network IO with e.g. the batch completion feature, the CPU may appear to
> have high utilisation.

How "batch completion" came into the picture? It elevates
iowait for any net apps, we have enough reports about it.


> This patchset adds a IOURING_ENTER_NO_IOWAIT flag that can be set on
> enter. If set, then current->in_iowait is not set. By default this flag

A worthwhile change but for _completely_ different reasons. So, first,
it's v3, not v2, considering the patchset from a couple month ago. And
since in essence nothing has changed, I can only repeat same points I
made back then.

The description reads like the flag's purpose is to change accounting,
and I'm vividly oppose any user exposed (per ring) toggle doing that.
We don't want the overhead, it's a very confusing feature, and not even
that helpful. iowait is monitored not by the app itself but by someone
else outside, likely by a different person, and even before trying to
make sense of numbers the monitor would need to learn first whether
_any_ program uses io_uring and what flags the application writer
decided to pass, even more fun when io_uring is used via a 3rd party
library.

Exactly same patches could make sense if you flip the description
and say "in_iowait is good for perfomance in some cases but
degrades power consumption for others, so here is a way to tune
performance", (just take Jamal's numbers). And that would need to
clearly state (including man) that the iowait statistic is a side
effect of it, we don't give it a thought, and the time accounting
aspect may and hopefully will change in the future.

Jens, can you remind what happened with separating iowait stats
vs the optimisation? I believed you sent some patches

> is not set to maintain existing behaviour i.e. in_iowait is always set.
> This is to prevent waiting for completions being accounted as CPU
> utilisation.

For accounting, it's more reasonable to keep it disabled by
default, so we stop getting millions complaints per day about
high iowait.

> Not setting in_iowait does mean that we also lose cpufreq optimisations
> above because in_iowait semantics couples 1 and 2 together. Eventually
> we will untangle the two so the optimisations can be enabled
> independently of the accounting.
> 
> IORING_FEAT_IOWAIT_TOGGLE is returned in io_uring_create() to indicate
> support. This will be used by liburing to check for this feature.
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
> v2:
>   - squash patches into one
>   - move no_iowait in struct io_wait_queue to the end
>   - always set iowq.no_iowait
> 
> ---
>   include/uapi/linux/io_uring.h | 2 ++
>   io_uring/io_uring.c           | 7 ++++---
>   io_uring/io_uring.h           | 1 +
>   3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 48c440edf674..3a94afa8665e 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -508,6 +508,7 @@ struct io_cqring_offsets {
>   #define IORING_ENTER_EXT_ARG		(1U << 3)
>   #define IORING_ENTER_REGISTERED_RING	(1U << 4)
>   #define IORING_ENTER_ABS_TIMER		(1U << 5)
> +#define IORING_ENTER_NO_IOWAIT		(1U << 6)

Just curious, why did we switch from a register opcode to an
ENTER flag?


-- 
Pavel Begunkov