Re: [PATCH] discontigmem support for ia32 NUMA box against 2.4.19pre8

[Christoph Hellwig]

On Wed, May 08, 2002 at 05:19:33PM -0700, Patricia Gaughen wrote:
> 
> Please consider this patch for inclusion into the next 2.4 release.
> Sent this patch out last week as an RFC.  I've resolved the comments 
> from that post, mostly regarding config options.  

I think this patch, unlike the two previous cleanups, still needs some
polishing.

> http://prdownloads.sourceforge.net/lse/meminit-2.4.19pre8.patch
> http://prdownloads.sourceforge.net/lse/setup_arch-2.4.19pre8.patch
> The discontigmem patch is available at:
> 
> http://prdownloads.sourceforge.net/lse/x86_discontigmem-2.4.19pre8.patch

Urgg, sourceforge seems to have turned these nice links into some download
selector crap.  I think it's really time to stop using it as it gets worse
all time..
Any chance you could post links directly to one of the mirrors next time?

>  if [ "$CONFIG_SMP" = "y" -a "$CONFIG_X86_CMPXCHG" = "y" ]; then
> --- linux-2.4.19pre8-cleanup/arch/i386/kernel/Makefile	Fri Nov  9 14:21:21 2001
> +++ linux-2.4.19pre8-multi/arch/i386/kernel/Makefile	Wed May  8 11:09:21 2002
> @@ -40,5 +40,7 @@
>  obj-$(CONFIG_X86_LOCAL_APIC)	+= mpparse.o apic.o nmi.o
>  obj-$(CONFIG_X86_IO_APIC)	+= io_apic.o acpitable.o
>  obj-$(CONFIG_X86_VISWS_APIC)	+= visws_apic.o
> +obj-$(CONFIG_X86_NUMAQ)		+= core_ibmnumaq.o

The core_ibmnumaq.* naming looks strange to me.  It seems derived from the
alpha naming where we support many different cores.  I think numaq.c
would fit much better in the naming of the other files in arch/i386/kernel/.
Please also note that the ifdef around the whole file body in core_ibmnumaq.c
is superflous as we already have the kbuild conditional.

> +obj-$(CONFIG_DISCONTIGMEM)	+= numa.o

Okay, this comes to the next issue, you seem to use CONFIG_DISCONTIGMEM
and CONFIG_X86_DISCONTIGMEM interchangable in arch/i386/* and numa.c in
fact has a big #ifdef CONFIG_X86_DISCONTIGMEM around all of the code.
AFAICS CONFIG_X86_DISCONTIGMEM is really the selector for the bootmem
workarounds and I think it shouldn't be used anywhere else, or even better
replaced by and HAVE_ARCH_BOOTMEM_NODE #define in asm/pgtable.h.

Also why is this file named numa.c and depends on CONFIG_DISCONTIGMEM?
Either it is NUMA-specific and depends on CONFIG_NUMA or it is dicontig
code and should be named discontig.c or something like that.  This file
is completly about memory managment, btw so I wonder why it isn't placed
in arch/i386/mm/..

> -static inline int page_is_ram (unsigned long pagenr)
> +inline int page_is_ram (unsigned long pagenr)

What about makeing this a static inline in one of the asm/ headers?
This way the external users also have it inline and I know besides
NUMAQ at least the LKCD people also want it.

> --- linux-2.4.19pre8-cleanup/include/asm-i386/mmzone.h	Wed Dec 31 16:00:00 1969
> +++ linux-2.4.19pre8-multi/include/asm-i386/mmzone.h	Wed May  8 11:09:21 2002
> @@ -0,0 +1,103 @@
> +/*
> + * Written by Pat Gaughen ([e-mail]) Mar 2002
> + *
> + */
> +
> +#ifndef _ASM_MMZONE_H_
> +#define _ASM_MMZONE_H_
> +
> +#ifdef CONFIG_DISCONTIGMEM
<snip>
> +#endif /* CONFIG_X86_DISCONTIGMEM */

hmm?