Re: [PATCH 4/10] hot-add-mem x86_64: Enable SPARSEMEM in srat.c

[Dave Hansen <haveblue@us.ibm.com>]

On Fri, 2006-08-04 at 07:14 -0600, Keith Mannthey wrote:
> From: Keith Mannthey <kmannth@us.ibm.com>
> 
>  Enable x86_64 srat.c to share code between both reserve and sparsemem based add memory
> paths.  Both paths need the hot-add area node locality infomration (nodes_add).  This 
> code refactors the code path to allow this. 

I won't respond to all of these, but the set looks pretty darn sane.  

> Signed-off-by: Keith Mannthey<kmannth@us.ibm.com>
> ---
>  srat.c |   51 +++++++++++++++++++++++++++++----------------------
>  1 files changed, 29 insertions(+), 22 deletions(-)
> 
> Files orig/arch/x86_64/mm/.srat.c.swp and current/arch/x86_64/mm/.srat.c.swp differ
> diff -urN orig/arch/x86_64/mm/srat.c current/arch/x86_64/mm/srat.c
> --- orig/arch/x86_64/mm/srat.c	2006-08-04 00:41:17.000000000 -0400
> +++ current/arch/x86_64/mm/srat.c	2006-08-04 01:02:25.000000000 -0400
> @@ -21,12 +21,6 @@
>  #include <asm/numa.h>
>  #include <asm/e820.h>
>  
> -#if (defined(CONFIG_ACPI_HOTPLUG_MEMORY) || \
> -	defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)) \
> -		&& !defined(CONFIG_MEMORY_HOTPLUG)
> -#define RESERVE_HOTADD 1
> -#endif

Thanks goodness this is going away :)

>  static struct acpi_table_slit *acpi_slit;
>  
>  static nodemask_t nodes_parsed __initdata;
> @@ -34,9 +28,6 @@
>  static struct bootnode nodes_add[MAX_NUMNODES] __initdata;
>  static int found_add_area __initdata;
>  int hotadd_percent __initdata = 0;
> -#ifndef RESERVE_HOTADD
> -#define hotadd_percent 0	/* Ignore all settings */
> -#endif
>  
>  /* Too small nodes confuse the VM badly. Usually they result
>     from BIOS bugs. */
> @@ -157,7 +148,7 @@
>  	       pxm, pa->apic_id, node);
>  }
>  
> -#ifdef RESERVE_HOTADD
> +#ifdef CONFIG_MEMORY_HOTPLUG_RESERVE
>  /*
>   * Protect against too large hotadd areas that would fill up memory.
>   */
> @@ -200,15 +191,37 @@
>  	return 1;
>  }
>  
> +static int update_end_of_memory(unsigned long end)
> +{
> +	found_add_area = 1;
> +	if ((end >> PAGE_SHIFT) > end_pfn)
> +		end_pfn = end >> PAGE_SHIFT;
> +	return 1;
> +}

I don't have a really strong feeling either way, but you can use
include/linux/pfn.h and PFN_DOWN() here, instead of the explicit >>'s.

> +static inline int save_add_info(void)
> +{
> +	return hotadd_percent > 0;
> +}

This name is a wee bit too generic to be in the global namespace.
Perhaps there should be a "memory" in there somewhere.


> -#ifdef RESERVE_HOTADD
> - 	if (ma->flags.hot_pluggable && reserve_hotadd(node, start, end) < 0) {
> + 	if (ma->flags.hot_pluggable && !reserve_hotadd(node, start, end) < 0) {
>  		/* Ignore hotadd region. Undo damage */
>  		printk(KERN_NOTICE "SRAT: Hotplug region ignored\n");
>  		*nd = oldnode;
>  		if ((nd->start | nd->end) == 0)
>  			node_clear(node, nodes_parsed);
>  	}
> -#endif
>  }

Cool.  No more #ifdef.

-- Dave