Message-ID: <alpine.LSU.2.11.2007302018350.2410@eggly.anvils>
Date:   Thu, 30 Jul 2020 21:06:55 -0700 (PDT)
From:   Hugh Dickins <hughd@...gle.com>
To:     Roman Gushchin <guro@...com>
cc:     Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>, linux-mm@...ck.org,
        kernel-team@...com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm: vmstat: fix /proc/sys/vm/stat_refresh generating
 false warnings

On Thu, 30 Jul 2020, Roman Gushchin wrote:
> On Wed, Jul 29, 2020 at 08:45:47PM -0700, Hugh Dickins wrote:
> > 
> > But a better idea is perhaps to redefine the behavior of
> > "echo >/proc/sys/vm/stat_refresh".  What if
> > "echo someparticularstring >/proc/sys/vm/stat_refresh" were to
> > disable or enable the warning (permanently? or just that time?):
> > disable would be more "back-compatible", but I think it's okay
> > if you prefer enable.  Or "someparticularstring" could actually
> > specify the warning threshold you want to use - you might echo
> > 125 or 16000, I might echo 0.  We can haggle over the default.
> 
> May I ask you, what kind of problems you have in your in mind,
> which can be revealed by these warnings? Or maybe there is some
> history attached?

Yes: 52b6f46bc163 mentions finding a bug of mine in NR_ISOLATED_FILE
accounting, but IIRC (though I might be making this up) there was
also a bug in the NR_ACTIVE or NR_INACTIVE FILE or ANON accounting.

When one of the stats used for balancing or limiting in vmscan.c
trends increasingly negative, it becomes increasingly difficult
for those heuristics (adding on to others, comparing with others)
to do what they're intended to do: they behave increasingly weirdly.

Now the same (or the opposite) is true if one of those stats trends
increasingly positive: but if it leaks positive, it's visible in
/proc/vmstat; whereas if it leaks negative, it's presented there as 0.

And most of the time (when unsynchronized) showing 0 is much better
than showing a transient negative.  But to help fix bugs, we do need
some way of seeing the negatives, and vm/stat_refresh provides an
opportunity to do so, when it synchronizes.

I'd be glad not to show the transients if I knew them: set a flag
on any that go negative, and only show if negative twice or more
in a row?  Perhaps, but I don't relish adding that, and think it
would be over-engineering.

It does sound to me like echoing the warning threshold into
/proc/sys/vm/stat_refresh is the best way to satisfy us both.

Though another alternative did occur to me overnight: we could
scrap the logged warning, and show "nr_whatever -53" as output
from /proc/sys/vm/stat_refresh: that too would be acceptable
to me, and you redirect to /dev/null.

(Why did I choose -53 in my example?  An in-joke: when I looked
through our machines for these warnings, on old kernels with my
old shmem hugepage implementation, there were a striking number
with "nr_shmem_freeholes -53"; but I'm a few years too late to
investigate what was going on there.)

> 
> If it's all about some particular counters, which are known to be
> strictly positive, maybe we should do the opposite, and check only
> those counters? Because in general it's not an indication of a problem.

Yet it's very curious how few stats ever generate such warnings:
you're convinced they're just transient noise, and you're probably right;
but I am a little suspicious of whether they are accounted correctly.

Hugh

Powered by blists - more mailing lists