Date:   Mon, 9 Oct 2017 09:55:49 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     kemi <kemi.wang@...el.com>
Cc:     "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Johannes Weiner <hannes@...xchg.org>,
        Christopher Lameter <cl@...ux.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vlastimil Babka <vbabka@...e.cz>,
        Dave <dave.hansen@...ux.intel.com>,
        Tim Chen <tim.c.chen@...el.com>,
        Andi Kleen <andi.kleen@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Ying Huang <ying.huang@...el.com>,
        Aaron Lu <aaron.lu@...el.com>,
        Proc sysctl <linux-fsdevel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] mm, sysctl: make NUMA stats configurable

On Mon 09-10-17 14:34:11, kemi wrote:
> 
> 
> On 2017年10月03日 17:23, Michal Hocko wrote:
> > On Thu 28-09-17 14:11:41, Kemi Wang wrote:
> >> This is the second step which introduces a tunable interface that allow
> >> numa stats configurable for optimizing zone_statistics(), as suggested by
> >> Dave Hansen and Ying Huang.
> >>
> >> =========================================================================
> >> When page allocation performance becomes a bottleneck and you can tolerate
> >> some possible tool breakage and decreased numa counter precision, you can
> >> do:
> >> 	echo [C|c]oarse > /proc/sys/vm/numa_stats_mode
> >> In this case, numa counter update is ignored. We can see about
> >> *4.8%*(185->176) drop of cpu cycles per single page allocation and reclaim
> >> on Jesper's page_bench01 (single thread) and *8.1%*(343->315) drop of cpu
> >> cycles per single page allocation and reclaim on Jesper's page_bench03 (88
> >> threads) running on a 2-Socket Broadwell-based server (88 threads, 126G
> >> memory).
> >>
> >> Benchmark link provided by Jesper D Brouer(increase loop times to
> >> 10000000):
> >> https://github.com/netoptimizer/prototype-kernel/tree/master/kernel/mm/
> >> bench
> >>
> >> =========================================================================
> >> When page allocation performance is not a bottleneck and you want all
> >> tooling to work, you can do:
> >> 	echo [S|s]trict > /proc/sys/vm/numa_stats_mode
> >>
> >> =========================================================================
> >> We recommend automatic detection of numa statistics by system, this is also
> >> system default configuration, you can do:
> >> 	echo [A|a]uto > /proc/sys/vm/numa_stats_mode
> >> In this case, numa counter update is skipped unless it has been read by
> >> users at least once, e.g. cat /proc/zoneinfo.
> > 
> > I am still not convinced the auto mode is worth all the additional code
> > and a safe default to use. The whole thing could have been 0/1 with a
> > simpler parsing and less code to catch readers.
> > 
> 
> I understood your concern. 
> Well, we may get rid of auto mode if there is some obvious disadvantage
> here. Now, I tend to keep it because most people may not touch this interface,
> and auto mode is helpful in such case.

But you cannot guarantee it won't break any existing users, can you?
Besides I do not remember anybody complaining about the performance
impact of these counters other than very specialized workloads which are
going to disable the accounting altogether. So I simply fail to see a
reason to add more code with a questionable semantic (see below on
partial reads).

> > E.g. why do we have to do static_branch_enable on any read or even
> > vmstat_stop? Wouldn't open be sufficient?
> > 
> 
> NUMA stats is used in four files:
> /proc/zoneinfo
> /proc/vmstat
> /sys/devices/system/node/node*/numastat
> /sys/devices/system/node/node*/vmstat
> In auto mode, each *read* will trigger the update of NUMA counter. 
> So, we should make sure the target branch is jumped to the branch 
> for NUMA counter update once the file is read from user space.
> the intension of static_branch_enable in vmstat_stop(in the call site 
> of file->file_ops.read) is for reading /proc/vmstat in case.  
> 
> I guess the *open* means file->file_op.open here, right?
> Do you suggest to move static_branch_enable to file->file_op.open? Thanks.

I haven't checked closely but what happens (or should happen) when you
do a partial read? Should you get an inconsistent results? Or is this
impossible?
-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists