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 <[email protected]>
---
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
On Wed, 30 Mar 2011 09:17:27 +0800
Shaohua Li <[email protected]> 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.
>
> ...
>
On Thu, 2011-03-31 at 06:51 +0800, Andrew Morton wrote:
> On Wed, 30 Mar 2011 09:17:27 +0800
> Shaohua Li <[email protected]> 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
On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <[email protected]> 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.
On Thu, 2011-03-31 at 10:34 +0800, Andrew Morton wrote:
> On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <[email protected]> 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
On Thu, 31 Mar 2011 10:57:19 +0800 Shaohua Li <[email protected]> wrote:
> On Thu, 2011-03-31 at 10:34 +0800, Andrew Morton wrote:
> > On Thu, 31 Mar 2011 08:56:43 +0800 Shaohua Li <[email protected]> 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.