meminfo Committed_AS underflows

DISCUSSION Dave Hansen 5 participants 12 messages Apr 14, 2009 – Apr 28, 2009

Subsystems (5)

Memory management PowerPC MM core Proc filesystem +1 more subsystems

lore

Share & tools

lore

Dave Hansen

author

2009-04-14 19:33 UTC #0 lore raw

I have a set of ppc64 machines that seem to spontaneously get underflows in /proc/meminfo's Committed_AS field:

        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB
              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB

As you can see, it bounces in and out of it. I think the problem is here:

        #define ACCT_THRESHOLD  max(16, NR_CPUS * 2)
        ...
        void vm_acct_memory(long pages)
        {
                long *local;
        
                preempt_disable();
                local = &__get_cpu_var(committed_space);
                *local += pages;
                if (*local > ACCT_THRESHOLD || *local < -ACCT_THRESHOLD) {
                        atomic_long_add(*local, &vm_committed_space);
                        *local = 0;
                }
                preempt_enable();
        }

Plus, some joker set CONFIG_NR_CPUS=1024.

nr_cpus (1024) * 2 * page_size (64k) = 128MB. That means each cpu can skew the counter by 128MB. With 1024 CPUs that means that we can have ~128GB of outstanding percpu accounting that meminfo doesn't see. Let's say we do vm_acct_memory(128MB-1) on 1023 of the CPUs, then on the other CPU, we do vm_acct_memory(-128GB).

The 1023 cpus won't ever hit the ACCT_THRESHOLD. The 1 CPU that did will decrement the global 'vm_committed_space' by ~128 GB. Underflow. Yay. This happens on a much smaller scale now.

Should we be protecting meminfo so that it spits slightly more sane numbers out to the user?

-- Dave

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

KOSAKI Motohiro

2009-04-15 02:04 UTC #1 lore raw

Quoted 46 lines

I have a set of ppc64 machines that seem to spontaneously get underflows in /proc/meminfo's Committed_AS field:

        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB
              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB

As you can see, it bounces in and out of it. I think the problem is here:

        #define ACCT_THRESHOLD  max(16, NR_CPUS * 2)
        ...
        void vm_acct_memory(long pages)
        {
                long *local;
        
                preempt_disable();
                local = &__get_cpu_var(committed_space);
                *local += pages;
                if (*local > ACCT_THRESHOLD || *local < -ACCT_THRESHOLD) {
                        atomic_long_add(*local, &vm_committed_space);
                        *local = 0;
                }
                preempt_enable();
        }

Plus, some joker set CONFIG_NR_CPUS=1024.

Quoted 6 lines

The 1023 cpus won't ever hit the ACCT_THRESHOLD. The 1 CPU that did will decrement the global 'vm_committed_space' by ~128 GB. Underflow. Yay. This happens on a much smaller scale now.
Should we be protecting meminfo so that it spits slightly more sane numbers out to the user?

Can you try to this patch? (Oh well, I can't reproduce this underflow on my small machine)

===============

Dave Hansen reported committed_AS field can underfolow.

Quoted 7 lines

        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB

Quoted 8 lines

              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB

Because NR_CPU can be greater than 1000. and meminfo_proc_show() doesn't have underflow check.

this patch have two change.

1. Change NR_CPU to nr_online_cpus()
vm_acct_memory() isn't fast-path. then cpumask_weight() calculation isn't so expensive and the parameter for scalability issue should consider number of _physical_ cpus. not theoretical maximum number.
2. Add under-flow check to meminfo_proc_show().
Almost field in /proc/meminfo have underflow check. but Committed_AS is significant exeption.
it should do.

Reported-by Dave Hansen <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 fs/proc/meminfo.c |    2 ++
 mm/swap.c         |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

fs/proc/meminfo.c +2 -0

Index: b/fs/proc/meminfo.c
===================================================================
@@ -36,6 +36,8 @@ static int meminfo_proc_show(struct seq_
 	si_meminfo(&i);
 	si_swapinfo(&i);
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;

mm/swap.c +1 -1

Index: b/mm/swap.c
===================================================================
@@ -519,7 +519,7 @@ EXPORT_SYMBOL(pagevec_lookup_tag);
  * We tolerate a little inaccuracy to avoid ping-ponging the counter between
  * CPUs
  */
-#define ACCT_THRESHOLD	max(16, NR_CPUS * 2)
+#define ACCT_THRESHOLD	max_t(long, 16, num_online_cpus() * 2)
 
 static DEFINE_PER_CPU(long, committed_space);

Balbir Singh

2009-04-15 03:35 UTC #2 lore raw

fs/proc/meminfo.c static int meminfo_proc_show(struct seq_ · line 40

Quoted 3 lines · KOSAKI Motohiro

fs/proc/meminfo.c +2 -0

 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;

Isn't this like pushing the problem under the rug?

Quoted 8 lines

fs/proc/meminfo.c +0 -0
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;
Index: b/mm/swap.c
===================================================================

mm/swap.c EXPORT_SYMBOL(pagevec_lookup_tag); · line 523

Quoted 6 lines

mm/swap.c +1 -1

@@ -519,7 +519,7 @@ EXPORT_SYMBOL(pagevec_lookup_tag);
  * We tolerate a little inaccuracy to avoid ping-ponging the counter between
  * CPUs
  */
-#define ACCT_THRESHOLD	max(16, NR_CPUS * 2)
+#define ACCT_THRESHOLD	max_t(long, 16, num_online_cpus() * 2)

Hmm.. this is a one time expansion, free of CPU hotplug.

Should we use nr_cpu_ids or num_possible_cpus()?

Signature · 7 lines

	Balbir

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

KOSAKI Motohiro

2009-04-15 04:10 UTC #3 lore raw

Quoted 7 lines · KOSAKI Motohiro

fs/proc/meminfo.c static int meminfo_proc_show(struct seq_ · line 40
Quoted 4 lines
fs/proc/meminfo.c +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
Isn't this like pushing the problem under the rug?

global_page_state() already has same logic.
IOW almost meminfo filed has this one (except Commited_AS).

Quoted 17 lines

Quoted 8 lines
fs/proc/meminfo.c +0 -0
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;
Index: b/mm/swap.c
===================================================================
mm/swap.c EXPORT_SYMBOL(pagevec_lookup_tag); · line 523
Quoted 6 lines
mm/swap.c +1 -1
@@ -519,7 +519,7 @@ EXPORT_SYMBOL(pagevec_lookup_tag);
  * We tolerate a little inaccuracy to avoid ping-ponging the counter between
  * CPUs
  */
-#define ACCT_THRESHOLD	max(16, NR_CPUS * 2)
+#define ACCT_THRESHOLD	max_t(long, 16, num_online_cpus() * 2)
 
Hmm.. this is a one time expansion, free of CPU hotplug.

Quoted 1 line

Should we use nr_cpu_ids or num_possible_cpus()?

#define num_online_cpus() cpumask_weight(cpu_online_mask)
#define num_possible_cpus() cpumask_weight(cpu_possible_mask)

num_possible_cpus() have the same calculation cost.
nr_cpu_ids isn't proper value.
it point to valid cpu-id range, no related number of online nor possible cpus.

Balbir Singh

2009-04-15 08:47 UTC #4 lore raw

Quoted 11 lines · KOSAKI Motohiro

Quoted 7 lines · KOSAKI Motohiro
fs/proc/meminfo.c static int meminfo_proc_show(struct seq_ · line 40
Quoted 4 lines
fs/proc/meminfo.c +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
Isn't this like pushing the problem under the rug?
global_page_state() already has same logic.
IOW almost meminfo filed has this one (except Commited_AS).

Quoted 18 lines

Quoted 18 lines
Quoted 9 lines
fs/proc/meminfo.c +0 -0
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;
Index: b/mm/swap.c
===================================================================
mm/swap.c EXPORT_SYMBOL(pagevec_lookup_tag); · line 523
Quoted 6 lines
mm/swap.c +1 -1
@@ -519,7 +519,7 @@ EXPORT_SYMBOL(pagevec_lookup_tag);
  * We tolerate a little inaccuracy to avoid ping-ponging the counter between
  * CPUs
  */
-#define ACCT_THRESHOLD	max(16, NR_CPUS * 2)
+#define ACCT_THRESHOLD	max_t(long, 16, num_online_cpus() * 2)
 
Hmm.. this is a one time expansion, free of CPU hotplug.

Quoted 9 lines

Quoted 1 line
Should we use nr_cpu_ids or num_possible_cpus()?
#define num_online_cpus() cpumask_weight(cpu_online_mask)
#define num_possible_cpus() cpumask_weight(cpu_possible_mask)
num_possible_cpus() have the same calculation cost.
nr_cpu_ids isn't proper value.
it point to valid cpu-id range, no related number of online nor possible cpus.

Since the value is just a basis for thresholds, num_online_cpus() might work.

Signature · 7 lines

	Balbir

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

Andrew Morton

maintainer

2009-04-27 20:31 UTC #5 lore raw

Quoted 8 lines · On Wed, 15 Apr 2009 14:17:13 +0530 Balbir Singh <[email protected]>…

Quoted 6 lines · KOSAKI Motohiro
Quoted 6 lines · KOSAKI Motohiro
fs/proc/meminfo.c static int meminfo_proc_show(struct seq_ · line 40
Quoted 4 lines
fs/proc/meminfo.c +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;

Is there a reason why we can't use a boring old percpu_counter for vm_committed_space? That way the meminfo code can just use percpu_counter_read_positive().

Or perhaps just percpu_counter_read(). The percpu_counter code does a better job of handling large cpu counts than the mysteriously-duplicative open-coded stuff we have there.

KOSAKI Motohiro

2009-04-28 03:07 UTC #6 lore raw

Quoted 12 lines

Quoted 8 lines · On Wed, 15 Apr 2009 14:17:13 +0530 Balbir Singh <[email protected]>…
Quoted 6 lines · KOSAKI Motohiro
Quoted 6 lines · KOSAKI Motohiro
Review
Quoted 4 lines
(quoted diff) +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;

Quoted 7 lines

Is there a reason why we can't use a boring old percpu_counter for vm_committed_space? That way the meminfo code can just use percpu_counter_read_positive().
Or perhaps just percpu_counter_read(). The percpu_counter code does a better job of handling large cpu counts than the mysteriously-duplicative open-coded stuff we have there.

At that time, I thought smallest patch is better because it can send -stable tree easily.
but maybe I was wrong. it made bikeshed discussion :(

ok, I'm going to right way.

=========================================
Subject: [PATCH] fix Committed_AS underfolow on large NR_CPUS environment

As reported by Dave Hansen, the Committed_AS field can underflow in certain situations:

Quoted 7 lines

        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB

Quoted 8 lines

              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB

Because NR_CPUS can be greater than 1000 and meminfo_proc_show() does not check for underflow.

But NR_CPUS proportional isn't good calculation. In general, possibility of lock contention is proportional to the number of online cpus, not theorical maximum cpus (NR_CPUS).
the current kernel has generic percpu-counter stuff. using it is right way. it makes code simplify and percpu_counter_read_positive() don't make underflow issue.

Reported-by Dave Hansen <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

Cc Eric B Munson <[email protected]>

---
 fs/proc/meminfo.c    |    2 +-
 include/linux/mman.h |    9 +++------
 mm/mmap.c            |   12 ++++++------
 mm/nommu.c           |   13 +++++++------
 mm/swap.c            |   46 ----------------------------------------------
 5 files changed, 17 insertions(+), 65 deletions(-)

fs/proc/meminfo.c +1 -1

Index: b/fs/proc/meminfo.c
===================================================================
@@ -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 = atomic_long_read(&vm_committed_space);
+	committed = percpu_counter_read_positive(&vm_committed_as);
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;

mm/mmap.c +6 -6

Index: b/mm/mmap.c
===================================================================
@@ -85,7 +85,7 @@ EXPORT_SYMBOL(vm_get_page_prot);
 int sysctl_overcommit_memory = OVERCOMMIT_GUESS;  /* heuristic overcommit */
 int sysctl_overcommit_ratio = 50;	/* default is 50% */
 int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
-atomic_long_t vm_committed_space = ATOMIC_LONG_INIT(0);
+struct percpu_counter vm_committed_as;
 
 /*
  * Check that a process has enough memory to allocate a new virtual

@@ -179,11 +179,7 @@ int __vm_enough_memory(struct mm_struct 
 	if (mm)
 		allowed -= mm->total_vm / 32;
 
-	/*
-	 * cast `allowed' as a signed long because vm_committed_space
-	 * sometimes has a negative value
-	 */
-	if (atomic_long_read(&vm_committed_space) < (long)allowed)
+	if (percpu_counter_read_positive(&vm_committed_as) < allowed)
 		return 0;
 error:
 	vm_unacct_memory(pages);

@@ -2481,4 +2477,8 @@ void mm_drop_all_locks(struct mm_struct 
  */
 void __init mmap_init(void)
 {
+	int ret;
+
+	ret = percpu_counter_init(&vm_committed_as, 0);
+	VM_BUG_ON(ret);
 }

mm/nommu.c +7 -6

Index: b/mm/nommu.c
===================================================================
@@ -62,7 +62,7 @@ void *high_memory;
 struct page *mem_map;
 unsigned long max_mapnr;
 unsigned long num_physpages;
-atomic_long_t vm_committed_space = ATOMIC_LONG_INIT(0);
+struct percpu_counter vm_committed_as;
 int sysctl_overcommit_memory = OVERCOMMIT_GUESS; /* heuristic overcommit */
 int sysctl_overcommit_ratio = 50; /* default is 50% */
 int sysctl_max_map_count = DEFAULT_MAX_MAP_COUNT;

@@ -463,6 +463,10 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
  */
 void __init mmap_init(void)
 {
+	int ret;
+
+	ret = percpu_counter_init(&vm_committed_as, 0);
+	VM_BUG_ON(ret);
 	vm_region_jar = KMEM_CACHE(vm_region, SLAB_PANIC);
 }

@@ -1847,12 +1851,9 @@ int __vm_enough_memory(struct mm_struct 
 	if (mm)
 		allowed -= mm->total_vm / 32;
 
-	/*
-	 * cast `allowed' as a signed long because vm_committed_space
-	 * sometimes has a negative value
-	 */
-	if (atomic_long_read(&vm_committed_space) < (long)allowed)
+	if (percpu_counter_read_positive(&vm_committed_as) < allowed)
 		return 0;
+
 error:
 	vm_unacct_memory(pages);

mm/swap.c +0 ~~-46~~

56 lines not on this page · raw message

include/linux/mman.h +3 -6

Index: b/include/linux/mman.h
===================================================================
@@ -12,21 +12,18 @@
 
 #ifdef __KERNEL__
 #include <linux/mm.h>
+#include <linux/percpu_counter.h>
 
 #include <asm/atomic.h>
 
 extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
-extern atomic_long_t vm_committed_space;
+extern struct percpu_counter vm_committed_as;
 
-#ifdef CONFIG_SMP
-extern void vm_acct_memory(long pages);
-#else
 static inline void vm_acct_memory(long pages)
 {
-	atomic_long_add(pages, &vm_committed_space);
+	percpu_counter_add(&vm_committed_as, pages);
 }
-#endif
 
 static inline void vm_unacct_memory(long pages)
 {

Andrew Morton

maintainer

2009-04-28 03:32 UTC #7 lore raw

Quoted 16 lines · On Tue, 28 Apr 2009 12:07:59 +0900 (JST) KOSAKI Motohiro…

Quoted 16 lines
Quoted 8 lines · On Wed, 15 Apr 2009 14:17:13 +0530 Balbir Singh <[email protected]>…
Quoted 6 lines · KOSAKI Motohiro
Quoted 6 lines · KOSAKI Motohiro
Review
Quoted 4 lines
(quoted diff) +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
Is there a reason why we can't use a boring old percpu_counter for vm_committed_space? That way the meminfo code can just use percpu_counter_read_positive().

Quoted 7 lines

Quoted 3 lines
Or perhaps just percpu_counter_read(). The percpu_counter code does a better job of handling large cpu counts than the mysteriously-duplicative open-coded stuff we have there.
At that time, I thought smallest patch is better because it can send -stable tree easily.
but maybe I was wrong. it made bikeshed discussion :(

Yes, I know what you mean. But otoh it's a good idea to keep -stable in sync with mainline - it means that -stable can merge things which have had a suitable amount of testing.

Quoted 38 lines

ok, I'm going to right way.
=========================================
Subject: [PATCH] fix Committed_AS underfolow on large NR_CPUS environment
As reported by Dave Hansen, the Committed_AS field can underflow in certain situations:
Quoted 7 lines
        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB
Quoted 8 lines
              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB
Because NR_CPUS can be greater than 1000 and meminfo_proc_show() does not check for underflow.
But NR_CPUS proportional isn't good calculation. In general, possibility of lock contention is proportional to the number of online cpus, not theorical maximum cpus (NR_CPUS).
the current kernel has generic percpu-counter stuff. using it is right way. it makes code simplify and percpu_counter_read_positive() don't make underflow issue.
Reported-by Dave Hansen <[email protected]>
Signed-off-by KOSAKI Motohiro <[email protected]>
Cc Eric B Munson <[email protected]>

Quoted 7 lines

---
 fs/proc/meminfo.c    |    2 +-
 include/linux/mman.h |    9 +++------
 mm/mmap.c            |   12 ++++++------
 mm/nommu.c           |   13 +++++++------
 mm/swap.c            |   46 ----------------------------------------------
 5 files changed, 17 insertions(+), 65 deletions(-)

Well that was nice.

There's potential here for weird performance regressions, so I think that if we do this in mainline, we should wait a while (a few weeks?) before backporting it.

Do we know how long this bug has existed for? Quite a while, I expect?

KOSAKI Motohiro

2009-04-28 04:25 UTC #8 lore raw

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
Is there a reason why we can't use a boring old percpu_counter for vm_committed_space? That way the meminfo code can just use percpu_counter_read_positive().
Or perhaps just percpu_counter_read(). The percpu_counter code does a better job of handling large cpu counts than the mysteriously-duplicative open-coded stuff we have there.

Quoted 7 lines

Quoted 3 lines
At that time, I thought smallest patch is better because it can send -stable tree easily.
but maybe I was wrong. it made bikeshed discussion :(
Yes, I know what you mean. But otoh it's a good idea to keep -stable in sync with mainline - it means that -stable can merge things which have had a suitable amount of testing.

Agreed.

Quoted 11 lines

Quoted 10 lines
Reported-by Dave Hansen <[email protected]>
Signed-off-by KOSAKI Motohiro <[email protected]>
Cc Eric B Munson <[email protected]>
---
 fs/proc/meminfo.c    |    2 +-
 include/linux/mman.h |    9 +++------
 mm/mmap.c            |   12 ++++++------
 mm/nommu.c           |   13 +++++++------
 mm/swap.c            |   46 ----------------------------------------------
 5 files changed, 17 insertions(+), 65 deletions(-)

Quoted 7 lines

Well that was nice.
There's potential here for weird performance regressions, so I think that if we do this in mainline, we should wait a while (a few weeks?) before backporting it.
Do we know how long this bug has existed for? Quite a while, I expect?

ACCT_THRESHOLD was introduced bit-keeper age.
powerpc default NR_CPUS is still less 128.

% grep NR_CPUS *
cell_defconfig:CONFIG_NR_CPUS=4
celleb_defconfig:CONFIG_NR_CPUS=4
chrp32_defconfig:CONFIG_NR_CPUS=4
g5_defconfig:CONFIG_NR_CPUS=4
iseries_defconfig:CONFIG_NR_CPUS=32
maple_defconfig:CONFIG_NR_CPUS=4
mpc86xx_defconfig:CONFIG_NR_CPUS=2
pasemi_defconfig:CONFIG_NR_CPUS=2
ppc64_defconfig:CONFIG_NR_CPUS=32
ps3_defconfig:CONFIG_NR_CPUS=2
pseries_defconfig:CONFIG_NR_CPUS=128

powerpc maximum NR_CPUS was increased following commit. I'm not sure about one year is short or long.

==========================================================
commit 90035fe378c7459ba19c43c63d5f878284224ce4

Author: Tony Breeds <[email protected]>
Date:   Thu Apr 24 13:43:49 2008 +1000

[POWERPC] Raise the upper limit of NR_CPUS and move the pacas into the BSS

This adds the required functionality to fill in all pacas at runtime.

With NR_CPUS=1024
text data bss dec hex filename
137 1704032 0 1704169 1a00e9 arch/powerpc/kernel/paca.o :Before
121 1179744 524288 1704153 1a00d9 arch/powerpc/kernel/paca.o :After

Also remove unneeded #includes from arch/powerpc/kernel/paca.c

Signed-off-by Tony Breeds <[email protected]>

Signed-off-by Paul Mackerras <[email protected]>

arch/powerpc/platforms/Kconfig.cputype +2 -2

index 5fc7fac..f7efaa9 100644
@@ -220,8 +220,8 @@ config SMP
          If you don't know what to do here, say N.
 
 config NR_CPUS
-       int "Maximum number of CPUs (2-128)"
-       range 2 128
+       int "Maximum number of CPUs (2-1024)"
+       range 2 1024
        depends on SMP
        default "32" if PPC64
        default "4"

Dave Hansen

author

2009-04-28 08:17 UTC #9 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

There's potential here for weird performance regressions, so I think that if we do this in mainline, we should wait a while (a few weeks?) before backporting it.
Do we know how long this bug has existed for? Quite a while, I expect?

Yeah, we didn't notice it until a recent enterprise distro got a config with NR_CPUS=1024. That opened the window up in a major way because of the way we calculated the threshold.

-- Dave

KOSAKI Motohiro

2009-04-15 04:12 UTC #10 lore raw

In reply to KOSAKI Motohiro

fix stupid mistake.

Changelog:
since v1
- change the type of commited value from ulong to long

=========================
Subject: [PATCH v2] fix Commited_AS underflow

Dave Hansen reported committed_AS field can underfolow.

Quoted 7 lines

        # while true; do cat /proc/meminfo  | grep _AS; sleep 1; done | uniq -c
              1 Committed_AS: 18446744073709323392 kB
             11 Committed_AS: 18446744073709455488 kB
              6 Committed_AS:    35136 kB
              5 Committed_AS: 18446744073709454400 kB
              7 Committed_AS:    35904 kB
              3 Committed_AS: 18446744073709453248 kB

Quoted 8 lines

              2 Committed_AS:    34752 kB
              9 Committed_AS: 18446744073709453248 kB
              8 Committed_AS:    34752 kB
              3 Committed_AS: 18446744073709320960 kB
              7 Committed_AS: 18446744073709454080 kB
              3 Committed_AS: 18446744073709320960 kB
              5 Committed_AS: 18446744073709454080 kB
              6 Committed_AS: 18446744073709320960 kB

Because NR_CPU can be greater than 1000. and meminfo_proc_show() doesn't have underflow check.

this patch have two change.

Reported-by Dave Hansen <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 fs/proc/meminfo.c |    4 +++-
 mm/swap.c         |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

fs/proc/meminfo.c +3 -1

Index: b/fs/proc/meminfo.c
===================================================================
@@ -22,7 +22,7 @@ void __attribute__((weak)) arch_report_m
 static int meminfo_proc_show(struct seq_file *m, void *v)
 {
 	struct sysinfo i;
-	unsigned long committed;
+	long committed;
 	unsigned long allowed;
 	struct vmalloc_info vmi;
 	long cached;

@@ -36,6 +36,8 @@ static int meminfo_proc_show(struct seq_
 	si_meminfo(&i);
 	si_swapinfo(&i);
 	committed = atomic_long_read(&vm_committed_space);
+	if (committed < 0)
+		committed = 0;
 	allowed = ((totalram_pages - hugetlb_total_pages())
 		* sysctl_overcommit_ratio / 100) + total_swap_pages;

mm/swap.c +1 -1

Index: b/mm/swap.c
===================================================================
@@ -519,7 +519,7 @@ EXPORT_SYMBOL(pagevec_lookup_tag);
  * We tolerate a little inaccuracy to avoid ping-ponging the counter between
  * CPUs
  */
-#define ACCT_THRESHOLD	max(16, NR_CPUS * 2)
+#define ACCT_THRESHOLD	max_t(long, 16, num_online_cpus() * 2)
 
 static DEFINE_PER_CPU(long, committed_space);

Alan Cox

2009-04-15 08:31 UTC #11 lore raw

In reply to Dave Hansen

Quoted 6 lines

The 1023 cpus won't ever hit the ACCT_THRESHOLD. The 1 CPU that did will decrement the global 'vm_committed_space' by ~128 GB. Underflow. Yay. This happens on a much smaller scale now.
Should we be protecting meminfo so that it spits slightly more sane numbers out to the user?

Yes. It used to be accurate but the changes were put in as the memory accounting value was actually starting to show up in profiles as it bounced around the CPUs - perhaps the constraint should instead be a worst case error as percentage of total system memory ?

Alan