Re: [PATCH v2 01/11] mm/vmstat: remove remote node draining

[Marcelo Tosatti]

On Thu, Mar 02, 2023 at 12:27:11PM -0500, Peter Xu wrote:
> On Thu, Mar 02, 2023 at 12:21:15PM -0500, Peter Xu wrote:
> > On Thu, Feb 09, 2023 at 12:01:51PM -0300, Marcelo Tosatti wrote:
> > > Draining of pages from the local pcp for a remote zone was necessary
> > > since:
> > > 
> > > "Note that remote node draining is a somewhat esoteric feature that is
> > > required on large NUMA systems because otherwise significant portions
> > > of system memory can become trapped in pcp queues. The number of pcp is
> > > determined by the number of processors and nodes in a system. A system
> > > with 4 processors and 2 nodes has 8 pcps which is okay. But a system
> > > with 1024 processors and 512 nodes has 512k pcps with a high potential
> > > for large amount of memory being caught in them."
> > 
> > How about mentioning more details on where does this come from?
> > 
> > afaict it's from commit 4037d45 since 2007.
> > 
> > So I digged that out mostly because I want to know why we did flush pcp at
> > all during vmstat update.  It already sounds weird to me but I could have
> > been missing important details.
> > 
> > The rational I had here is refresh_cpu_vm_stats(true) is mostly being
> > called by the shepherd afaict, while:
> > 
> >   (1) The frequency of that interval is defined as sysctl_stat_interval,
> >       which has nothing yet to do with pcp pages but only stat at least in
> >       the name of it, and,
> > 
> >   (2) vmstat_work is only queued if need_update() here:
> > 
> > 	for_each_online_cpu(cpu) {
> > 		struct delayed_work *dw = &per_cpu(vmstat_work, cpu);
> > 
> > 		if (!delayed_work_pending(dw) && need_update(cpu))
> > 			queue_delayed_work_on(cpu, mm_percpu_wq, dw, 0);
> > 
> > 		cond_resched();
> > 	}
> > 
> >       need_update() tells us "we should flush vmstats", nothing it tells
> >       about "we should flush pcp list"..
> > 
> > I looked into the 2007 commit, besides what Marcelo quoted, I do see
> > there's a major benefit of reusing cache lines, quotting from the commit:
> > 
> >         Move the node draining so that is is done when the vm statistics
> >         are updated.  At that point we are already touching all the
> >         cachelines with the pagesets of a processor.
> > 
> > However I didn't see why it's rational to flush pcp list when vmstat needs
> > flushing either.  I also don't know whether that "cacheline locality" hold
> > true or not, because I saw that the pcp page list is split from vmstats
> > since 2021:
> > 
> >     commit 28f836b6777b6f42dce068a40d83a891deaaca37
> >     Author: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> >     Date:   Mon Jun 28 19:41:38 2021 -0700
> > 
> >     mm/page_alloc: split per cpu page lists and zone stats
> > 
> > I'm not even sure my A-b or R-b worth anything at all here, just offer
> > something I got from git archaeology so maybe helpful to readers and
> > reasoning to this patch.  The correctness of archaeology needs help from
> > others (Christoph and Gel?)..  I would just say if there's anything useful
> > or correct may worth collect some into the commit log.
> > 
> > So from what I can tell this patch makes sense.
> 
> One thing I forgot to mention, which may be a slight abi change, is that I
> think the pcp page drain is also triggered by /proc/PID/refresh_vm_stats
> (even though again I don't see why flushing pcp is strictly needed).  It's
> just that I don't know whether there's potential user app that can leverage
> this.
> 
> The worst case is we can drain pcp list for refresh_vm_stats procfs
> explicitly, but I'm not sure whether it'll be worthwhile either, probably
> just to be safe.

This is a good point.

Documentation/admin-guide/sysctl/vm.rst:

stat_refresh
============

Any read or write (by root only) flushes all the per-cpu vm statistics
into their global totals, for more accurate reports when testing
e.g. cat /proc/sys/vm/stat_refresh /proc/meminfo

As a side-effect, it also checks for negative totals (elsewhere reported
as 0) and "fails" with EINVAL if any are found, with a warning in dmesg.
(At time of writing, a few stats are known sometimes to be found negative,
with no ill effects: errors and warnings on these stats are suppressed.)


Will add "drain_all_pages(NULL)" call to the start of stat_refresh
handler.

Thanks.