2009-12-07 16:24:03

by Jan Beulich

[permalink] [raw]
Subject: [PATCH] mm/vmalloc: don't use vmalloc_end

At least on ia64 vmalloc_end is a global variable that VMALLOC_END
expands to. Hence having a local variable named vmalloc_end and
initialized from VMALLOC_END won't work on such platforms. Rename
these variables, and for consistency also rename vmalloc_start.

Signed-off-by: Jan Beulich <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Tony Luck <[email protected]>

---
mm/vmalloc.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

--- linux-2.6.32/mm/vmalloc.c
+++ 2.6.32-dont-use-vmalloc_end/mm/vmalloc.c
@@ -2060,13 +2060,13 @@ static unsigned long pvm_determine_end(s
struct vmap_area **pprev,
unsigned long align)
{
- const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
+ const unsigned long end = VMALLOC_END & ~(align - 1);
unsigned long addr;

if (*pnext)
- addr = min((*pnext)->va_start & ~(align - 1), vmalloc_end);
+ addr = min((*pnext)->va_start & ~(align - 1), end);
else
- addr = vmalloc_end;
+ addr = end;

while (*pprev && (*pprev)->va_end > addr) {
*pnext = *pprev;
@@ -2105,8 +2105,8 @@ struct vm_struct **pcpu_get_vm_areas(con
const size_t *sizes, int nr_vms,
size_t align, gfp_t gfp_mask)
{
- const unsigned long vmalloc_start = ALIGN(VMALLOC_START, align);
- const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
+ const unsigned long vstart = ALIGN(VMALLOC_START, align);
+ const unsigned long vend = VMALLOC_END & ~(align - 1);
struct vmap_area **vas, *prev, *next;
struct vm_struct **vms;
int area, area2, last_area, term_area;
@@ -2142,7 +2142,7 @@ struct vm_struct **pcpu_get_vm_areas(con
}
last_end = offsets[last_area] + sizes[last_area];

- if (vmalloc_end - vmalloc_start < last_end) {
+ if (vend - vstart < last_end) {
WARN_ON(true);
return NULL;
}
@@ -2167,7 +2167,7 @@ retry:
end = start + sizes[area];

if (!pvm_find_next_prev(vmap_area_pcpu_hole, &next, &prev)) {
- base = vmalloc_end - last_end;
+ base = vend - last_end;
goto found;
}
base = pvm_determine_end(&next, &prev, align) - end;
@@ -2180,7 +2180,7 @@ retry:
* base might have underflowed, add last_end before
* comparing.
*/
- if (base + last_end < vmalloc_start + last_end) {
+ if (base + last_end < vstart + last_end) {
spin_unlock(&vmap_area_lock);
if (!purged) {
purge_vmap_area_lazy();



2009-12-07 23:36:53

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

(cc linux-ia64)

On Mon, 07 Dec 2009 16:24:03 +0000
"Jan Beulich" <[email protected]> wrote:

> At least on ia64 vmalloc_end is a global variable that VMALLOC_END
> expands to. Hence having a local variable named vmalloc_end and
> initialized from VMALLOC_END won't work on such platforms. Rename
> these variables, and for consistency also rename vmalloc_start.
>

erk. So does 2.6.32's vmalloc() actually work correctly on ia64?

Perhaps vmalloc_end wasn't a well chosen name for an arch-specific
global variable.

arch/m68k/include/asm/pgtable_mm.h does the same thing. Did it break too?

> ---
> mm/vmalloc.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> --- linux-2.6.32/mm/vmalloc.c
> +++ 2.6.32-dont-use-vmalloc_end/mm/vmalloc.c
> @@ -2060,13 +2060,13 @@ static unsigned long pvm_determine_end(s
> struct vmap_area **pprev,
> unsigned long align)
> {
> - const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
> + const unsigned long end = VMALLOC_END & ~(align - 1);
> unsigned long addr;
>
> if (*pnext)
> - addr = min((*pnext)->va_start & ~(align - 1), vmalloc_end);
> + addr = min((*pnext)->va_start & ~(align - 1), end);
> else
> - addr = vmalloc_end;
> + addr = end;
>
> while (*pprev && (*pprev)->va_end > addr) {
> *pnext = *pprev;
> @@ -2105,8 +2105,8 @@ struct vm_struct **pcpu_get_vm_areas(con
> const size_t *sizes, int nr_vms,
> size_t align, gfp_t gfp_mask)
> {
> - const unsigned long vmalloc_start = ALIGN(VMALLOC_START, align);
> - const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
> + const unsigned long vstart = ALIGN(VMALLOC_START, align);
> + const unsigned long vend = VMALLOC_END & ~(align - 1);
> struct vmap_area **vas, *prev, *next;
> struct vm_struct **vms;
> int area, area2, last_area, term_area;
> @@ -2142,7 +2142,7 @@ struct vm_struct **pcpu_get_vm_areas(con
> }
> last_end = offsets[last_area] + sizes[last_area];
>
> - if (vmalloc_end - vmalloc_start < last_end) {
> + if (vend - vstart < last_end) {
> WARN_ON(true);
> return NULL;
> }
> @@ -2167,7 +2167,7 @@ retry:
> end = start + sizes[area];
>
> if (!pvm_find_next_prev(vmap_area_pcpu_hole, &next, &prev)) {
> - base = vmalloc_end - last_end;
> + base = vend - last_end;
> goto found;
> }
> base = pvm_determine_end(&next, &prev, align) - end;
> @@ -2180,7 +2180,7 @@ retry:
> * base might have underflowed, add last_end before
> * comparing.
> */
> - if (base + last_end < vmalloc_start + last_end) {
> + if (base + last_end < vstart + last_end) {
> spin_unlock(&vmap_area_lock);
> if (!purged) {
> purge_vmap_area_lazy();
>

2009-12-08 00:33:31

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On 12/08/2009 08:35 AM, Andrew Morton wrote:
> (cc linux-ia64)
>
> On Mon, 07 Dec 2009 16:24:03 +0000
> "Jan Beulich" <[email protected]> wrote:
>
>> At least on ia64 vmalloc_end is a global variable that VMALLOC_END
>> expands to. Hence having a local variable named vmalloc_end and
>> initialized from VMALLOC_END won't work on such platforms. Rename
>> these variables, and for consistency also rename vmalloc_start.
>>
>
> erk. So does 2.6.32's vmalloc() actually work correctly on ia64?
>
> Perhaps vmalloc_end wasn't a well chosen name for an arch-specific
> global variable.
>
> arch/m68k/include/asm/pgtable_mm.h does the same thing. Did it break too?

Hmmm... ISTR writing a patch updating ia64 so that it doesn't use that
macro. Looking it up.... Yeap, 126b3fcdecd350cad9700908d0ad845084e26a31
in percpu#for-next.

ia64: don't alias VMALLOC_END to vmalloc_end

If CONFIG_VIRTUAL_MEM_MAP is enabled, ia64 defines macro VMALLOC_END
as unsigned long variable vmalloc_end which is adjusted to prepare
room for vmemmap. This becomes probnlematic if a local variables
vmalloc_end is defined in some function (not very unlikely) and
VMALLOC_END is used in the function - the function thinks its
referencing the global VMALLOC_END value but would be referencing its
own local vmalloc_end variable.

There's no reason VMALLOC_END should be a macro. Just define it as an
unsigned long variable if CONFIG_VIRTUAL_MEM_MAP is set to avoid nasty
surprises.

Signed-off-by: Tejun Heo <[email protected]>
Acked-by: Tony Luck <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: linux-ia64 <[email protected]>
Cc: Christoph Lameter <[email protected]>

2.6.32 doesn't use new allocator on ia64 yet and the above commit will
be sent to Linus soon which will also enable new allocator.

Thanks.

--
tejun

2009-12-08 00:39:15

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

Hello,

On 12/08/2009 08:35 AM, Andrew Morton wrote:
> arch/m68k/include/asm/pgtable_mm.h does the same thing. Did it break too?

Oh... m64k does the same thing. I think the correct thing to do here
would be to convert arch code as in ia64. I think defining
VMALLOC_END to vmalloc_end is a bit error-prone. If it were defined
simply as vmalloc_end, it's unnoticeable by both the compiler and
developer.

Thanks.

--
tejun

2009-12-08 00:50:25

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Mon, Dec 07, 2009 at 03:35:52PM -0800, Andrew Morton wrote:
> erk. So does 2.6.32's vmalloc() actually work correctly on ia64?
>
> Perhaps vmalloc_end wasn't a well chosen name for an arch-specific
> global variable.

Can we enable -Wshadow now? Please?

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-12-08 01:05:06

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Mon, 7 Dec 2009 17:50:29 -0700
Matthew Wilcox <[email protected]> wrote:

> On Mon, Dec 07, 2009 at 03:35:52PM -0800, Andrew Morton wrote:
> > erk. So does 2.6.32's vmalloc() actually work correctly on ia64?
> >
> > Perhaps vmalloc_end wasn't a well chosen name for an arch-specific
> > global variable.
>
> Can we enable -Wshadow now? Please?
>

That would be good. How much mess would it make?

2009-12-08 06:56:19

by Tejun Heo

[permalink] [raw]
Subject: [PATCH] m68k: don't alias VMALLOC_END to vmalloc_end

On SUN3, m68k defines macro VMALLOC_END as unsigned long variable
vmalloc_end which is adjusted from mmu_emu_init(). This becomes
problematic if a local variables vmalloc_end is defined in some
function (not very unlikely) and VMALLOC_END is used in the function -
the function thinks its referencing the global VMALLOC_END value but
would be referencing its own local vmalloc_end variable.

There's no reason VMALLOC_END should be a macro. Just define it as an
unsigned long variable to avoid nasty surprises.

Signed-off-by: Tejun Heo <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Roman Zippel <[email protected]>
---
Okay, here it is. Compile tested. Geert, Roman, if you guys don't
object, I'd like to push it with the rest of percpu changes to Linus.
What do you think?

Thanks.

arch/m68k/include/asm/pgtable_mm.h | 3 +--
arch/m68k/sun3/mmu_emu.c | 8 ++++----
2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/include/asm/pgtable_mm.h b/arch/m68k/include/asm/pgtable_mm.h
index fe60e1a..0ea9f09 100644
--- a/arch/m68k/include/asm/pgtable_mm.h
+++ b/arch/m68k/include/asm/pgtable_mm.h
@@ -83,9 +83,8 @@
#define VMALLOC_START (((unsigned long) high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
#define VMALLOC_END KMAP_START
#else
-extern unsigned long vmalloc_end;
#define VMALLOC_START 0x0f800000
-#define VMALLOC_END vmalloc_end
+extern unsigned long VMALLOC_END;
#endif /* CONFIG_SUN3 */

/* zero page used for uninitialized stuff */
diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
index 3cd1939..25e2b14 100644
--- a/arch/m68k/sun3/mmu_emu.c
+++ b/arch/m68k/sun3/mmu_emu.c
@@ -45,8 +45,8 @@
** Globals
*/

-unsigned long vmalloc_end;
-EXPORT_SYMBOL(vmalloc_end);
+unsigned long VMALLOC_END;
+EXPORT_SYMBOL(VMALLOC_END);

unsigned long pmeg_vaddr[PMEGS_NUM];
unsigned char pmeg_alloc[PMEGS_NUM];
@@ -172,8 +172,8 @@ void mmu_emu_init(unsigned long bootmem_end)
#endif
// the lowest mapping here is the end of our
// vmalloc region
- if(!vmalloc_end)
- vmalloc_end = seg;
+ if (!VMALLOC_END)
+ VMALLOC_END = seg;

// mark the segmap alloc'd, and reserve any
// of the first 0xbff pages the hardware is

2009-12-08 08:23:38

by Jan Beulich

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

>>> Andrew Morton <[email protected]> 08.12.09 00:35 >>>
>(cc linux-ia64)
>
>On Mon, 07 Dec 2009 16:24:03 +0000
>"Jan Beulich" <[email protected]> wrote:
>
>> At least on ia64 vmalloc_end is a global variable that VMALLOC_END
>> expands to. Hence having a local variable named vmalloc_end and
>> initialized from VMALLOC_END won't work on such platforms. Rename
>> these variables, and for consistency also rename vmalloc_start.
>>
>
>erk. So does 2.6.32's vmalloc() actually work correctly on ia64?

According to Tejun the problem is just cosmetic (i.e. causes build
warnings), since the functions affected aren't being used (yet) on
ia64. So feel free to drop the patch again, given that he has a patch
queued to address the issue by renaming the arch variable.

I wonder though why that code is being built on ia64 at all if it's not
being used (i.e. why it doesn't depend on a CONFIG_*, HAVE_*, or
NEED_* manifest constant).

Jan

2009-12-08 08:28:50

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

Hello,

On 12/08/2009 05:23 PM, Jan Beulich wrote:
> According to Tejun the problem is just cosmetic (i.e. causes build
> warnings), since the functions affected aren't being used (yet) on
> ia64. So feel free to drop the patch again, given that he has a patch
> queued to address the issue by renaming the arch variable.
>
> I wonder though why that code is being built on ia64 at all if it's not
> being used (i.e. why it doesn't depend on a CONFIG_*, HAVE_*, or
> NEED_* manifest constant).

Hmm... it shouldn't be building. Can you please attach .config?

Thanks.

--
tejun

2009-12-08 08:39:52

by Jan Beulich

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

>>> Tejun Heo <[email protected]> 08.12.09 09:29 >>>
>Hmm... it shouldn't be building.

How can it not be building? It's in vmalloc.c (which must be built) and not
inside any conditional.

>Can you please attach .config?

Attached anyway.

Jan


Attachments:
(No filename) (241.00 B)
.config (47.30 kB)
Download all attachments

2009-12-08 08:57:16

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

Hello,

On 12/08/2009 05:39 PM, Jan Beulich wrote:
>>>> Tejun Heo <[email protected]> 08.12.09 09:29 >>>
>> Hmm... it shouldn't be building.
>
> How can it not be building? It's in vmalloc.c (which must be built) and not
> inside any conditional.

Ah... yes, right. Somehow I was thinking it lived in percpu.c. Sorry
about that. Probably the right thing to do is to wrap the function
inside CONFIG ifdef's. I'll prep a patch.

Thanks.

--
tejun

2009-12-08 09:08:04

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] m68k: don't alias VMALLOC_END to vmalloc_end

On Tue, Dec 8, 2009 at 07:57, Tejun Heo <[email protected]> wrote:
> On SUN3, m68k defines macro VMALLOC_END as unsigned long variable
> vmalloc_end which is adjusted from mmu_emu_init().  This becomes
> problematic if a local variables vmalloc_end is defined in some
> function (not very unlikely) and VMALLOC_END is used in the function -
> the function thinks its referencing the global VMALLOC_END value but
> would be referencing its own local vmalloc_end variable.
>
> There's no reason VMALLOC_END should be a macro.  Just define it as an
> unsigned long variable to avoid nasty surprises.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Roman Zippel <[email protected]>
> ---
> Okay, here it is.  Compile tested.  Geert, Roman, if you guys don't
> object, I'd like to push it with the rest of percpu changes to Linus.
> What do you think?

Fine for me, except that by convention allcaps is reserved for macros?

>
> Thanks.
>
>  arch/m68k/include/asm/pgtable_mm.h |    3 +--
>  arch/m68k/sun3/mmu_emu.c           |    8 ++++----
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/m68k/include/asm/pgtable_mm.h b/arch/m68k/include/asm/pgtable_mm.h
> index fe60e1a..0ea9f09 100644
> --- a/arch/m68k/include/asm/pgtable_mm.h
> +++ b/arch/m68k/include/asm/pgtable_mm.h
> @@ -83,9 +83,8 @@
>  #define VMALLOC_START (((unsigned long) high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
>  #define VMALLOC_END KMAP_START
>  #else
> -extern unsigned long vmalloc_end;
>  #define VMALLOC_START 0x0f800000
> -#define VMALLOC_END vmalloc_end
> +extern unsigned long VMALLOC_END;
>  #endif /* CONFIG_SUN3 */
>
>  /* zero page used for uninitialized stuff */
> diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
> index 3cd1939..25e2b14 100644
> --- a/arch/m68k/sun3/mmu_emu.c
> +++ b/arch/m68k/sun3/mmu_emu.c
> @@ -45,8 +45,8 @@
>  ** Globals
>  */
>
> -unsigned long vmalloc_end;
> -EXPORT_SYMBOL(vmalloc_end);
> +unsigned long VMALLOC_END;
> +EXPORT_SYMBOL(VMALLOC_END);
>
>  unsigned long pmeg_vaddr[PMEGS_NUM];
>  unsigned char pmeg_alloc[PMEGS_NUM];
> @@ -172,8 +172,8 @@ void mmu_emu_init(unsigned long bootmem_end)
>  #endif
>                        // the lowest mapping here is the end of our
>                        // vmalloc region
> -                       if(!vmalloc_end)
> -                               vmalloc_end = seg;
> +                       if (!VMALLOC_END)
> +                               VMALLOC_END = seg;
>
>                        // mark the segmap alloc'd, and reserve any
>                        // of the first 0xbff pages the hardware is
>



--
Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2009-12-08 09:11:47

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Tue, Dec 8, 2009 at 00:35, Andrew Morton <[email protected]> wrote:
> (cc linux-ia64)
>
> On Mon, 07 Dec 2009 16:24:03 +0000
> "Jan Beulich" <[email protected]> wrote:
>
>> At least on ia64 vmalloc_end is a global variable that VMALLOC_END
>> expands to. Hence having a local variable named vmalloc_end and
>> initialized from VMALLOC_END won't work on such platforms. Rename
>> these variables, and for consistency also rename vmalloc_start.
>>
>
> erk.  So does 2.6.32's vmalloc() actually work correctly on ia64?
>
> Perhaps vmalloc_end wasn't a well chosen name for an arch-specific
> global variable.
>
> arch/m68k/include/asm/pgtable_mm.h does the same thing.  Did it break too?

Rename to m68k_vmalloc_{end,start}?

Hmm, sounds better than introducing allcaps variables...

>
>> ---
>>  mm/vmalloc.c |   16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> --- linux-2.6.32/mm/vmalloc.c
>> +++ 2.6.32-dont-use-vmalloc_end/mm/vmalloc.c
>> @@ -2060,13 +2060,13 @@ static unsigned long pvm_determine_end(s
>>                                      struct vmap_area **pprev,
>>                                      unsigned long align)
>>  {
>> -     const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
>> +     const unsigned long end = VMALLOC_END & ~(align - 1);
>>       unsigned long addr;
>>
>>       if (*pnext)
>> -             addr = min((*pnext)->va_start & ~(align - 1), vmalloc_end);
>> +             addr = min((*pnext)->va_start & ~(align - 1), end);
>>       else
>> -             addr = vmalloc_end;
>> +             addr = end;
>>
>>       while (*pprev && (*pprev)->va_end > addr) {
>>               *pnext = *pprev;
>> @@ -2105,8 +2105,8 @@ struct vm_struct **pcpu_get_vm_areas(con
>>                                    const size_t *sizes, int nr_vms,
>>                                    size_t align, gfp_t gfp_mask)
>>  {
>> -     const unsigned long vmalloc_start = ALIGN(VMALLOC_START, align);
>> -     const unsigned long vmalloc_end = VMALLOC_END & ~(align - 1);
>> +     const unsigned long vstart = ALIGN(VMALLOC_START, align);
>> +     const unsigned long vend = VMALLOC_END & ~(align - 1);
>>       struct vmap_area **vas, *prev, *next;
>>       struct vm_struct **vms;
>>       int area, area2, last_area, term_area;
>> @@ -2142,7 +2142,7 @@ struct vm_struct **pcpu_get_vm_areas(con
>>       }
>>       last_end = offsets[last_area] + sizes[last_area];
>>
>> -     if (vmalloc_end - vmalloc_start < last_end) {
>> +     if (vend - vstart < last_end) {
>>               WARN_ON(true);
>>               return NULL;
>>       }
>> @@ -2167,7 +2167,7 @@ retry:
>>       end = start + sizes[area];
>>
>>       if (!pvm_find_next_prev(vmap_area_pcpu_hole, &next, &prev)) {
>> -             base = vmalloc_end - last_end;
>> +             base = vend - last_end;
>>               goto found;
>>       }
>>       base = pvm_determine_end(&next, &prev, align) - end;
>> @@ -2180,7 +2180,7 @@ retry:
>>                * base might have underflowed, add last_end before
>>                * comparing.
>>                */
>> -             if (base + last_end < vmalloc_start + last_end) {
>> +             if (base + last_end < vstart + last_end) {
>>                       spin_unlock(&vmap_area_lock);
>>                       if (!purged) {
>>                               purge_vmap_area_lazy();
>>
>



--
Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2009-12-08 09:24:40

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

Hello,

Geert Uytterhoeven wrote:
> Rename to m68k_vmalloc_{end,start}?
> Hmm, sounds better than introducing allcaps variables...

We definitely can do that too but I don't know. It still has the risk
of aliasing behind both the developer's and compiler's back. Aliasing
a macro directly to a macro just isn't a very good idea. We can
definitely make it more complex - make the prefix more unlikely,
prevent it from being used as lvalue and so on but given that it's a
pretty special case anyway, I think all caps variable isn't too bad
here.

But you're the maintainer, if you prefer the following version, I'll
go forward with it.

Thanks.

diff --git a/arch/m68k/include/asm/pgtable_mm.h b/arch/m68k/include/asm/pgtable_mm.h
index fe60e1a..2db057f 100644
--- a/arch/m68k/include/asm/pgtable_mm.h
+++ b/arch/m68k/include/asm/pgtable_mm.h
@@ -83,9 +83,9 @@
#define VMALLOC_START (((unsigned long) high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
#define VMALLOC_END KMAP_START
#else
-extern unsigned long vmalloc_end;
#define VMALLOC_START 0x0f800000
-#define VMALLOC_END vmalloc_end
+extern unsigned long m68k_vmalloc_end;
+#define VMALLOC_END m68k_vmalloc_end
#endif /* CONFIG_SUN3 */

/* zero page used for uninitialized stuff */
diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
index 3cd1939..94f81ec 100644
--- a/arch/m68k/sun3/mmu_emu.c
+++ b/arch/m68k/sun3/mmu_emu.c
@@ -45,8 +45,8 @@
** Globals
*/

-unsigned long vmalloc_end;
-EXPORT_SYMBOL(vmalloc_end);
+unsigned long m68k_vmalloc_end;
+EXPORT_SYMBOL(m68k_vmalloc_end);

unsigned long pmeg_vaddr[PMEGS_NUM];
unsigned char pmeg_alloc[PMEGS_NUM];
@@ -172,8 +172,8 @@ void mmu_emu_init(unsigned long bootmem_end)
#endif
// the lowest mapping here is the end of our
// vmalloc region
- if(!vmalloc_end)
- vmalloc_end = seg;
+ if (!m68k_vmalloc_end)
+ m68k_vmalloc_end = seg;

// mark the segmap alloc'd, and reserve any
// of the first 0xbff pages the hardware is

--
tejun

2009-12-09 08:46:34

by Tejun Heo

[permalink] [raw]
Subject: [PATCH] m68k: rename global variable vmalloc_end to m68k_vmalloc_end

On SUN3, m68k defines macro VMALLOC_END as unsigned long variable
vmalloc_end which is adjusted from mmu_emu_init(). This becomes
problematic if a local variables vmalloc_end is defined in some
function (not very unlikely) and VMALLOC_END is used in the function -
the function thinks its referencing the global VMALLOC_END value but
would be referencing its own local vmalloc_end variable.

Rename the global variable to m68k_vmlloc_end which is much less
likely to be used as local variable name.

Signed-off-by: Tejun Heo <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Roman Zippel <[email protected]>
---
This patch has been queued to percpu#for-linus. Thanks.

arch/m68k/include/asm/pgtable_mm.h | 4 ++--
arch/m68k/sun3/mmu_emu.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/include/asm/pgtable_mm.h b/arch/m68k/include/asm/pgtable_mm.h
index fe60e1a..aca0e28 100644
--- a/arch/m68k/include/asm/pgtable_mm.h
+++ b/arch/m68k/include/asm/pgtable_mm.h
@@ -83,9 +83,9 @@
#define VMALLOC_START (((unsigned long) high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
#define VMALLOC_END KMAP_START
#else
-extern unsigned long vmalloc_end;
+extern unsigned long m68k_vmalloc_end;
#define VMALLOC_START 0x0f800000
-#define VMALLOC_END vmalloc_end
+#define VMALLOC_END m68k_vmalloc_end
#endif /* CONFIG_SUN3 */

/* zero page used for uninitialized stuff */
diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
index 3cd1939..94f81ec 100644
--- a/arch/m68k/sun3/mmu_emu.c
+++ b/arch/m68k/sun3/mmu_emu.c
@@ -45,8 +45,8 @@
** Globals
*/

-unsigned long vmalloc_end;
-EXPORT_SYMBOL(vmalloc_end);
+unsigned long m68k_vmalloc_end;
+EXPORT_SYMBOL(m68k_vmalloc_end);

unsigned long pmeg_vaddr[PMEGS_NUM];
unsigned char pmeg_alloc[PMEGS_NUM];
@@ -172,8 +172,8 @@ void mmu_emu_init(unsigned long bootmem_end)
#endif
// the lowest mapping here is the end of our
// vmalloc region
- if(!vmalloc_end)
- vmalloc_end = seg;
+ if (!m68k_vmalloc_end)
+ m68k_vmalloc_end = seg;

// mark the segmap alloc'd, and reserve any
// of the first 0xbff pages the hardware is
--
1.6.4.2

2009-12-09 17:33:13

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Tue, 8 Dec 2009, Jan Beulich wrote:

> According to Tejun the problem is just cosmetic (i.e. causes build
> warnings), since the functions affected aren't being used (yet) on
> ia64. So feel free to drop the patch again, given that he has a patch
> queued to address the issue by renaming the arch variable.

I thought the new code must be used in order for the new percpu allocator
to work? Or is this referring to other code?

> I wonder though why that code is being built on ia64 at all if it's not
> being used (i.e. why it doesn't depend on a CONFIG_*, HAVE_*, or
> NEED_* manifest constant).

Tony: Can you confirm that the new percpu stuff works on IA64? (Or is
there nobody left to care?)

2009-12-09 17:48:39

by Luck, Tony

[permalink] [raw]
Subject: RE: [PATCH] mm/vmalloc: don't use vmalloc_end

> Tony: Can you confirm that the new percpu stuff works on IA64.

If all the pieces are in (either Linus tree, or linux-next) then
it is working (well these both still build & boot on my test systems).

-Tony

2009-12-09 18:10:47

by Mike Travis

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end



Christoph Lameter wrote:
> On Tue, 8 Dec 2009, Jan Beulich wrote:
>
>> According to Tejun the problem is just cosmetic (i.e. causes build
>> warnings), since the functions affected aren't being used (yet) on
>> ia64. So feel free to drop the patch again, given that he has a patch
>> queued to address the issue by renaming the arch variable.
>
> I thought the new code must be used in order for the new percpu allocator
> to work? Or is this referring to other code?
>
>> I wonder though why that code is being built on ia64 at all if it's not
>> being used (i.e. why it doesn't depend on a CONFIG_*, HAVE_*, or
>> NEED_* manifest constant).
>
> Tony: Can you confirm that the new percpu stuff works on IA64? (Or is
> there nobody left to care?)

Christoph, I have access to a 640p system for a couple more weeks if
there's anything you'd like me to check out.

Thanks,
Mike

2009-12-09 18:24:29

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Wed, 9 Dec 2009, Mike Travis wrote:

> > Tony: Can you confirm that the new percpu stuff works on IA64? (Or is
> > there nobody left to care?)
>
> Christoph, I have access to a 640p system for a couple more weeks if
> there's anything you'd like me to check out.

Boot with 2.6.32 and see if the per cpu allocator works. Check if there
are any changes to memory consumption. Create a few thousand virtual
ethernet devices and see if the system keels over.

It may also be good to run some scheduler test. Compare AIM9 of latest
SLES with 2.6.32. Concurrent page fault test? Then a performance test with
lots of concurrency but the usual stuff wont work since HPC apps usually
pin.

Run latencytest (available in the lldiag package) from
kernel.org/pub/linux/kernel/people/christoph/lldiag and see how the
disturbances by the OS are changed.

2009-12-09 18:25:52

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Wed, 9 Dec 2009, Christoph Lameter wrote:

> Boot with 2.6.32 and see if the per cpu allocator works. Check if there
> are any changes to memory consumption. Create a few thousand virtual
> ethernet devices and see if the system keels over.

Argh. You have to wait till 2.6.33-rc1 I believe to get the conversion of
the SNMP MIBs for the new per cpu allocator.

2009-12-09 18:37:19

by Mike Travis

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end



Christoph Lameter wrote:
> On Wed, 9 Dec 2009, Mike Travis wrote:
>
>>> Tony: Can you confirm that the new percpu stuff works on IA64? (Or is
>>> there nobody left to care?)
>> Christoph, I have access to a 640p system for a couple more weeks if
>> there's anything you'd like me to check out.
>
> Boot with 2.6.32 and see if the per cpu allocator works. Check if there
> are any changes to memory consumption. Create a few thousand virtual
> ethernet devices and see if the system keels over.

Any advice on how to go about the above would be helpful... ;-)

>
> It may also be good to run some scheduler test. Compare AIM9 of latest
> SLES with 2.6.32. Concurrent page fault test? Then a performance test with
> lots of concurrency but the usual stuff wont work since HPC apps usually
> pin.

I'm doing some aim7/9 comparisons right now between SPARSE and DISCONTIG
memory configs using sles11 + 2.6.32. Which other benchmarks would you
recommend for the other tests?

>
> Run latencytest (available in the lldiag package) from
> kernel.org/pub/linux/kernel/people/christoph/lldiag and see how the
> disturbances by the OS are changed.

I'll put that on the list.

2009-12-09 18:47:56

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] mm/vmalloc: don't use vmalloc_end

On Wed, 9 Dec 2009, Mike Travis wrote:

> > Boot with 2.6.32 and see if the per cpu allocator works. Check if there
> > are any changes to memory consumption. Create a few thousand virtual
> > ethernet devices and see if the system keels over.
>
> Any advice on how to go about the above would be helpful... ;-)

I believe you can create an additional alias device with

ifconfig eth0:<N>

or so.

> I'm doing some aim7/9 comparisons right now between SPARSE and DISCONTIG
> memory configs using sles11 + 2.6.32. Which other benchmarks would you
> recommend for the other tests?

See f.e. http://kernel-perf.sourceforge.net/about_tests.php

lmbench?

2009-12-09 23:41:59

by Tejun Heo

[permalink] [raw]
Subject: [PATCH -stable] vmalloc: conditionalize build of pcpu_get_vm_areas()

pcpu_get_vm_areas() is used only when dynamic percpu allocator is used
by the architecture. In 2.6.32, ia64 doesn't use dynamic percpu
allocator and has a macro which makes pcpu_get_vm_areas() buggy via
local/global variable aliasing and triggers compile warning.

The problem is fixed in upstream and ia64 uses dynamic percpu
allocators, so the only left issue is inclusion of unnecessary code
and compile warning on ia64 on 2.6.32.

Don't build pcpu_get_vm_areas() if legacy percpu allocator is in use.

Signed-off-by: Tejun Heo <[email protected]>
Reported-by: Jan Beulich <[email protected]>
Cc: [email protected]
---
Please note that this commit won't appear on upstream.

Thanks.

include/linux/vmalloc.h | 2 ++
mm/vmalloc.c | 2 ++
2 files changed, 4 insertions(+)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 227c2a5..3c123c3 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -115,9 +115,11 @@ extern rwlock_t vmlist_lock;
extern struct vm_struct *vmlist;
extern __init void vm_area_register_early(struct vm_struct *vm, size_t align);

+#ifndef CONFIG_HAVE_LEGACY_PER_CPU_AREA
struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets,
const size_t *sizes, int nr_vms,
size_t align, gfp_t gfp_mask);
+#endif

void pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms);

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 0f551a4..7758726 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1993,6 +1993,7 @@ void free_vm_area(struct vm_struct *area)
}
EXPORT_SYMBOL_GPL(free_vm_area);

+#ifndef CONFIG_HAVE_LEGACY_PER_CPU_AREA
static struct vmap_area *node_to_va(struct rb_node *n)
{
return n ? rb_entry(n, struct vmap_area, rb_node) : NULL;
@@ -2257,6 +2258,7 @@ err_free:
kfree(vms);
return NULL;
}
+#endif

/**
* pcpu_free_vm_areas - free vmalloc areas for percpu allocator

2009-12-16 23:13:17

by Greg KH

[permalink] [raw]
Subject: Re: [stable] [PATCH -stable] vmalloc: conditionalize build of pcpu_get_vm_areas()

On Thu, Dec 10, 2009 at 08:43:16AM +0900, Tejun Heo wrote:
> pcpu_get_vm_areas() is used only when dynamic percpu allocator is used
> by the architecture. In 2.6.32, ia64 doesn't use dynamic percpu
> allocator and has a macro which makes pcpu_get_vm_areas() buggy via
> local/global variable aliasing and triggers compile warning.
>
> The problem is fixed in upstream and ia64 uses dynamic percpu
> allocators, so the only left issue is inclusion of unnecessary code
> and compile warning on ia64 on 2.6.32.
>
> Don't build pcpu_get_vm_areas() if legacy percpu allocator is in use.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Reported-by: Jan Beulich <[email protected]>
> Cc: [email protected]
> ---
> Please note that this commit won't appear on upstream.

So this is only needed for the .32 kernel stable tree? Not .31? And
it's not upstream as it was solved differently there?

thanks,

greg k-h

2009-12-16 23:59:38

by Tejun Heo

[permalink] [raw]
Subject: Re: [stable] [PATCH -stable] vmalloc: conditionalize build of pcpu_get_vm_areas()

Hello, Greg.

On 12/17/2009 08:12 AM, Greg KH wrote:
>> Please note that this commit won't appear on upstream.
>
> So this is only needed for the .32 kernel stable tree? Not .31? And
> it's not upstream as it was solved differently there?

Yeap, .32 is the only affected one and in the upstream the problem is
solved way back and ia64 is already using the new dynamic allocator.

Thanks.

--
tejun

2009-12-17 00:03:26

by Greg KH

[permalink] [raw]
Subject: Re: [stable] [PATCH -stable] vmalloc: conditionalize build of pcpu_get_vm_areas()

On Thu, Dec 17, 2009 at 09:01:52AM +0900, Tejun Heo wrote:
> Hello, Greg.
>
> On 12/17/2009 08:12 AM, Greg KH wrote:
> >> Please note that this commit won't appear on upstream.
> >
> > So this is only needed for the .32 kernel stable tree? Not .31? And
> > it's not upstream as it was solved differently there?
>
> Yeap, .32 is the only affected one and in the upstream the problem is
> solved way back and ia64 is already using the new dynamic allocator.

Great, I've queued this up now, thanks for letting me know.

greg k-h