linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH]mmap: improve scalability for updating vm_committed_as
@ 2011-03-30  1:17 Shaohua Li
  2011-03-30 22:51 ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-03-30  1:17 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

In a workload with a lot of mmap/mumap, updating vm_committed_as is
a scalability issue, because the percpu_counter_batch is too small, and
the update needs hold percpu_counter lock.
On the other hand, vm_committed_as is only used in OVERCOMMIT_NEVER case,
which isn't the default setting.
We can make the batch bigger in other cases and then switch to small batch
in OVERCOMMIT_NEVER case, so that we will have no scalability issue with
default setting. We flush all CPUs' percpu counter when switching
sysctl_overcommit_memory, so there is no race the counter is incorrect.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

---
 fs/proc/meminfo.c    |    2 +-
 include/linux/mman.h |   10 +++++++++-
 kernel/sysctl.c      |    5 ++---
 mm/mmap.c            |   27 +++++++++++++++++++++++++++
 mm/nommu.c           |   27 +++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 5 deletions(-)

Index: linux/include/linux/mman.h
===================================================================
--- linux.orig/include/linux/mman.h	2011-03-29 16:28:57.000000000 +0800
+++ linux/include/linux/mman.h	2011-03-30 09:01:38.000000000 +0800
@@ -20,9 +20,17 @@ extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern struct percpu_counter vm_committed_as;
 
+extern int overcommit_memory_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp, loff_t *ppos);
 static inline void vm_acct_memory(long pages)
 {
-	percpu_counter_add(&vm_committed_as, pages);
+	/* avoid overflow and the value is big enough */
+	int batch = INT_MAX/2;
+
+	if (sysctl_overcommit_memory == OVERCOMMIT_NEVER)
+		batch = percpu_counter_batch;
+
+	__percpu_counter_add(&vm_committed_as, pages, batch);
 }
 
 static inline void vm_unacct_memory(long pages)
Index: linux/fs/proc/meminfo.c
===================================================================
--- linux.orig/fs/proc/meminfo.c	2011-03-29 16:28:57.000000000 +0800
+++ linux/fs/proc/meminfo.c	2011-03-30 09:01:38.000000000 +0800
@@ -35,7 +35,7 @@ static int meminfo_proc_show(struct seq_
 #define K(x) ((x) << (PAGE_SHIFT - 10))
 	si_meminfo(&i);
 	si_swapinfo(&i);
-	committed = percpu_counter_read_positive(&vm_committed_as);
+	committed = percpu_counter_sum_positive(&vm_committed_as);
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;
 
Index: linux/kernel/sysctl.c
===================================================================
--- linux.orig/kernel/sysctl.c	2011-03-29 16:28:57.000000000 +0800
+++ linux/kernel/sysctl.c	2011-03-30 09:01:38.000000000 +0800
@@ -56,6 +56,7 @@
 #include <linux/kprobes.h>
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
+#include <linux/mman.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -86,8 +87,6 @@
 #if defined(CONFIG_SYSCTL)
 
 /* External variables not in a header file. */
-extern int sysctl_overcommit_memory;
-extern int sysctl_overcommit_ratio;
 extern int max_threads;
 extern int core_uses_pid;
 extern int suid_dumpable;
@@ -977,7 +976,7 @@ static struct ctl_table vm_table[] = {
 		.data		= &sysctl_overcommit_memory,
 		.maxlen		= sizeof(sysctl_overcommit_memory),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= overcommit_memory_handler,
 		.extra1		= &zero,
 		.extra2		= &two,
 	},
Index: linux/mm/mmap.c
===================================================================
--- linux.orig/mm/mmap.c	2011-03-30 08:59:23.000000000 +0800
+++ linux/mm/mmap.c	2011-03-30 09:01:38.000000000 +0800
@@ -93,6 +93,33 @@ int sysctl_max_map_count __read_mostly =
  */
 struct percpu_counter vm_committed_as ____cacheline_internodealigned_in_smp;
 
+static void overcommit_drain_counter(struct work_struct *dummy)
+{
+	/*
+	 * Flush percpu counter to global counter when batch is changed, see
+	 * vm_acct_memory for detail
+	 */
+	vm_acct_memory(0);
+}
+
+int overcommit_memory_handler(struct ctl_table *table, int write,
+                void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int error;
+
+	error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+	if (error)
+		return error;
+
+	if (write) {
+		/* Make sure each CPU sees the new sysctl_overcommit_memory */
+		smp_wmb();
+		schedule_on_each_cpu(overcommit_drain_counter);
+	}
+
+	return 0;
+}
+
 /*
  * Check that a process has enough memory to allocate a new virtual
  * mapping. 0 means there is enough memory for the allocation to
Index: linux/mm/nommu.c
===================================================================
--- linux.orig/mm/nommu.c	2011-03-29 16:28:57.000000000 +0800
+++ linux/mm/nommu.c	2011-03-30 09:01:38.000000000 +0800
@@ -1859,6 +1859,33 @@ void unmap_mapping_range(struct address_
 }
 EXPORT_SYMBOL(unmap_mapping_range);
 
+static void overcommit_drain_counter(struct work_struct *dummy)
+{
+	/*
+	 * Flush percpu counter to global counter when batch is changed, see
+	 * vm_acct_memory for detail
+	 */
+	vm_acct_memory(0);
+}
+
+int overcommit_memory_handler(struct ctl_table *table, int write,
+                void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int error;
+
+	error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+	if (error)
+		return error;
+
+	if (write) {
+		/* Make sure each CPU sees the new sysctl_overcommit_memory */
+		smp_wmb();
+		schedule_on_each_cpu(overcommit_drain_counter);
+	}
+
+	return 0;
+}
+
 /*
  * Check that a process has enough memory to allocate a new virtual
  * mapping. 0 means there is enough memory for the allocation to


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]mmap: improve scalability for updating vm_committed_as
  2011-03-30  1:17 [PATCH]mmap: improve scalability for updating vm_committed_as Shaohua Li
@ 2011-03-30 22:51 ` Andrew Morton
  2011-03-31  0:56   ` Shaohua Li
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2011-03-30 22:51 UTC (permalink / raw)
  To: Shaohua Li; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

On Wed, 30 Mar 2011 09:17:27 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

> In a workload with a lot of mmap/mumap, updating vm_committed_as is
> a scalability issue, because the percpu_counter_batch is too small, and
> the update needs hold percpu_counter lock.
> On the other hand, vm_committed_as is only used in OVERCOMMIT_NEVER case,
> which isn't the default setting.
> We can make the batch bigger in other cases and then switch to small batch
> in OVERCOMMIT_NEVER case, so that we will have no scalability issue with
> default setting. We flush all CPUs' percpu counter when switching
> sysctl_overcommit_memory, so there is no race the counter is incorrect.

The patch is purportedly a performance improvement, but the changelog
didn't tell us how much it improves performance?

> ...
>
> --- linux.orig/include/linux/mman.h	2011-03-29 16:28:57.000000000 +0800
> +++ linux/include/linux/mman.h	2011-03-30 09:01:38.000000000 +0800
> @@ -20,9 +20,17 @@ extern int sysctl_overcommit_memory;
>  extern int sysctl_overcommit_ratio;
>  extern struct percpu_counter vm_committed_as;
>  
> +extern int overcommit_memory_handler(struct ctl_table *table, int write,
> +		void __user *buffer, size_t *lenp, loff_t *ppos);
>  static inline void vm_acct_memory(long pages)
>  {
> -	percpu_counter_add(&vm_committed_as, pages);
> +	/* avoid overflow and the value is big enough */
> +	int batch = INT_MAX/2;
> +
> +	if (sysctl_overcommit_memory == OVERCOMMIT_NEVER)
> +		batch = percpu_counter_batch;
> +
> +	__percpu_counter_add(&vm_committed_as, pages, batch);
>  }

It would be better to create a global __read_mostly variable for this
and alter its value within the sysctl, rather than recalculating it
each time.

This again points at the need to make the batch count a field within
the percpu_counter.

>  static inline void vm_unacct_memory(long pages)
> Index: linux/fs/proc/meminfo.c
> ===================================================================
> --- linux.orig/fs/proc/meminfo.c	2011-03-29 16:28:57.000000000 +0800
> +++ linux/fs/proc/meminfo.c	2011-03-30 09:01:38.000000000 +0800
> @@ -35,7 +35,7 @@ static int meminfo_proc_show(struct seq_
>  #define K(x) ((x) << (PAGE_SHIFT - 10))
>  	si_meminfo(&i);
>  	si_swapinfo(&i);
> -	committed = percpu_counter_read_positive(&vm_committed_as);
> +	committed = percpu_counter_sum_positive(&vm_committed_as);
>  	allowed = ((totalram_pages - hugetlb_total_pages())
>  		* sysctl_overcommit_ratio / 100) + total_swap_pages;

This is a big change, and it wasn't even changelogged.  It's
potentially a tremendous increase in the expense of a read from
/proc/meminfo, which is a file that lots of tools will be polling. 
Many of those tools we don't even know about or have access to.

The change is unneeded if sysctl_overcommit_memory==OVERCOMMIT_NEVER,
but that's hardly a fix.

Quite worrisome.

Perhaps a better approach would be to carefully tune the batch size
according to the size of the machine.  Going all the way to INT_MAX/2
is surely overkill.


> Index: linux/kernel/sysctl.c
> ===================================================================
> --- linux.orig/kernel/sysctl.c	2011-03-29 16:28:57.000000000 +0800
> +++ linux/kernel/sysctl.c	2011-03-30 09:01:38.000000000 +0800
> @@ -56,6 +56,7 @@
>  #include <linux/kprobes.h>
>  #include <linux/pipe_fs_i.h>
>  #include <linux/oom.h>
> +#include <linux/mman.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/processor.h>
> @@ -86,8 +87,6 @@
>  #if defined(CONFIG_SYSCTL)
>  
>  /* External variables not in a header file. */
> -extern int sysctl_overcommit_memory;
> -extern int sysctl_overcommit_ratio;
>  extern int max_threads;
>  extern int core_uses_pid;
>  extern int suid_dumpable;
> @@ -977,7 +976,7 @@ static struct ctl_table vm_table[] = {
>  		.data		= &sysctl_overcommit_memory,
>  		.maxlen		= sizeof(sysctl_overcommit_memory),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> +		.proc_handler	= overcommit_memory_handler,
>  		.extra1		= &zero,
>  		.extra2		= &two,
>  	},
> Index: linux/mm/mmap.c
> ===================================================================
> --- linux.orig/mm/mmap.c	2011-03-30 08:59:23.000000000 +0800
> +++ linux/mm/mmap.c	2011-03-30 09:01:38.000000000 +0800
> @@ -93,6 +93,33 @@ int sysctl_max_map_count __read_mostly =
>   */
>  struct percpu_counter vm_committed_as ____cacheline_internodealigned_in_smp;
>  
> +static void overcommit_drain_counter(struct work_struct *dummy)
> +{
> +	/*
> +	 * Flush percpu counter to global counter when batch is changed, see
> +	 * vm_acct_memory for detail
> +	 */
> +	vm_acct_memory(0);
> +}
> +
> +int overcommit_memory_handler(struct ctl_table *table, int write,
> +                void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int error;
> +
> +	error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +	if (error)
> +		return error;
> +
> +	if (write) {
> +		/* Make sure each CPU sees the new sysctl_overcommit_memory */
> +		smp_wmb();
> +		schedule_on_each_cpu(overcommit_drain_counter);
> +	}
> +
> +	return 0;
> +}

Calling vm_acct_memory(0) is a bit of a hack.

Rather than open-coding this twice, it would be better to introduce a
new percpu_counter core primitive to collapse the counters.

>
> ...
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]mmap: improve scalability for updating vm_committed_as
  2011-03-30 22:51 ` Andrew Morton
@ 2011-03-31  0:56   ` Shaohua Li
  2011-03-31  2:34     ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-03-31  0:56 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

On Thu, 2011-03-31 at 06:51 +0800, Andrew Morton wrote:
> On Wed, 30 Mar 2011 09:17:27 +0800
> Shaohua Li <shaohua.li@intel.com> wrote:
> 
> > In a workload with a lot of mmap/mumap, updating vm_committed_as is
> > a scalability issue, because the percpu_counter_batch is too small, and
> > the update needs hold percpu_counter lock.
> > On the other hand, vm_committed_as is only used in OVERCOMMIT_NEVER case,
> > which isn't the default setting.
> > We can make the batch bigger in other cases and then switch to small batch
> > in OVERCOMMIT_NEVER case, so that we will have no scalability issue with
> > default setting. We flush all CPUs' percpu counter when switching
> > sysctl_overcommit_memory, so there is no race the counter is incorrect.
> 
> The patch is purportedly a performance improvement, but the changelog
> didn't tell us how much it improves performance?
I thought improving the scalability is enough, but anyway, I will add it later.

> > --- linux.orig/include/linux/mman.h	2011-03-29 16:28:57.000000000 +0800
> > +++ linux/include/linux/mman.h	2011-03-30 09:01:38.000000000 +0800
> > @@ -20,9 +20,17 @@ extern int sysctl_overcommit_memory;
> >  extern int sysctl_overcommit_ratio;
> >  extern struct percpu_counter vm_committed_as;
> >  
> > +extern int overcommit_memory_handler(struct ctl_table *table, int write,
> > +		void __user *buffer, size_t *lenp, loff_t *ppos);
> >  static inline void vm_acct_memory(long pages)
> >  {
> > -	percpu_counter_add(&vm_committed_as, pages);
> > +	/* avoid overflow and the value is big enough */
> > +	int batch = INT_MAX/2;
> > +
> > +	if (sysctl_overcommit_memory == OVERCOMMIT_NEVER)
> > +		batch = percpu_counter_batch;
> > +
> > +	__percpu_counter_add(&vm_committed_as, pages, batch);
> >  }
> 
> It would be better to create a global __read_mostly variable for this
> and alter its value within the sysctl, rather than recalculating it
> each time.
ok

> This again points at the need to make the batch count a field within
> the percpu_counter.

> >  static inline void vm_unacct_memory(long pages)
> > Index: linux/fs/proc/meminfo.c
> > ===================================================================
> > --- linux.orig/fs/proc/meminfo.c	2011-03-29 16:28:57.000000000 +0800
> > +++ linux/fs/proc/meminfo.c	2011-03-30 09:01:38.000000000 +0800
> > @@ -35,7 +35,7 @@ static int meminfo_proc_show(struct seq_
> >  #define K(x) ((x) << (PAGE_SHIFT - 10))
> >  	si_meminfo(&i);
> >  	si_swapinfo(&i);
> > -	committed = percpu_counter_read_positive(&vm_committed_as);
> > +	committed = percpu_counter_sum_positive(&vm_committed_as);
> >  	allowed = ((totalram_pages - hugetlb_total_pages())
> >  		* sysctl_overcommit_ratio / 100) + total_swap_pages;
> 
> This is a big change, and it wasn't even changelogged.  It's
> potentially a tremendous increase in the expense of a read from
> /proc/meminfo, which is a file that lots of tools will be polling. 
> Many of those tools we don't even know about or have access to.
Assume we don't read /proc/meminfo too often.

> The change is unneeded if sysctl_overcommit_memory==OVERCOMMIT_NEVER,
> but that's hardly a fix.
> 
> Quite worrisome.
> 
> Perhaps a better approach would be to carefully tune the batch size
> according to the size of the machine.  Going all the way to INT_MAX/2
> is surely overkill.
I understand the concern. But the tuning according to machien size is
quite hard. Say a machine with 16 CPUs and we don't want the per-cpu
counter to be bigger than 1% memory. If we do mmap/munmap 32M, then the
system must have:
32M*16*100*N = 50*N G memory. To reduce the lock contention, N must be
more than 8. so the system must have more than 400G memory, where most
system hasn't such big memory.
the INT_MAX/2 is an arbitrary number because the batch counter is
meaningless with sysctl_overcommit_memory != OVERCOMMIT_NEVER

> > Index: linux/kernel/sysctl.c
> > ===================================================================
> > --- linux.orig/kernel/sysctl.c	2011-03-29 16:28:57.000000000 +0800
> > +++ linux/kernel/sysctl.c	2011-03-30 09:01:38.000000000 +0800
> > @@ -56,6 +56,7 @@
> >  #include <linux/kprobes.h>
> >  #include <linux/pipe_fs_i.h>
> >  #include <linux/oom.h>
> > +#include <linux/mman.h>
> >  
> >  #include <asm/uaccess.h>
> >  #include <asm/processor.h>
> > @@ -86,8 +87,6 @@
> >  #if defined(CONFIG_SYSCTL)
> >  
> >  /* External variables not in a header file. */
> > -extern int sysctl_overcommit_memory;
> > -extern int sysctl_overcommit_ratio;
> >  extern int max_threads;
> >  extern int core_uses_pid;
> >  extern int suid_dumpable;
> > @@ -977,7 +976,7 @@ static struct ctl_table vm_table[] = {
> >  		.data		= &sysctl_overcommit_memory,
> >  		.maxlen		= sizeof(sysctl_overcommit_memory),
> >  		.mode		= 0644,
> > -		.proc_handler	= proc_dointvec_minmax,
> > +		.proc_handler	= overcommit_memory_handler,
> >  		.extra1		= &zero,
> >  		.extra2		= &two,
> >  	},
> > Index: linux/mm/mmap.c
> > ===================================================================
> > --- linux.orig/mm/mmap.c	2011-03-30 08:59:23.000000000 +0800
> > +++ linux/mm/mmap.c	2011-03-30 09:01:38.000000000 +0800
> > @@ -93,6 +93,33 @@ int sysctl_max_map_count __read_mostly =
> >   */
> >  struct percpu_counter vm_committed_as ____cacheline_internodealigned_in_smp;
> >  
> > +static void overcommit_drain_counter(struct work_struct *dummy)
> > +{
> > +	/*
> > +	 * Flush percpu counter to global counter when batch is changed, see
> > +	 * vm_acct_memory for detail
> > +	 */
> > +	vm_acct_memory(0);
> > +}
> > +
> > +int overcommit_memory_handler(struct ctl_table *table, int write,
> > +                void __user *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +	int error;
> > +
> > +	error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > +	if (error)
> > +		return error;
> > +
> > +	if (write) {
> > +		/* Make sure each CPU sees the new sysctl_overcommit_memory */
> > +		smp_wmb();
> > +		schedule_on_each_cpu(overcommit_drain_counter);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Calling vm_acct_memory(0) is a bit of a hack.
> 
> Rather than open-coding this twice, it would be better to introduce a
> new percpu_counter core primitive to collapse the counters.
ok, that's fine.

Thanks,
Shaohua

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]mmap: improve scalability for updating vm_committed_as
  2011-03-31  0:56   ` Shaohua Li
@ 2011-03-31  2:34     ` Andrew Morton
  2011-03-31  2:57       ` Shaohua Li
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2011-03-31  2:34 UTC (permalink / raw)
  To: Shaohua Li; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <shaohua.li@intel.com> wrote:

> > This is a big change, and it wasn't even changelogged.  It's
> > potentially a tremendous increase in the expense of a read from
> > /proc/meminfo, which is a file that lots of tools will be polling. 
> > Many of those tools we don't even know about or have access to.
> Assume we don't read /proc/meminfo too often.

That's a poor assumption.  top(1) and vmstat(8) read it, for a start. 
There will be zillions of locally-developed monitoring tools which read
meminfo.

Now, it could be that something under meminfo reads _already_ does a
massive walk across all CPUs.  If so then we'll have already trained
people to avoid reading /proc/meminfo and this change might be
acceptable.

But if this isn't the case then it's quite likely that this change will
hurt some people quite a lot.  And, unfortunately, the sort of people
who we will hurt tend to be people who don't run our stuff until a long
time (years) after we wrote it.  By which time it's going to be quite
expensive to get a fix down the chain and into their hands.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]mmap: improve scalability for updating vm_committed_as
  2011-03-31  2:34     ` Andrew Morton
@ 2011-03-31  2:57       ` Shaohua Li
  2011-03-31  3:06         ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-03-31  2:57 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

On Thu, 2011-03-31 at 10:34 +0800, Andrew Morton wrote:
> On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <shaohua.li@intel.com> wrote:
> 
> > > This is a big change, and it wasn't even changelogged.  It's
> > > potentially a tremendous increase in the expense of a read from
> > > /proc/meminfo, which is a file that lots of tools will be polling. 
> > > Many of those tools we don't even know about or have access to.
> > Assume we don't read /proc/meminfo too often.
> 
> That's a poor assumption.  top(1) and vmstat(8) read it, for a start. 
> There will be zillions of locally-developed monitoring tools which read
> meminfo.
> 
> Now, it could be that something under meminfo reads _already_ does a
> massive walk across all CPUs.  If so then we'll have already trained
> people to avoid reading /proc/meminfo and this change might be
> acceptable.
> 
> But if this isn't the case then it's quite likely that this change will
> hurt some people quite a lot.  And, unfortunately, the sort of people
> who we will hurt tend to be people who don't run our stuff until a long
> time (years) after we wrote it.  By which time it's going to be quite
> expensive to get a fix down the chain and into their hands.
Just looked at the code. nr_blockdev_pages() of si_meminfo iterate all
block devices. For people who care about the time, their system must
have more block devices than CPUs. so this isn't a big issue?

Thanks,
Shaohua

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]mmap: improve scalability for updating vm_committed_as
  2011-03-31  2:57       ` Shaohua Li
@ 2011-03-31  3:06         ` Andrew Morton
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2011-03-31  3:06 UTC (permalink / raw)
  To: Shaohua Li; +Cc: linux-mm, lkml, Andi Kleen, Rik van Riel, Hugh Dickins

On Thu, 31 Mar 2011 10:57:19 +0800 Shaohua Li <shaohua.li@intel.com> wrote:

> On Thu, 2011-03-31 at 10:34 +0800, Andrew Morton wrote:
> > On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <shaohua.li@intel.com> wrote:
> > 
> > > > This is a big change, and it wasn't even changelogged.  It's
> > > > potentially a tremendous increase in the expense of a read from
> > > > /proc/meminfo, which is a file that lots of tools will be polling. 
> > > > Many of those tools we don't even know about or have access to.
> > > Assume we don't read /proc/meminfo too often.
> > 
> > That's a poor assumption.  top(1) and vmstat(8) read it, for a start. 
> > There will be zillions of locally-developed monitoring tools which read
> > meminfo.
> > 
> > Now, it could be that something under meminfo reads _already_ does a
> > massive walk across all CPUs.  If so then we'll have already trained
> > people to avoid reading /proc/meminfo and this change might be
> > acceptable.
> > 
> > But if this isn't the case then it's quite likely that this change will
> > hurt some people quite a lot.  And, unfortunately, the sort of people
> > who we will hurt tend to be people who don't run our stuff until a long
> > time (years) after we wrote it.  By which time it's going to be quite
> > expensive to get a fix down the chain and into their hands.
> Just looked at the code. nr_blockdev_pages() of si_meminfo iterate all
> block devices. For people who care about the time, their system must
> have more block devices than CPUs.

How can we be sure of that?

> so this isn't a big issue?

Well it might be.  Experience tells us that some people are likely to
get bitten by this.

It's far safer and saner to find a solution which doesn't have big fat
failure modes!

Also, we don't (yet) know what we're *gaining* for this big fat failure
mode.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2011-03-31  3:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-30  1:17 [PATCH]mmap: improve scalability for updating vm_committed_as Shaohua Li
2011-03-30 22:51 ` Andrew Morton
2011-03-31  0:56   ` Shaohua Li
2011-03-31  2:34     ` Andrew Morton
2011-03-31  2:57       ` Shaohua Li
2011-03-31  3:06         ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).