Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 6 Aug 2020 09:41:05 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Roman Gushchin <guro@fb.com>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>, linux-mm@kvack.org,
        kernel-team@fb.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm: vmstat: fix /proc/sys/vm/stat_refresh generating
 false warnings
In-Reply-To: <20200806035140.GA1162560@carbon.dhcp.thefacebook.com>
Message-ID: <alpine.LSU.2.11.2008060824380.9630@eggly.anvils>
References: <20200714173920.3319063-1-guro@fb.com> <alpine.LSU.2.11.2007291902340.6363@eggly.anvils> <20200730162348.GA679955@carbon.dhcp.thefacebook.com> <alpine.LSU.2.11.2007302018350.2410@eggly.anvils> <20200801011821.GA859734@carbon.dhcp.thefacebook.com>
 <alpine.LSU.2.11.2007311915130.9716@eggly.anvils> <20200804004012.GA1049259@carbon.dhcp.thefacebook.com> <alpine.LSU.2.11.2008051913580.8184@eggly.anvils> <20200806035140.GA1162560@carbon.dhcp.thefacebook.com>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 5 Aug 2020, Roman Gushchin wrote:
> On Wed, Aug 05, 2020 at 08:01:33PM -0700, Hugh Dickins wrote:
> > 
> > I shall certainly want to reintroduce those stats to checking for
> > negatives, even if it's in a patch that never earns your approval,
> > and just ends up kept internal for debugging.  But equally certainly,
> > I must not suddenly reintroduce that checking without gaining some
> > experience of it (and perhaps getting as irritated as you by more
> > transient negatives).
> > 
> > I said earlier that I'd prefer you to rip out all that checking for
> > negatives, rather than retaining it with the uselessly over-generous
> > 125 * nr_cpus leeway.  Please, Roman, would you send Andrew a patch
> > doing that, to replace the patch in this thread?  Or if you prefer,
> > I can do so.
> 
> Sure, I can do it. But can you, please, explain why you want them to be
> eliminated? Is this because they will never fire, you think?

Yes, I've never seen a machine on which vm/stat_refresh reported -16000
or less, or anything approaching that (true, this laptop does not have
128 cpus, but...).  Maybe they get reinstalled or otherwise rebooted
before reaching numbers of that magnitude.  Maybe having the warnings
shown at much lower magnitudes has helped to root-cause and eliminate
some of them.

Waiting until the heat-death-of-the-universe theoretical worst case is
so unhelpful as to defeat the purpose of the warning.  I think you do
understand, but perhaps not all readers of this thread understand,
that vm/stat_refresh merges all per-cpu counts into the global atomic
immediately before deciding negative.  The only problem is that
"immediately" is not instantaneous across cpus, so the possibility
of work started on one cpu but completed on another during the course
of the refresh, causing false negatives, is real though not great.

> 
> In my humble opinion they might be quite useful: any systematic under- or
> overflow will eventually trigger this warning, and we'll know for sure that
> something is wrong. So I'd add a similar check for node counters without
> any hesitation.

It's true that while developing, we can all make mistakes so big that
"eventually" will show up quickly, and even that warning could help.
But since you'll only show them when they reach -16000 (sorry, I keep
going back to the 128 cpu case, just to make it more vivid), it won't
be of any use to catch races later in development, or in production.

Whereas my own patch would just fix the missing items, and continue
to annoy you with occasional ignorable warnings - so I cannot submit
that, and wouldn't want to submit it right now, without having tried
it out for a while, to check what kind of noise it will generate.

So I thought it best, either to leave mm/vmstat.c alone for the moment,
or else just delete the disputed and incomplete code; coming back to
it later when we have something we can agree upon.

But I think you're preferring to resubmit your 125*nr_cpus patch to akpm,
with the missing NR_VM_NODE_STAT_ITEMS added in (as either one or two
patches), with foreshortened testing but the reassurance that since
it's so hard to reach the point of showing the warnings, new negatives
will either be quiet, or easily fixed before v5.10 released: okay,
I can more easily Hack that to my preference than Ack it or Nak it.

Hugh