kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@suse.de>
To: Balbir Singh <bsingharora@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	tglx@linutronix.de, mingo@kernel.org, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	rostedt@goodmis.org, bsegall@google.com, bristot@redhat.com,
	pbonzini@redhat.com, maz@kernel.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	riel@surriel.com, hannes@cmpxchg.org,
	Paul Wise <pabs3@bonedaddy.net>
Subject: Re: [PATCH 0/6] sched,delayacct: Some cleanups
Date: Wed, 12 May 2021 12:34:19 +0100	[thread overview]
Message-ID: <20210512113419.GF3672@suse.de> (raw)
In-Reply-To: <20210507123810.GB4236@balbir-desktop>

On Fri, May 07, 2021 at 10:38:10PM +1000, Balbir Singh wrote:
> On Thu, May 06, 2021 at 11:13:52AM +0200, Peter Zijlstra wrote:
> > On Thu, May 06, 2021 at 08:29:40AM +1000, Balbir Singh wrote:
> > > On Wed, May 05, 2021 at 12:59:40PM +0200, Peter Zijlstra wrote:
> > > > Hi,
> > > > 
> > > > Due to:
> > > > 
> > > >   https://lkml.kernel.org/r/0000000000001d43ac05c0f5c6a0@google.com
> > > > 
> > > > and general principle, delayacct really shouldn't be using ktime (pvclock also
> > > > really shouldn't be doing what it does, but that's another story). This lead me
> > > > to looking at the SCHED_INFO, SCHEDSTATS, DELAYACCT (and PSI) accounting hell.
> > > > 
> > > > The rest of the patches are an attempt at simplifying all that a little. All
> > > > that crud is enabled by default for distros which is leading to a death by a
> > > > thousand cuts.
> > > > 
> > > > The last patch is an attempt at default disabling DELAYACCT, because I don't
> > > > think anybody actually uses that much, but what do I know, there were no ill
> > > > effects on my testbox. Perhaps we should mirror
> > > > /proc/sys/kernel/sched_schedstats and provide a delayacct sysctl for runtime
> > > > frobbing.
> > > >
> > > 
> > > There are tools like iotop that use delayacct to display information. 
> > 
> > Right, but how many actual people use that? Does that justify saddling
> > the whole sodding world with the overhead?
> >
> 
> Not sure I have that data.
>  

It's used occasionally as a debugging tool when looking at IO problems
in general. Like sched_schedstats, it seems unnecesary to incur overhead
unless someone is debugging.

Minimally disabling had a small positive impact -- tbench 0-8% gain
depending on thread count and machine, positive impact in general to
specjbb2005, neutral on hackbench and most of the other workloads checked.

> > > When the
> > > code was checked in, we did run SPEC* back in the day 2006 to find overheads,
> > > nothing significant showed. Do we have any date on the overhead your seeing?
> > 
> > I've not looked, but having it disabled saves that per-task allocation
> > and that spinlock in delayacct_end() for iowait wakeups and a bunch of
> > cache misses ofcourse.
> > 
> > I doubt SPEC is a benchmark that tickles those paths much if at all.
> > 
> > The thing is; we can't just keep growing more and more stats, that'll
> > kill us quite dead.
> 
> 
> I don't disagree, we've had these around for a while and I know of users
> that use these stats to find potential starvation. I am OK with default
> disabled. I suspect distros will have the final say.
> 

I think default disabled should be fine. At worst when dealing with a bug
there would be a need to either reboot or enable at runtime with patch
7 included and add that instruction to the bug report when requesting
iotop data. At worst, a distro could revert the patch if iotop generated
too many bug reports or patch iotop in the distro package.

Alternatively, I've added Paul Wise to the cc who is the latest
committer to iotop.  Maybe he knows who could add/commit a check for
sysctl.sched_delayacct and if it exists then check if it's 1 and display
an error suggesting corrective action (add delayacct to the kernel command
line or sysctl sched.sched_delayacct=1). iotop appears to be in maintenance
mode but gets occasional commits even if it has not had a new version
since 2013 so maybe it could get a 0.7 tag if such a check was added.

-- 
Mel Gorman
SUSE Labs

  reply	other threads:[~2021-05-12 11:34 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05 10:59 Peter Zijlstra
2021-05-05 10:59 ` [PATCH 1/6] delayacct: Use sched_clock() Peter Zijlstra
2021-05-05 14:40   ` Rik van Riel
2021-05-06 13:59   ` Johannes Weiner
2021-05-06 14:17     ` Peter Zijlstra
2021-05-06 15:17       ` Johannes Weiner
2021-05-07 12:40   ` Balbir Singh
2021-05-12 10:43   ` Mel Gorman
2021-05-05 10:59 ` [PATCH 2/6] sched: Rename sched_info_{queued,dequeued} Peter Zijlstra
2021-05-05 14:39   ` Rik van Riel
2021-05-06 13:59   ` Johannes Weiner
2021-05-10  8:45   ` Balbir Singh
2021-05-12 10:49   ` Mel Gorman
2021-05-05 10:59 ` [PATCH 3/6] sched: Simplify sched_info_on() Peter Zijlstra
2021-05-06 14:03   ` Johannes Weiner
2021-05-12 11:10   ` Mel Gorman
2021-05-12 11:32     ` Peter Zijlstra
2021-05-12 12:51       ` Mel Gorman
2021-05-05 10:59 ` [PATCH 4/6] kvm: Select SCHED_INFO instead of TASK_DELAY_ACCT Peter Zijlstra
2021-05-05 11:37   ` Paolo Bonzini
2021-05-06 14:38   ` Marc Zyngier
2021-05-07 12:42   ` Balbir Singh
2021-05-12 11:11   ` Mel Gorman
2021-05-05 10:59 ` [PATCH 5/6] delayacct: Add static_branch in scheduler hooks Peter Zijlstra
2021-05-06 14:05   ` Johannes Weiner
2021-05-10  8:42   ` Balbir Singh
2021-05-12 11:13   ` Mel Gorman
2021-05-05 10:59 ` [PATCH 6/6] [RFC] delayacct: Default disabled Peter Zijlstra
2021-05-12 11:35   ` Mel Gorman
2021-05-05 22:29 ` [PATCH 0/6] sched,delayacct: Some cleanups Balbir Singh
2021-05-06  9:13   ` Peter Zijlstra
2021-05-07 12:38     ` Balbir Singh
2021-05-12 11:34       ` Mel Gorman [this message]
2021-05-12 11:38         ` Peter Zijlstra
2021-05-12 12:23         ` Paul Wise
2021-05-12 13:00           ` Mel Gorman
2021-05-13  1:29             ` Paul Wise
2021-06-25  0:50         ` Paul Wise
2021-05-07  9:05 ` Thomas Gleixner
2021-05-10  7:08 ` Ingo Molnar
2021-05-10 12:05 ` [PATCH 7/6] delayacct: Add sysctl to enable at runtime Peter Zijlstra
2021-05-10 12:06   ` Peter Zijlstra
2021-05-12 11:40   ` Mel Gorman

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 \
    --in-reply-to=20210512113419.GF3672@suse.de \
    --to=mgorman@suse.de \
    --cc=bristot@redhat.com \
    --cc=bsegall@google.com \
    --cc=bsingharora@gmail.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=hannes@cmpxchg.org \
    --cc=juri.lelli@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=mingo@kernel.org \
    --cc=pabs3@bonedaddy.net \
    --cc=pbonzini@redhat.com \
    --cc=peterz@infradead.org \
    --cc=riel@surriel.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=vincent.guittot@linaro.org \
    --subject='Re: [PATCH 0/6] sched,delayacct: Some cleanups' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).