RE: [PATCH] aio: make aio wait path to account iowait time

[Tianxianting <tian.xianting@h3c.com>]

Thanks peterz, jan
So, enable aio iowait time accounting is a bad idea:(
I gained a lot from you, thanks

-----Original Message-----
From: peterz@infradead.org [mailto:peterz@infradead.org] 
Sent: Friday, August 28, 2020 6:52 PM
To: Jan Kara <jack@suse.cz>
Cc: tianxianting (RD) <tian.xianting@h3c.com>; viro@zeniv.linux.org.uk; bcrl@kvack.org; mingo@redhat.com; juri.lelli@redhat.com; vincent.guittot@linaro.org; dietmar.eggemann@arm.com; rostedt@goodmis.org; bsegall@google.com; mgorman@suse.de; linux-fsdevel@vger.kernel.org; linux-aio@kvack.org; linux-kernel@vger.kernel.org; Tejun Heo <tj@kernel.org>; hannes@cmpxchg.org
Subject: Re: [PATCH] aio: make aio wait path to account iowait time

On Fri, Aug 28, 2020 at 11:41:29AM +0200, Jan Kara wrote:
> On Fri 28-08-20 11:07:29, peterz@infradead.org wrote:
> > On Fri, Aug 28, 2020 at 02:07:12PM +0800, Xianting Tian wrote:
> > > As the normal aio wait path(read_events() ->
> > > wait_event_interruptible_hrtimeout()) doesn't account iowait time, 
> > > so use this patch to make it to account iowait time, which can 
> > > truely reflect the system io situation when using a tool like 'top'.
> > 
> > Do be aware though that io_schedule() is potentially far more 
> > expensive than regular schedule() and io-wait accounting as a whole 
> > is a trainwreck.
> 
> Hum, I didn't know that io_schedule() is that much more expensive. 
> Thanks for info.

It's all relative, but it can add up under contention. And since these storage thingies are getting faster every year, I'm assuming these schedule rates are increasing along with it.

> > When in_iowait is set schedule() and ttwu() will have to do 
> > additional atomic ops, and (much) worse, PSI will take additional locks.
> > 
> > And all that for a number that, IMO, is mostly useless, see the 
> > comment with nr_iowait().
> 
> Well, I understand the limited usefulness of the system or even per 
> CPU percentage spent in IO wait. However whether a particular task is 
> sleeping waiting for IO or not

So strict per-task state is not a problem, and we could easily change
get_task_state() to distinguish between IO-wait or not, basically duplicate S/D state into an IO-wait variant of the same. Although even this has ABI implications :-(

> is IMO a useful diagnostic information and there are several places in 
> the kernel that take that into account (PSI, hangcheck timer, cpufreq, 
> ...).

So PSI is the one I hate most. We spend an aweful lot of time to not have to take the old rq->lock on wakeup, and PSI reintroduced it for accounting purposes -- I hate accounting overhead. :/

There's a number of high frequency scheduling workloads where it really adds up, which is the reason we got rid of it in the first place.

OTOH, PSI gives more sensible numbers, although it goes side-ways when you introduce affinity masks / cpusets.

The menu-cpufreq gov is known crazy and we're all hard working on replacing it.

And the tick-sched usage is, iirc, the nohz case of iowait.

> So I don't see that properly accounting that a task is waiting for IO 
> is just "expensive random number generator" as you mention below :). 
> But I'm open to being educated...

It's the userspace iowait, and in particular the per-cpu iowait numbers that I hate. Only on UP does any of that make sense.

But we can't remove them because ABI :-(