On 2/24/24 5:58 PM, Pavel Begunkov wrote: > On 2/24/24 18:51, Jens Axboe wrote: >> On 2/24/24 8:31 AM, Pavel Begunkov wrote: >>> On 2/24/24 05:07, David Wei wrote: >>>> Currently we unconditionally account time spent waiting for events in CQ >>>> ring as iowait time. >>>> >>>> Some userspace tools consider iowait time to be CPU util/load which can >>>> be misleading as the process is sleeping. High iowait time might be >>>> indicative of issues for storage IO, but for network IO e.g. socket >>>> recv() we do not control when the completions happen so its value >>>> misleads userspace tooling. >>>> >>>> This patch gates the previously unconditional iowait accounting behind a >>>> new IORING_REGISTER opcode. By default time is not accounted as iowait, >>>> unless this is explicitly enabled for a ring. Thus userspace can decide, >>>> depending on the type of work it expects to do, whether it wants to >>>> consider cqring wait time as iowait or not. >>> >>> 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. >> >> For sure, it's a stupid metric. But at the same time, educating people >> on this can be like talking to a brick wall, and it'll be years of doing >> that before we're making a dent in it. Hence I do think that just >> exposing the knob and letting the storage side use it, if they want, is >> the path of least resistance. I'm personally not going to do a crusade >> on iowait to eliminate it, I don't have the time for that. I'll educate > > Exactly my point but with a different conclusion. The path of least I think that's because I'm a realist, and you are an idealist ;-) > resistance is to have io_uring not accounted to iowait. That's how > it was so nobody should complain about it, you don't have to care about > it at all, you don't have to educate people on iowait when it comes up > with in the context of that knob, and you don't have to educate folks > on what this knob is and wtf it's there, and we're not pretending that > it works when it's not. I don't think anyone cares about iowait going away for waiting on events with io_uring, but some would very much care about losing the cpufreq connection which is why it got added in the first place. If we can trivially do that without iowait, then we should certainly just do that and call it good. THAT is the main question to answer, in form of a patch. >> people when it comes up, like I have been doing, but pulling this to >> conclusion would be 10+ years easily. >> >>> 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. >>> And if we want to save the cpu freq iowait optimisation, we >>> should just split notion of iowait reporting and iowait cpufreq >>> tuning. >> >> For io_uring, splitting the cpufreq from iowait is certainly the right >> approach. And then just getting rid of iowait completely on the io_uring >> side. This can be done without preaching about iowait to everyone that >> has bad metrics for their healt monitoring, which is why I like that a >> lot. I did ponder that the other day as well. >> >> You still kind of run into a problem with that in terms of when short vs >> long waits are expected. On the io_uring side, we use the "do I have >> any requests pending" for that, which is obviously not fine grained >> enough. We could apply it on just "do I have any requests against >> regular files" instead, which would then translate to needing further >> tracking on our side. Probably fine to just apply it for the existing >> logic, imho. > > Let's say there are two problems, one is the accounting mess, which > is IMHO clear. The second is the optimisation, which is not mentioned > in the patch and kind of an orthogonal issue. If we want a knob to > disable/enable the cpufreq thing, it should be separate from iowait > accounting, because it sounds like a really unfortunate side effect > when you enable the optimisation and the iowait goes pounding the roof. > Then people never touch it, especially in a framework, because an > system admin will be pretty surprised by the metric. Fully agree, it's all about not losing the short sleep high latency wakeup, which is what caused the performance to drop for low QD IOs. > Not like I'm a fan of the idea of having a userspace knob for the > optimisation, it'd be pretty complicated to use, and hopefully there > is a more intelligent approach. That would certainly be nice. -- Jens Axboe
