public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: David Wei <[email protected]>,
	Pavel Begunkov <[email protected]>,
	[email protected]
Subject: Re: [PATCH v1 1/4] io_uring: only account cqring wait time as iowait if enabled for a ring
Date: Sat, 24 Feb 2024 11:55:50 -0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 2/24/24 10:20 AM, David Wei wrote:
>> I don't believe it's a sane approach. I think we agree that per
>> cpu iowait is a silly and misleading metric. I have hard time to
>> define what it is, and I'm sure most probably people complaining
>> wouldn't be able to tell as well. Now we're taking that metric
>> and expose even more knobs to userspace.
>>
>> Another argument against is that per ctx is not the right place
>> to have it. It's a system metric, and you can imagine some system
>> admin looking for it. Even in cases when had some meaning w/o
>> io_uring now without looking at what flags io_uring has it's
>> completely meaningless, and it's too much to ask.> 
>> I don't understand why people freak out at seeing hi iowait,
>> IMHO it perfectly fits the definition of io_uring waiting for
>> IO / completions, but at this point it might be better to just
>> revert it to the old behaviour of not reporting iowait at all.
> 
> Irrespective of how misleading iowait is, many tools include it in its
> CPU util/load calculations and users then use those metrics for e.g.
> load balancing. In situations with storage workloads, iowait can be
> useful even if its usefulness is limited. The problem that this patch is
> trying to resolve is in mixed storage/network workloads on the same
> system, where iowait has some usefulness (due to storage workloads)
> _but_ I don't want network workloads contributing to the metric.
> 
> This does put the onus on userspace to do the right thing - decide
> whether iowait makes sense for a workload or not. I don't have enough
> kernel experience to know whether this expectation is realistic or not.
> But, it is turned off by default so if userspace does not set it (which
> seems like the most likely thing) then iowait accounting is off just
> like the old behaviour. Perhaps we need to make it clearer to storage
> use-cases to turn it on in order to get the optimisation?

Personally I don't care too much about per-ctx iowait, I don't think
it's an issue at all. Fact is, most workloads that do storage and
networking would likely use a ring for each. And if they do mix, then
just pick if you care about iowait or not. Long term, would the toggle
iowait thing most likely just go away? Yep it would. But it's not like
it's any kind of maintenance burden. tldr - if we can do cpufreq
boosting easily on waits without adding iowait to the mix, then that'd
be great and we can just do that. If not, let's add the iowait toggle
and just be done with it.

>> And if we want to save the cpu freq iowait optimisation, we
>> should just split notion of iowait reporting and iowait cpufreq
>> tuning.
> 
> Yeah, that could be an option. I'll take a look at it.

It'd be trivial to do, only issue I see is that it'd require another set
of per-runqueue atomics to count for short waits on top of the
nr_iowaits we already do. I doubt the scheduling side will be receptive
to that.

-- 
Jens Axboe


  reply	other threads:[~2024-02-24 18:55 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-24  5:07 [PATCH v1 1/4] io_uring: only account cqring wait time as iowait if enabled for a ring David Wei
2024-02-24  5:07 ` [PATCH v1 2/4] liburing: add examples/proxy to .gitignore David Wei
2024-02-24  5:07 ` [PATCH v1 3/4] liburing: add helper for IORING_REGISTER_IOWAIT David Wei
2024-02-24 15:29   ` Jens Axboe
2024-02-24 16:39     ` David Wei
2024-02-24  5:07 ` [PATCH v1 4/4] liburing: add unit test for io_uring_register_iowait() David Wei
2024-02-24 15:28 ` [PATCH v1 1/4] io_uring: only account cqring wait time as iowait if enabled for a ring Jens Axboe
2024-02-24 15:31 ` Pavel Begunkov
2024-02-24 17:20   ` David Wei
2024-02-24 18:55     ` Jens Axboe [this message]
2024-02-25  1:39       ` Pavel Begunkov
2024-02-25 16:43         ` Jens Axboe
2024-02-25 21:11           ` Jens Axboe
2024-02-25 21:33             ` Jens Axboe
2024-02-26 14:56             ` Pavel Begunkov
2024-02-26 15:22               ` Jens Axboe
2024-02-24 18:51   ` Jens Axboe
2024-02-25  0:58     ` Pavel Begunkov
2024-02-25 16:39       ` Jens Axboe
2024-03-05 14:59         ` Christian Loehle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox