Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751413AbdIOLuA (ORCPT ); Fri, 15 Sep 2017 07:50:00 -0400 Received: from mx2.suse.de ([195.135.220.15]:48061 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751184AbdIOLt7 (ORCPT ); Fri, 15 Sep 2017 07:49:59 -0400 Date: Fri, 15 Sep 2017 13:49:52 +0200 From: Michal Hocko To: Kemi Wang Cc: "Luis R . Rodriguez" , Kees Cook , Andrew Morton , Jonathan Corbet , Mel Gorman , Johannes Weiner , Christopher Lameter , Sebastian Andrzej Siewior , Vlastimil Babka , Hillf Danton , Dave , Tim Chen , Andi Kleen , Jesper Dangaard Brouer , Ying Huang , Aaron Lu , Proc sysctl , Linux MM , Linux Kernel Subject: Re: [PATCH 1/3] mm, sysctl: make VM stats configurable Message-ID: <20170915114952.czb7nbsioqguxxk3@dhcp22.suse.cz> References: <1505467406-9945-1-git-send-email-kemi.wang@intel.com> <1505467406-9945-2-git-send-email-kemi.wang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1505467406-9945-2-git-send-email-kemi.wang@intel.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5644 Lines: 170 On Fri 15-09-17 17:23:24, Kemi Wang wrote: > This patch adds a tunable interface that allows VM stats configurable, as > suggested by Dave Hansen and Ying Huang. > > When performance becomes a bottleneck and you can tolerate some possible > tool breakage and some decreased counter precision (e.g. numa counter), you > can do: > echo [C|c]oarse > /proc/sys/vm/vmstat_mode > > When performance is not a bottleneck and you want all tooling to work, you > can do: > echo [S|s]trict > /proc/sys/vm/vmstat_mode > > We recommend automatic detection of virtual memory statistics by system, > this is also system default configuration, you can do: > echo [A|a]uto > /proc/sys/vm/vmstat_mode > > The next patch handles numa statistics distinctively based-on different VM > stats mode. I would just merge this with the second patch so that it is clear how those modes are implemented. I am also wondering why cannot we have a much simpler interface and implementation to enable/disable numa stats (btw. sysctl_vm_numa_stats would be more descriptive IMHO). Why do we need an auto-mode? Is it safe to enforce by default. Is it possible that userspace can get confused to see 0 NUMA stats in the first read while other allocation stats are non-zero? > Reported-by: Jesper Dangaard Brouer > Suggested-by: Dave Hansen > Suggested-by: Ying Huang > Signed-off-by: Kemi Wang > --- > include/linux/vmstat.h | 14 ++++++++++ > kernel/sysctl.c | 7 +++++ > mm/vmstat.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+) > > diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h > index ade7cb5..c3634c7 100644 > --- a/include/linux/vmstat.h > +++ b/include/linux/vmstat.h > @@ -9,6 +9,20 @@ > > extern int sysctl_stat_interval; > > +/* > + * vmstat_mode: > + * 0 = auto mode of vmstat, automatic detection of VM statistics. > + * 1 = strict mode of vmstat, keep all VM statistics. > + * 2 = coarse mode of vmstat, ignore unimportant VM statistics. > + */ > +#define VMSTAT_AUTO_MODE 0 > +#define VMSTAT_STRICT_MODE 1 > +#define VMSTAT_COARSE_MODE 2 > +#define VMSTAT_MODE_LEN 16 > +extern char sysctl_vmstat_mode[]; > +extern int sysctl_vmstat_mode_handler(struct ctl_table *table, int write, > + void __user *buffer, size_t *length, loff_t *ppos); > + > #ifdef CONFIG_VM_EVENT_COUNTERS > /* > * Light weight per cpu counter implementation. > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 6648fbb..f5b813b 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -1234,6 +1234,13 @@ static struct ctl_table kern_table[] = { > > static struct ctl_table vm_table[] = { > { > + .procname = "vmstat_mode", > + .data = &sysctl_vmstat_mode, > + .maxlen = VMSTAT_MODE_LEN, > + .mode = 0644, > + .proc_handler = sysctl_vmstat_mode_handler, > + }, > + { > .procname = "overcommit_memory", > .data = &sysctl_overcommit_memory, > .maxlen = sizeof(sysctl_overcommit_memory), > diff --git a/mm/vmstat.c b/mm/vmstat.c > index 4bb13e7..e675ad2 100644 > --- a/mm/vmstat.c > +++ b/mm/vmstat.c > @@ -32,6 +32,76 @@ > > #define NUMA_STATS_THRESHOLD (U16_MAX - 2) > > +int vmstat_mode = VMSTAT_AUTO_MODE; > +char sysctl_vmstat_mode[VMSTAT_MODE_LEN] = "auto"; > +static const char *vmstat_mode_name[3] = {"auto", "strict", "coarse"}; > +static DEFINE_MUTEX(vmstat_mode_lock); > + > + > +static int __parse_vmstat_mode(char *s) > +{ > + const char *str = s; > + > + if (strcmp(str, "auto") == 0 || strcmp(str, "Auto") == 0) > + vmstat_mode = VMSTAT_AUTO_MODE; > + else if (strcmp(str, "strict") == 0 || strcmp(str, "Strict") == 0) > + vmstat_mode = VMSTAT_STRICT_MODE; > + else if (strcmp(str, "coarse") == 0 || strcmp(str, "Coarse") == 0) > + vmstat_mode = VMSTAT_COARSE_MODE; > + else { > + pr_warn("Ignoring invalid vmstat_mode value: %s\n", s); > + return -EINVAL; > + } > + return 0; > +} > + > +int sysctl_vmstat_mode_handler(struct ctl_table *table, int write, > + void __user *buffer, size_t *length, loff_t *ppos) > +{ > + char old_string[VMSTAT_MODE_LEN]; > + int ret, oldval; > + > + mutex_lock(&vmstat_mode_lock); > + if (write) > + strncpy(old_string, (char *)table->data, VMSTAT_MODE_LEN); > + ret = proc_dostring(table, write, buffer, length, ppos); > + if (ret || !write) { > + mutex_unlock(&vmstat_mode_lock); > + return ret; > + } > + > + oldval = vmstat_mode; > + if (__parse_vmstat_mode((char *)table->data)) { > + /* > + * invalid sysctl_vmstat_mode value, restore saved string > + */ > + strncpy((char *)table->data, old_string, VMSTAT_MODE_LEN); > + vmstat_mode = oldval; > + } else { > + /* > + * check whether vmstat mode changes or not > + */ > + if (vmstat_mode == oldval) { > + /* no change */ > + mutex_unlock(&vmstat_mode_lock); > + return 0; > + } else if (vmstat_mode == VMSTAT_AUTO_MODE) > + pr_info("vmstat mode changes from %s to auto mode\n", > + vmstat_mode_name[oldval]); > + else if (vmstat_mode == VMSTAT_STRICT_MODE) > + pr_info("vmstat mode changes from %s to strict mode\n", > + vmstat_mode_name[oldval]); > + else if (vmstat_mode == VMSTAT_COARSE_MODE) > + pr_info("vmstat mode changes from %s to coarse mode\n", > + vmstat_mode_name[oldval]); > + else > + pr_warn("invalid vmstat_mode:%d\n", vmstat_mode); > + } > + > + mutex_unlock(&vmstat_mode_lock); > + return 0; > +} > + > #ifdef CONFIG_VM_EVENT_COUNTERS > DEFINE_PER_CPU(struct vm_event_state, vm_event_states) = {{0}}; > EXPORT_PER_CPU_SYMBOL(vm_event_states); > -- > 2.7.4 > -- Michal Hocko SUSE Labs