Re: [PATCH v3] mm, sysctl: make NUMA stats configurable

[Michal Hocko <mhocko@kernel.org>]

On Mon 09-10-17 14:34:11, kemi wrote:
> 
> 
> On 2017a1'10ae??03ae?JPY 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>