Re: meminfo Committed_AS underflows

[Andrew Morton]

On Tue, 28 Apr 2009 12:07:59 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> wrote:

> > On Wed, 15 Apr 2009 14:17:13 +0530
> > Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > * KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> [2009-04-15 13:10:06]:
> > > 
> > > > > * KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> [2009-04-15 11:04:59]:
> > > > > 
> > > > > >  	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.
> 
> 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.

> 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:
> 
> >         # 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
> 
> 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 <dave@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> Cc: Eric B Munson <ebmunson@xxxxxxxxxx>
> ---
>  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?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/