[PATCH] i386: srat and numaq cleanup

[PATCH] i386: srat and numaq cleanup

[Magnus Damm]

Cleanup the i386 NUMA code by creating inline no-op functions for
get_memcfg_numaq/srat() and get_zholes_size_numaq/srat().

Signed-off-by: Magnus Damm <magnus@valinux.co.jp>
---

Applies on top of linux-2.6.14-rc2-git8-mhp1

 arch/i386/kernel/srat.c   |   10 ++++++++--
 include/asm-i386/mmzone.h |   26 +++++++++++++++++---------
 include/asm-i386/numaq.h  |   10 ++++++++--
 include/asm-i386/srat.h   |   15 ++++++++++-----
 4 files changed, 43 insertions(+), 18 deletions(-)

--- from-0002/arch/i386/kernel/srat.c
+++ to-work/arch/i386/kernel/srat.c	2005-10-05 16:49:00.000000000 +0900
@@ -56,6 +56,7 @@ struct node_memory_chunk_s {
 static struct node_memory_chunk_s node_memory_chunk[MAXCHUNKS];
 
 static int num_memory_chunks;		/* total number of memory chunks */
+static int has_srat;
 static int zholes_size_init;
 static unsigned long zholes_size[MAX_NUMNODES * MAX_NR_ZONES];
 
@@ -317,7 +318,7 @@ out_fail:
 	return 0;
 }
 
-int __init get_memcfg_from_srat(void)
+int __init get_memcfg_srat(void)
 {
 	struct acpi_table_header *header = NULL;
 	struct acpi_table_rsdp *rsdp = NULL;
@@ -403,6 +404,8 @@ int __init get_memcfg_from_srat(void)
 			continue;
 
 		/* we've found the srat table. don't need to look at any more tables */
+		has_srat = 1;
+
 		return acpi20_parse_srat((struct acpi_table_srat *)header);
 	}
 out_err:
@@ -449,8 +452,11 @@ static void __init get_zholes_init(void)
 	}
 }
 
-unsigned long * __init get_zholes_size(int nid)
+unsigned long * __init get_zholes_size_srat(int nid)
 {
+	if (!has_srat)
+		return NULL;
+
 	if (!zholes_size_init) {
 		zholes_size_init++;
 		get_zholes_init();
--- from-0041/include/asm-i386/mmzone.h
+++ to-work/include/asm-i386/mmzone.h	2005-10-05 16:49:00.000000000 +0900
@@ -12,11 +12,8 @@
 extern struct pglist_data *node_data[];
 #define NODE_DATA(nid)	(node_data[nid])
 
-#ifdef CONFIG_X86_NUMAQ
-	#include <asm/numaq.h>
-#else	/* summit or generic arch */
-	#include <asm/srat.h>
-#endif
+#include <asm/numaq.h>
+#include <asm/srat.h>
 
 extern int get_memcfg_numa_flat(void );
 /*
@@ -26,17 +23,28 @@ extern int get_memcfg_numa_flat(void );
  */
 static inline void get_memcfg_numa(void)
 {
-#ifdef CONFIG_X86_NUMAQ
 	if (get_memcfg_numaq())
 		return;
-#elif defined(CONFIG_ACPI_SRAT)
-	if (get_memcfg_from_srat())
+
+	if (get_memcfg_srat())
 		return;
-#endif
 
 	get_memcfg_numa_flat();
 }
 
+static inline unsigned long *get_zholes_size(int nid)
+{
+	unsigned long *ret;
+
+	if ((ret = get_zholes_size_numaq(nid)))
+		return ret;
+
+	if ((ret = get_zholes_size_srat(nid)))
+		return ret;
+
+	return NULL;
+}
+
 extern int early_pfn_to_nid(unsigned long pfn);
 extern void __init remap_numa_kva(void);
 extern unsigned long calculate_numa_remap_pages(void);
--- from-0001/include/asm-i386/numaq.h
+++ to-work/include/asm-i386/numaq.h	2005-10-05 16:49:00.000000000 +0900
@@ -155,10 +155,16 @@ struct sys_cfg_data {
         struct	eachquadmem eq[MAX_NUMNODES];	/* indexed by quad id */
 };
 
-static inline unsigned long *get_zholes_size(int nid)
+#else /* CONFIG_X86_NUMAQ */
+
+static inline int get_memcfg_numaq(void) { return 0; }
+
+#endif /* CONFIG_X86_NUMAQ */
+
+static inline unsigned long *get_zholes_size_numaq(int nid)
 {
 	return NULL;
 }
-#endif /* CONFIG_X86_NUMAQ */
+
 #endif /* NUMAQ_H */
 
--- from-0001/include/asm-i386/srat.h
+++ to-work/include/asm-i386/srat.h	2005-10-05 16:49:00.000000000 +0900
@@ -27,11 +27,16 @@
 #ifndef _ASM_SRAT_H_
 #define _ASM_SRAT_H_
 
-#ifndef CONFIG_ACPI_SRAT
-#error CONFIG_ACPI_SRAT not defined, and srat.h header has been included
-#endif
+#ifdef CONFIG_ACPI_SRAT
 
-extern int get_memcfg_from_srat(void);
-extern unsigned long *get_zholes_size(int);
+extern int get_memcfg_srat(void);
+extern unsigned long *get_zholes_size_srat(int);
+
+#else /* CONFIG_ACPI_SRAT */
+
+static inline int get_memcfg_srat(void) { return 0; }
+static inline unsigned long *get_zholes_size_srat(int nid) { return NULL; }
+
+#endif /* CONFIG_ACPI_SRAT */
 
 #endif /* _ASM_SRAT_H_ */

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] i386: srat and numaq cleanup

[Dave Hansen]

On Wed, 2005-10-05 at 17:39 +0900, Magnus Damm wrote:
> Cleanup the i386 NUMA code by creating inline no-op functions for
> get_memcfg_numaq/srat() and get_zholes_size_numaq/srat().

>  arch/i386/kernel/srat.c   |   10 ++++++++--
>  include/asm-i386/mmzone.h |   26 +++++++++++++++++---------
>  include/asm-i386/numaq.h  |   10 ++++++++--
>  include/asm-i386/srat.h   |   15 ++++++++++-----
>  4 files changed, 43 insertions(+), 18 deletions(-)

I'm highly suspicious of any "cleanup" that adds more code than it
deletes.  What does this clean up?

This patch is a little bit confused.  It makes the
get_zholes_size_srat() always safe to call at runtime.  However, it
still creates a compile-time stub version of it as well.  

In addition, you already have the srat.c-local zholes_size_init, but you
still add the has_srat variable.  Seems a bit superfluous.

Calling get_zholes_size_numaq() at runtime is unnecessary.  The NUMA-Q
is not supported with the ARCH_GENERIC code.

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] i386: srat and numaq cleanup

[Magnus Damm]

On 10/6/05, Dave Hansen <haveblue@us.ibm.com> wrote:
> On Wed, 2005-10-05 at 17:39 +0900, Magnus Damm wrote:
> > Cleanup the i386 NUMA code by creating inline no-op functions for
> > get_memcfg_numaq/srat() and get_zholes_size_numaq/srat().
>
> >  arch/i386/kernel/srat.c   |   10 ++++++++--
> >  include/asm-i386/mmzone.h |   26 +++++++++++++++++---------
> >  include/asm-i386/numaq.h  |   10 ++++++++--
> >  include/asm-i386/srat.h   |   15 ++++++++++-----
> >  4 files changed, 43 insertions(+), 18 deletions(-)
>
> I'm highly suspicious of any "cleanup" that adds more code than it
> deletes.  What does this clean up?

Hehe, I realized that it added code when I generated the diffstat, but
I believe that the code gets cleaner.

The patch removes #ifdefs from get_memcfg_numa() and introduces an
inline get_zholes_size(). The #ifdefs are moved down one level to the
files srat.h and numaq.h and empty inline functions are added. These
empty inline function are probably the reason for the added lines.

> This patch is a little bit confused.  It makes the
> get_zholes_size_srat() always safe to call at runtime.  However, it
> still creates a compile-time stub version of it as well.

Without this patch get_zholes_size() is defined in either srat.c or
numaq.h. With this patch get_zholes_size() is turned into an inline
function like get_memcfg_numa(), and this function calls all
machine-specific implementations.

This new behavior requires that the machine-specific code keeps track
if it has been initialized or not, and only lets get_zholes_size_xxx()
return non-NULL if initialized.

> In addition, you already have the srat.c-local zholes_size_init, but you
> still add the has_srat variable.  Seems a bit superfluous.

Nah, zholes_size_init is used to call get_zholes_init() just once. 
has_srat is used to determine if get_memcfg_numa() did succeed.

> Calling get_zholes_size_numaq() at runtime is unnecessary.  The NUMA-Q
> is not supported with the ARCH_GENERIC code.

Yes. But does this patch do anything that makes that harder?

Another side effect of the patch is that get_zholes_size() always gets
defined, regardless of if numaq or srat is enabled or not. That is
useful for the NUMA emulation code.

Thanks,

/ magnus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] i386: srat and numaq cleanup

[Dave Hansen]

On Thu, 2005-10-06 at 19:29 +0900, Magnus Damm wrote:
> On 10/6/05, Dave Hansen <haveblue@us.ibm.com> wrote:
> > I'm highly suspicious of any "cleanup" that adds more code than it
> > deletes.  What does this clean up?
>
> The patch removes #ifdefs from get_memcfg_numa() and introduces an
> inline get_zholes_size(). The #ifdefs are moved down one level to the
> files srat.h and numaq.h and empty inline functions are added. These
> empty inline function are probably the reason for the added lines.

It does remove two #ifdefs, but it adds two #else blocks in other
places.

I also noticed that acpi20_parse_srat() can fail.  So, has_srat may
belong in that function, not in get_memcfg_from_srat()

Why ever have this block?

> +       if ((ret = get_zholes_size_numaq(nid)))
> +               return ret;

get_zholes_size_numaq() is *ALWAYS* empty/false, right?  There's no need
to have a stub for it.

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] i386: srat and numaq cleanup

[Magnus Damm]

On 10/6/05, Dave Hansen <haveblue@us.ibm.com> wrote:
> On Thu, 2005-10-06 at 19:29 +0900, Magnus Damm wrote:
> > On 10/6/05, Dave Hansen <haveblue@us.ibm.com> wrote:
> > > I'm highly suspicious of any "cleanup" that adds more code than it
> > > deletes.  What does this clean up?
> >
> > The patch removes #ifdefs from get_memcfg_numa() and introduces an
> > inline get_zholes_size(). The #ifdefs are moved down one level to the
> > files srat.h and numaq.h and empty inline functions are added. These
> > empty inline function are probably the reason for the added lines.
>
> It does remove two #ifdefs, but it adds two #else blocks in other
> places.
>
> I also noticed that acpi20_parse_srat() can fail.  So, has_srat may
> belong in that function, not in get_memcfg_from_srat()

Yes, that is better.

> Why ever have this block?
>
> > +       if ((ret = get_zholes_size_numaq(nid)))
> > +               return ret;
>
> get_zholes_size_numaq() is *ALWAYS* empty/false, right?  There's no need
> to have a stub for it.

That is correct. I just kept it there to make the srat and numaq code
more similar, but I'd be happy to remove it. If you still consider
this as a cleanup, please let me know and I will generate a new patch.

Thanks,

/ magnus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] i386: srat and numaq cleanup

[Dave Hansen]

On Fri, 2005-10-07 at 16:54 +0900, Magnus Damm wrote:
> 
> > get_zholes_size_numaq() is *ALWAYS* empty/false, right?  There's no
> need
> > to have a stub for it.
> 
> That is correct. I just kept it there to make the srat and numaq code
> more similar, but I'd be happy to remove it. If you still consider
> this as a cleanup, please let me know and I will generate a new patch.

I'd pull it.  No need to add new code just for parity.

Thanks,

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>