Date:   Thu, 2 Mar 2023 15:56:58 -0300
From:   Marcelo Tosatti <mtosatti@redhat.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     Christoph Lameter <cl@linux.com>,
        Aaron Tomlin <atomlin@atomlin.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v2 01/11] mm/vmstat: remove remote node draining
Message-ID: <ZADxelKv7VrkFoeC@tpad>
References: <20230209150150.380060673@redhat.com>
 <20230209153204.656996515@redhat.com>
 <ZADbC9RnmVtpC6kE@x1n>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZADbC9RnmVtpC6kE@x1n>
Precedence: bulk

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@techsingularity.net>
>     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, 

"A-b or R-b" ? 

I think your points are valid.

Also, the fact that sysctl_stat_interval is large (a second or more),
means that any cache locality concern is would be limited to that
time span.

> 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.

Agreed, i forgot to include the commit id in the changelog.

> So from what I can tell this patch makes sense.

Thanks!