Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Ake]

Use of rlim[RLIMIT_RSS] in mm/filemap.c is wrong.
It is passed down to kernel as a number of bytes but is being used as a
number of pages.

There is also a misinformative comment in fs/proc/array.c
in proc_pid_stat where it says
mm ? mm->rss : 0, /* you might want to shift this left 3 */
the number 3 should probably be PAGE_SHIFT-10.

-- 
Ake Sandgren, HPC2N, Umea University, S-90187 Umea, Sweden
Internet: ake@hpc2n.umu.se	Phone: +46 90 7866134 Fax: +46 90 7866126
Mobile: +46 70 7716134 WWW: http://www.hpc2n.umu.se

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Marcelo Tosatti]

On Wed, Jan 26, 2005 at 12:07:50PM +0100, Ake wrote:
> Use of rlim[RLIMIT_RSS] in mm/filemap.c is wrong.
> It is passed down to kernel as a number of bytes but is being used as a
> number of pages.
> 
> There is also a misinformative comment in fs/proc/array.c
> in proc_pid_stat where it says
> mm ? mm->rss : 0, /* you might want to shift this left 3 */
> the number 3 should probably be PAGE_SHIFT-10.

Amazing that this has never been noticed before - I bet not many people use RSS 
limits with madvise().

This transform the rlimit in pages before the comparison, can you please test
it.

--- a/mm/filemap.c.orig	2004-11-17 09:54:22.000000000 -0200
+++ b/mm/filemap.c	2005-01-26 15:21:10.614842296 -0200
@@ -2609,6 +2609,9 @@
 	error = -EIO;
 	rlim_rss = current->rlim ?  current->rlim[RLIMIT_RSS].rlim_cur :
 				LONG_MAX; /* default: see resource.h */
+
+	rlim_rss = (rlim_rss & PAGE_MASK) >> PAGE_SHIFT;
+
 	if ((vma->vm_mm->rss + (end - start)) > rlim_rss)
 		return error;
 

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Ake]

On Wed, Jan 26, 2005 at 12:49:04PM -0200, Marcelo Tosatti wrote:
> > There is also a misinformative comment in fs/proc/array.c
> > in proc_pid_stat where it says
> > mm ? mm->rss : 0, /* you might want to shift this left 3 */
> > the number 3 should probably be PAGE_SHIFT-10.

Don't forget the comment (it's still there in 2.6) :-)

> Amazing that this has never been noticed before - I bet not many people use RSS 
> limits with madvise().

Neither do we. :-)

I just found it when trying to figure out if the kernel actually used
any of the rlimits and if so how.
(Trying to make PBS work correctly)

> This transform the rlimit in pages before the comparison, can you please test
> it.
> 
> --- a/mm/filemap.c.orig	2004-11-17 09:54:22.000000000 -0200
> +++ b/mm/filemap.c	2005-01-26 15:21:10.614842296 -0200
> @@ -2609,6 +2609,9 @@
>  	error = -EIO;
>  	rlim_rss = current->rlim ?  current->rlim[RLIMIT_RSS].rlim_cur :
>  				LONG_MAX; /* default: see resource.h */
> +
> +	rlim_rss = (rlim_rss & PAGE_MASK) >> PAGE_SHIFT;
> +
>  	if ((vma->vm_mm->rss + (end - start)) > rlim_rss)
>  		return error;

Since we don't use it i can't really test it, but a visual inspection is
good enough in this simple case. And since this is the only place in 2.4
that RLIMIT_RSS is used, the problem is solved.

BTW do you know if there is any plans for 2.6++ to actually use
RLIMIT_RSS? I saw a hint in that direction in mm/thrash.c
grab_swap_token but it is commented out and only skeleton code...

I'm asking since the above code piece has been removed.

-- 
Ake Sandgren, HPC2N, Umea University, S-90187 Umea, Sweden
Internet: ake@hpc2n.umu.se	Phone: +46 90 7866134 Fax: +46 90 7866126
Mobile: +46 70 7716134 WWW: http://www.hpc2n.umu.se

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Marcelo Tosatti]

On Thu, Jan 27, 2005 at 07:38:49AM +0100, Ake wrote:

> On Wed, Jan 26, 2005 at 12:49:04PM -0200, Marcelo Tosatti wrote:
> > > There is also a misinformative comment in fs/proc/array.c
 > > in proc_pid_stat where it says
> > > mm ? mm->rss : 0, /* you might want to shift this left 3 */
> > > the number 3 should probably be PAGE_SHIFT-10.
> 
> Don't forget the comment (it's still there in 2.6) :-)
> 
> > Amazing that this has never been noticed before - I bet not many people use RSS 
> > limits with madvise().
> 
> Neither do we. :-)
> 
> I just found it when trying to figure out if the kernel actually used
> any of the rlimits and if so how.
> (Trying to make PBS work correctly)
> 
> > This transform the rlimit in pages before the comparison, can you please test
> > it.
> > 
> > --- a/mm/filemap.c.orig	2004-11-17 09:54:22.000000000 -0200
> > +++ b/mm/filemap.c	2005-01-26 15:21:10.614842296 -0200
> > @@ -2609,6 +2609,9 @@
> >  	error = -EIO;
> >  	rlim_rss = current->rlim ?  current->rlim[RLIMIT_RSS].rlim_cur :
> >  				LONG_MAX; /* default: see resource.h */
> > +
> > +	rlim_rss = (rlim_rss & PAGE_MASK) >> PAGE_SHIFT;
> > +
> >  	if ((vma->vm_mm->rss + (end - start)) > rlim_rss)
> >  		return error;
> 
> Since we don't use it i can't really test it, but a visual inspection is
> good enough in this simple case. And since this is the only place in 2.4
> that RLIMIT_RSS is used, the problem is solved.
> 
> BTW do you know if there is any plans for 2.6++ to actually use
> RLIMIT_RSS? I saw a hint in that direction in mm/thrash.c
> grab_swap_token but it is commented out and only skeleton code...

Nope, RLIMIT_RSS does not seem to be used at all in v2.6:

linux-2.6.11-rc1 # findgrep RLIMIT_RSS
./fs/proc/array.c
./include/asm-arm26/resource.h
./include/asm-ia64/resource.h
./include/asm-frv/resource.h
./include/asm-i386/resource.h
./include/asm-alpha/resource.h
./include/asm-ppc64/resource.h
./include/asm-x86_64/resource.h
./include/asm-s390/resource.h
./include/asm-mips/resource.h
./include/asm-sparc64/resource.h
./include/asm-cris/resource.h
./include/asm-v850/resource.h
./include/asm-m68k/resource.h
./include/asm-sparc/resource.h
./include/asm-parisc/resource.h
./include/asm-arm/resource.h
./include/asm-sh/resource.h
./include/asm-m32r/resource.h
./include/asm-h8300/resource.h
./include/asm-ppc/resource.h

> I'm asking since the above code piece has been removed.

Its there for compatibility reasons, support for it might be added
in the future?

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Marcelo Tosatti]

On Fri, Jan 28, 2005 at 03:09:40PM +0000, Hugh Dickins wrote:
> On Thu, 27 Jan 2005, Marcelo Tosatti wrote:
> > On Thu, Jan 27, 2005 at 07:38:49AM +0100, Ake wrote:
> > > On Wed, Jan 26, 2005 at 12:49:04PM -0200, Marcelo Tosatti wrote:
> > > > 
> > > > --- a/mm/filemap.c.orig	2004-11-17 09:54:22.000000000 -0200
> > > > +++ b/mm/filemap.c	2005-01-26 15:21:10.614842296 -0200
> > > > @@ -2609,6 +2609,9 @@
> > > >  	error = -EIO;
> > > >  	rlim_rss = current->rlim ?  current->rlim[RLIMIT_RSS].rlim_cur :
> > > >  				LONG_MAX; /* default: see resource.h */
> > > > +
> > > > +	rlim_rss = (rlim_rss & PAGE_MASK) >> PAGE_SHIFT;
> > > > +
> > > >  	if ((vma->vm_mm->rss + (end - start)) > rlim_rss)
> > > >  		return error;
> 
> Isn't the right fix just to remove rlim_rss and this RLIMIT_RSS code
> from here?  It's pretty silly for madvise alone to be taking notice
> of it.  Presumably the code crept in by mistake from some tree which
> was using an RLIMIT_RSS patch on 2.4.

True. Will do it. 

> > > Since we don't use it i can't really test it, but a visual inspection is
> > > good enough in this simple case. And since this is the only place in 2.4
> > > that RLIMIT_RSS is used, the problem is solved.
> > > 
> > > BTW do you know if there is any plans for 2.6++ to actually use
> > > RLIMIT_RSS? I saw a hint in that direction in mm/thrash.c
> > > grab_swap_token but it is commented out and only skeleton code...
> > 
> > Nope, RLIMIT_RSS does not seem to be used at all in v2.6:
> > 
> > Its there for compatibility reasons, support for it might be added
> > in the future?
> 
> Rik had a patch implementing RLIMIT_RSS in 2.6-mm for a while.
> But I think there were a couple of problems with it, and no great
> demand for the feature, so Andrew dropped it until someone makes
> a better case for it.
> 
> Hugh

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Ake]

On Fri, Jan 28, 2005 at 03:09:40PM +0000, Hugh Dickins wrote:
> > > BTW do you know if there is any plans for 2.6++ to actually use
> > > RLIMIT_RSS? I saw a hint in that direction in mm/thrash.c
> > > grab_swap_token but it is commented out and only skeleton code...
> > 
> > Nope, RLIMIT_RSS does not seem to be used at all in v2.6:
> > 
> > Its there for compatibility reasons, support for it might be added
> > in the future?
> 
> Rik had a patch implementing RLIMIT_RSS in 2.6-mm for a while.
> But I think there were a couple of problems with it, and no great
> demand for the feature, so Andrew dropped it until someone makes
> a better case for it.

Well, the use for it is for compute clusters where you really would like
to be able to limit this. Esp on smp boxes where there is multiple
compute jobs running simultaneously. Be it mpi or separate jobs you
really want to limit their RSS use so they don't steal memory from each
other.

-- 
Ake Sandgren, HPC2N, Umea University, S-90187 Umea, Sweden
Internet: ake@hpc2n.umu.se	Phone: +46 90 7866134 Fax: +46 90 7866126
Mobile: +46 70 7716134 WWW: http://www.hpc2n.umu.se

Re: Bug in 2.4.26 in mm/filemap.c when using RLIMIT_RSS

[Hugh Dickins]

On Thu, 27 Jan 2005, Marcelo Tosatti wrote:
> On Thu, Jan 27, 2005 at 07:38:49AM +0100, Ake wrote:
> > On Wed, Jan 26, 2005 at 12:49:04PM -0200, Marcelo Tosatti wrote:
> > > 
> > > --- a/mm/filemap.c.orig	2004-11-17 09:54:22.000000000 -0200
> > > +++ b/mm/filemap.c	2005-01-26 15:21:10.614842296 -0200
> > > @@ -2609,6 +2609,9 @@
> > >  	error = -EIO;
> > >  	rlim_rss = current->rlim ?  current->rlim[RLIMIT_RSS].rlim_cur :
> > >  				LONG_MAX; /* default: see resource.h */
> > > +
> > > +	rlim_rss = (rlim_rss & PAGE_MASK) >> PAGE_SHIFT;
> > > +
> > >  	if ((vma->vm_mm->rss + (end - start)) > rlim_rss)
> > >  		return error;

Isn't the right fix just to remove rlim_rss and this RLIMIT_RSS code
from here?  It's pretty silly for madvise alone to be taking notice
of it.  Presumably the code crept in by mistake from some tree which
was using an RLIMIT_RSS patch on 2.4.

> > Since we don't use it i can't really test it, but a visual inspection is
> > good enough in this simple case. And since this is the only place in 2.4
> > that RLIMIT_RSS is used, the problem is solved.
> > 
> > BTW do you know if there is any plans for 2.6++ to actually use
> > RLIMIT_RSS? I saw a hint in that direction in mm/thrash.c
> > grab_swap_token but it is commented out and only skeleton code...
> 
> Nope, RLIMIT_RSS does not seem to be used at all in v2.6:
> 
> Its there for compatibility reasons, support for it might be added
> in the future?

Rik had a patch implementing RLIMIT_RSS in 2.6-mm for a while.
But I think there were a couple of problems with it, and no great
demand for the feature, so Andrew dropped it until someone makes
a better case for it.

Hugh