Re: [-mm][PATCH 2/4] Setup the memrlimit controller (v5)

[Andrew Morton]

On Wed, 11 Jun 2008 22:17:12 +0200
Andrea Righi <righi.andrea@xxxxxxxxx> wrote:

> Andrew Morton wrote:
> > On Wed, 11 Jun 2008 19:10:40 +0200 (MEST)
> > Andrea Righi <righi.andrea@xxxxxxxxx> wrote:
> > 
> >> Balbir Singh wrote:
> >>> +static int memrlimit_cgroup_write_strategy(char *buf, unsigned long long *tmp)
> >>> +{
> >>> +	*tmp = memparse(buf, &buf);
> >>> +	if (*buf != '\0')
> >>> +		return -EINVAL;
> >>> +
> >>> +	*tmp = PAGE_ALIGN(*tmp);
> >>> +	return 0;
> >>> +}
> >> We shouldn't use PAGE_ALIGN() here, otherwise we limit the address space
> >> to 4GB on 32-bit architectures (that could be reasonable, because this
> >> is a per-cgroup limit and not per-process).
> >>
> >> Signed-off-by: Andrea Righi <righi.andrea@xxxxxxxxx>
> >> ---
> >>  mm/memrlimitcgroup.c |    4 +++-
> >>  1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/mm/memrlimitcgroup.c b/mm/memrlimitcgroup.c
> >> index 9a03d7d..2d42ff3 100644
> >> --- a/mm/memrlimitcgroup.c
> >> +++ b/mm/memrlimitcgroup.c
> >> @@ -29,6 +29,8 @@
> >>  #include <linux/res_counter.h>
> >>  #include <linux/memrlimitcgroup.h>
> >>  
> >> +#define PAGE_ALIGN64(addr) (((((addr)+PAGE_SIZE-1))>>PAGE_SHIFT)<<PAGE_SHIFT)
> >> +
> >>  struct cgroup_subsys memrlimit_cgroup_subsys;
> >>  
> >>  struct memrlimit_cgroup {
> >> @@ -124,7 +126,7 @@ static int memrlimit_cgroup_write_strategy(char *buf, unsigned long long *tmp)
> >>  	if (*buf != '\0')
> >>  		return -EINVAL;
> >>  
> >> -	*tmp = PAGE_ALIGN(*tmp);
> >> +	*tmp = PAGE_ALIGN64(*tmp);
> >>  	return 0;
> >>  }
> >>  
> > 
> > I don't beleive the change is needed.
> > 
> > #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
> > 
> > that implementation will behaved as desired when passed a 64-bit addr?
> 
> If I'm not doing something wrong, here is what happens on my i386 box:
> 
> $ uname -m
> i686
> $ cat 64-bit-page-align.c
> #include <stdio.h>
> #include <asm/page.h>
> 
> #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
> #define PAGE_ALIGN64(addr) (((((addr)+PAGE_SIZE-1))>>PAGE_SHIFT)<<PAGE_SHIFT)
> 
> #define SIZE ((1ULL << 32) - 1)
> 
> int main(int argc, char **argv)
> {
> 	unsigned long long good, bad;
> 
> 	good = (unsigned long long)PAGE_ALIGN64(SIZE);
> 	bad = (unsigned long long)PAGE_ALIGN(SIZE);
> 
> 	fprintf(stdout, "good = %llu, bad = %llu\n", good, bad);
> 
> 	return 0;
> }
> $ gcc -O2 -o 64-bit-page-align 64-bit-page-align.c
> $ ./64-bit-page-align
> good = 4294967296, bad = 0
>                    ^^^^^^^
> On a x86_64, instead, both PAGE_ALIGN()s work as expected:

That's weird.  We have an expression which contains a combination of UL
constants and ULL constants, and the compiler _isn't_ converting
everything into ULL.


> $ uname -m
> x86_64
> $ gcc -O2 -o 64-bit-page-align 64-bit-page-align.c
> $ ./64-bit-page-align
> good = 4294967296, bad = 4294967296
> 
> At least we could add something like:
> 
> #ifdef CONFIG_32BIT
> #define PAGE_ALIGN64(addr) (((((addr)+PAGE_SIZE-1))>>PAGE_SHIFT)<<PAGE_SHIFT)
> #else
> #define PAGE_ALIGN64(addr) PAGE_ALIGN(addr)
> #endif
> 
> But IMHO the single PAGE_ALIGN64() implementation is more clear.

No, we should just fix PAGE_ALIGN.  It should work correctly when
passed a long-long.  Otherwse it's just a timebomb.

This:

#define PAGE_ALIGN(addr) ({				\
	typeof(addr) __size = PAGE_SIZE;		\
	typeof(addr) __mask = PAGE_MASK;		\
	(addr + __size - 1) & __mask;			\
})

(with a suitable comment) does what we want.  I didn't check to see
whether this causes the compiler to generate larger code, but it
shouldn't.

If we go this way then first we should hoist the PAGE_ALIGN definitions
out of include/asm-*/page.h and into, umm, include/linux/mm.h. 
That might cause build errors but from a bit of grepping around, only
include/asm-arm/cacheflush.h is at risk, and it already includes
linux/mm.h.
--
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/