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

Andrew Morton
2008-06-11T22:56:19Z

On Thu, 12 Jun 2008 00:47:27 +0200
Andrea Righi <righi.andrea@gmail.com> wrote:

> Andrew Morton wrote:
> >> 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.
> >
>
> No, it doesn't work. The problem seems to be in the PAGE_MASK definition
> (from include/asm-x86/page.h for example):
>
> /* PAGE_SHIFT determines the page size */
> #define PAGE_SHIFT 12
> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> #define PAGE_MASK (~(PAGE_SIZE-1))
>
> The "~" is performed on a 32-bit value, so everything in "and" with
> PAGE_MASK greater than 4GB will be truncated to the 32-bit boundary.

OK, I oversimplified my testcase.

> What do you think about the following?
>
> #define PAGE_SIZE64 (1ULL << PAGE_SHIFT)
> #define PAGE_MASK64 (~(PAGE_SIZE64 - 1))
>
> #define PAGE_ALIGN(addr) ({ \
> typeof(addr) __size = PAGE_SIZE; \
> typeof(addr) __ret = (addr) + __size - 1; \
> __ret > -1UL ? __ret & PAGE_MASK64 : __ret & PAGE_MASK; \
> })

Complex. And I'd worry about added code overhead.

What about

#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)

?

afaict ALIGN() tries to do the right thing, and if it doesn't, we
should fix ALIGN().