[RFC patch] delete improper hot pluggable code of memory affinity

[RFC patch] delete improper hot pluggable code of memory affinity

[Luming Yu]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

Hello,

The current kernel code *wrongly* interprets Hot Pluggable bit of
Memory Affinity Structure (SRAT table in ACPI spec).
if Hot Pluggable bit is set and CONFIG_MEMORY_HOTPLUG_SPARSE is NOT
set, the memory Affinity will
be ignored. And a faked Node will be used...

An alternative is to enable CONFIG_MEMORY_HOTPLUG_SPARSE *always*
along with acpi_numa_memory_affinity_init.
Please decide which one is appropriate.

The downside of this patch is *some useful info* is lost and a follow
up patch is needed.

**The patch is enclosed in text attachment*
**Using web client to send the patch* *
**below is for review, please apply attached  patch*/

Thanks,
Luming


Signed-off-by: Yu Luming <luming.yu@intel.com>

 srat_64.c |   16 ----------------
 1 file changed, 16 deletions(-)


diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
index 2dfcbf9..82423e5 100644
--- a/arch/x86/mm/srat_64.c
+++ b/arch/x86/mm/srat_64.c
@@ -172,11 +172,6 @@ acpi_numa_processor_affinity_init(struct
acpi_srat_cpu_affinity *pa)
 	       pxm, apic_id, node);
 }

-#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-static inline int save_add_info(void) {return 1;}
-#else
-static inline int save_add_info(void) {return 0;}
-#endif
 /*
  * Update nodes_add[]
  * This code supports one contiguous hot add area per node
@@ -249,9 +244,6 @@ acpi_numa_memory_affinity_init(struct
acpi_srat_mem_affinity *ma)
 	}
 	if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
 		return;
-
-	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
-		return;
 	start = ma->base_address;
 	end = start + ma->length;
 	pxm = ma->proximity_domain;
@@ -291,14 +283,6 @@ acpi_numa_memory_affinity_init(struct
acpi_srat_mem_affinity *ma)
 	e820_register_active_regions(node, start >> PAGE_SHIFT,
 				     end >> PAGE_SHIFT);

-	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
-		update_nodes_add(node, start, end);
-		/* restore nodes[node] */
-		*nd = oldnode;
-		if ((nd->start | nd->end) == 0)
-			node_clear(node, nodes_parsed);
-	}
-
 	node_memblk_range[num_node_memblks].start = start;
 	node_memblk_range[num_node_memblks].end = end;
 	memblk_nodeid[num_node_memblks] = node;

[-- Attachment #2: 0.patch --]
[-- Type: application/octet-stream, Size: 1361 bytes --]

diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
index 2dfcbf9..82423e5 100644
--- a/arch/x86/mm/srat_64.c
+++ b/arch/x86/mm/srat_64.c
@@ -172,11 +172,6 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 	       pxm, apic_id, node);
 }
 
-#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-static inline int save_add_info(void) {return 1;}
-#else
-static inline int save_add_info(void) {return 0;}
-#endif
 /*
  * Update nodes_add[]
  * This code supports one contiguous hot add area per node
@@ -249,9 +244,6 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 	}
 	if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
 		return;
-
-	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
-		return;
 	start = ma->base_address;
 	end = start + ma->length;
 	pxm = ma->proximity_domain;
@@ -291,14 +283,6 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 	e820_register_active_regions(node, start >> PAGE_SHIFT,
 				     end >> PAGE_SHIFT);
 
-	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
-		update_nodes_add(node, start, end);
-		/* restore nodes[node] */
-		*nd = oldnode;
-		if ((nd->start | nd->end) == 0)
-			node_clear(node, nodes_parsed);
-	}
-
 	node_memblk_range[num_node_memblks].start = start;
 	node_memblk_range[num_node_memblks].end = end;
 	memblk_nodeid[num_node_memblks] = node;

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Yasunori Goto]

Hi, Luming-san.

> The current kernel code *wrongly* interprets Hot Pluggable bit of
> Memory Affinity Structure (SRAT table in ACPI spec).

I'm not sure your patch is correct or not yet, but I would like
to tell you a critical point about the definition of 
Memory Affinity Structure.

The spec says the Enable bit of Memory Affinity Structure means that
the contents of its memory affinity structure is only VALID.
It doesn't mean memory is really connected at the area.
It means only that OS can read the entry.

When the enabled bit and hot pluggable bit is set on, 
it may mean that the area may be hot-added after boot up.
So, kernel must check e820 or efi to confirm that memory is
really connected.

If you already know it, sorry for noise....
Just for your information.

Thanks.



> if Hot Pluggable bit is set and CONFIG_MEMORY_HOTPLUG_SPARSE is NOT
> set, the memory Affinity will
> be ignored. And a faked Node will be used...
> 
> An alternative is to enable CONFIG_MEMORY_HOTPLUG_SPARSE *always*
> along with acpi_numa_memory_affinity_init.
> Please decide which one is appropriate.
> 
> The downside of this patch is *some useful info* is lost and a follow
> up patch is needed.
> 
> **The patch is enclosed in text attachment*
> **Using web client to send the patch* *
> **below is for review, please apply attached  patch*/
> 
> Thanks,
> Luming
> 
> 
> Signed-off-by: Yu Luming <luming.yu@intel.com>
> 
>  srat_64.c |   16 ----------------
>  1 file changed, 16 deletions(-)
> 
> 
> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
> index 2dfcbf9..82423e5 100644
> --- a/arch/x86/mm/srat_64.c
> +++ b/arch/x86/mm/srat_64.c
> @@ -172,11 +172,6 @@ acpi_numa_processor_affinity_init(struct
> acpi_srat_cpu_affinity *pa)
>  	       pxm, apic_id, node);
>  }
> 
> -#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
> -static inline int save_add_info(void) {return 1;}
> -#else
> -static inline int save_add_info(void) {return 0;}
> -#endif
>  /*
>   * Update nodes_add[]
>   * This code supports one contiguous hot add area per node
> @@ -249,9 +244,6 @@ acpi_numa_memory_affinity_init(struct
> acpi_srat_mem_affinity *ma)
>  	}
>  	if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
>  		return;
> -
> -	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
> -		return;
>  	start = ma->base_address;
>  	end = start + ma->length;
>  	pxm = ma->proximity_domain;
> @@ -291,14 +283,6 @@ acpi_numa_memory_affinity_init(struct
> acpi_srat_mem_affinity *ma)
>  	e820_register_active_regions(node, start >> PAGE_SHIFT,
>  				     end >> PAGE_SHIFT);
> 
> -	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> -		update_nodes_add(node, start, end);
> -		/* restore nodes[node] */
> -		*nd = oldnode;
> -		if ((nd->start | nd->end) == 0)
> -			node_clear(node, nodes_parsed);
> -	}
> -
>  	node_memblk_range[num_node_memblks].start = start;
>  	node_memblk_range[num_node_memblks].end = end;
>  	memblk_nodeid[num_node_memblks] = node;

-- 
Yasunori Goto 

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Luming Yu]

Without a fix like my proposal, I have seen NUMA configure disabled by
kernel (due to the code the patch deletes) on a system with Enabled
bit set , and Hotplug-able bit cleared, and
CONFIG_MEMORY_HOTPLUG_SPARSE disabled.

On Fri, Jul 17, 2009 at 1:16 PM, Yasunori Goto<y-goto@jp.fujitsu.com> wrote:
>
> Hi, Luming-san.
>
>> The current kernel code *wrongly* interprets Hot Pluggable bit of
>> Memory Affinity Structure (SRAT table in ACPI spec).
>
> I'm not sure your patch is correct or not yet, but I would like
> to tell you a critical point about the definition of
> Memory Affinity Structure.
>
> The spec says the Enable bit of Memory Affinity Structure means that
> the contents of its memory affinity structure is only VALID.
> It doesn't mean memory is really connected at the area.
> It means only that OS can read the entry.
>
> When the enabled bit and hot pluggable bit is set on,
> it may mean that the area may be hot-added after boot up.
> So, kernel must check e820 or efi to confirm that memory is
> really connected.
>
> If you already know it, sorry for noise....
> Just for your information.
>
> Thanks.
>
>
>
>> if Hot Pluggable bit is set and CONFIG_MEMORY_HOTPLUG_SPARSE is NOT
>> set, the memory Affinity will
>> be ignored. And a faked Node will be used...
>>
>> An alternative is to enable CONFIG_MEMORY_HOTPLUG_SPARSE *always*
>> along with acpi_numa_memory_affinity_init.
>> Please decide which one is appropriate.
>>
>> The downside of this patch is *some useful info* is lost and a follow
>> up patch is needed.
>>
>> **The patch is enclosed in text attachment*
>> **Using web client to send the patch* *
>> **below is for review, please apply attached  patch*/
>>
>> Thanks,
>> Luming
>>
>>
>> Signed-off-by: Yu Luming <luming.yu@intel.com>
>>
>>  srat_64.c |   16 ----------------
>>  1 file changed, 16 deletions(-)
>>
>>
>> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
>> index 2dfcbf9..82423e5 100644
>> --- a/arch/x86/mm/srat_64.c
>> +++ b/arch/x86/mm/srat_64.c
>> @@ -172,11 +172,6 @@ acpi_numa_processor_affinity_init(struct
>> acpi_srat_cpu_affinity *pa)
>>              pxm, apic_id, node);
>>  }
>>
>> -#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
>> -static inline int save_add_info(void) {return 1;}
>> -#else
>> -static inline int save_add_info(void) {return 0;}
>> -#endif
>>  /*
>>   * Update nodes_add[]
>>   * This code supports one contiguous hot add area per node
>> @@ -249,9 +244,6 @@ acpi_numa_memory_affinity_init(struct
>> acpi_srat_mem_affinity *ma)
>>       }
>>       if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
>>               return;
>> -
>> -     if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
>> -             return;
>>       start = ma->base_address;
>>       end = start + ma->length;
>>       pxm = ma->proximity_domain;
>> @@ -291,14 +283,6 @@ acpi_numa_memory_affinity_init(struct
>> acpi_srat_mem_affinity *ma)
>>       e820_register_active_regions(node, start >> PAGE_SHIFT,
>>                                    end >> PAGE_SHIFT);
>>
>> -     if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
>> -             update_nodes_add(node, start, end);
>> -             /* restore nodes[node] */
>> -             *nd = oldnode;
>> -             if ((nd->start | nd->end) == 0)
>> -                     node_clear(node, nodes_parsed);
>> -     }
>> -
>>       node_memblk_range[num_node_memblks].start = start;
>>       node_memblk_range[num_node_memblks].end = end;
>>       memblk_nodeid[num_node_memblks] = node;
>
> --
> Yasunori Goto
>
>
>

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Yasunori Goto]

> Without a fix like my proposal, I have seen NUMA configure disabled by
> kernel (due to the code the patch deletes) on a system with Enabled
> bit set , and Hotplug-able bit cleared, and
> CONFIG_MEMORY_HOTPLUG_SPARSE disabled.

Ok. I guess that save_add_info() was to check percentage of 
reserve area when CONFIG_MEMORY_HOTPLUG_RESERVE is set.
Its code was removed at 2.6.25, save_add_info() may be garbage now.

However, I have one question now.

>> - 	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
>> - 		update_nodes_add(node, start, end);
>> - 		/* restore nodes[node] */
>> - 		*nd = oldnode;
>> - 		if ((nd->start | nd->end) == 0)
>> - 			node_clear(node, nodes_parsed);
>> -     }

I don't understand why you remove this code. could you tell me why?

Bye.


> 
> On Fri, Jul 17, 2009 at 1:16 PM, Yasunori Goto<y-goto@jp.fujitsu.com> wrote:
> >
> > Hi, Luming-san.
> >
> >> The current kernel code *wrongly* interprets Hot Pluggable bit of
> >> Memory Affinity Structure (SRAT table in ACPI spec).
> >
> > I'm not sure your patch is correct or not yet, but I would like
> > to tell you a critical point about the definition of
> > Memory Affinity Structure.
> >
> > The spec says the Enable bit of Memory Affinity Structure means that
> > the contents of its memory affinity structure is only VALID.
> > It doesn't mean memory is really connected at the area.
> > It means only that OS can read the entry.
> >
> > When the enabled bit and hot pluggable bit is set on,
> > it may mean that the area may be hot-added after boot up.
> > So, kernel must check e820 or efi to confirm that memory is
> > really connected.
> >
> > If you already know it, sorry for noise....
> > Just for your information.
> >
> > Thanks.
> >
> >
> >
> >> if Hot Pluggable bit is set and CONFIG_MEMORY_HOTPLUG_SPARSE is NOT
> >> set, the memory Affinity will
> >> be ignored. And a faked Node will be used...
> >>
> >> An alternative is to enable CONFIG_MEMORY_HOTPLUG_SPARSE *always*
> >> along with acpi_numa_memory_affinity_init.
> >> Please decide which one is appropriate.
> >>
> >> The downside of this patch is *some useful info* is lost and a follow
> >> up patch is needed.
> >>
> >> **The patch is enclosed in text attachment*
> >> **Using web client to send the patch* *
> >> **below is for review, please apply attached ?patch*/
> >>
> >> Thanks,
> >> Luming
> >>
> >>
> >> Signed-off-by: Yu Luming <luming.yu@intel.com>
> >>
> >> ?srat_64.c | ? 16 ----------------
> >> ?1 file changed, 16 deletions(-)
> >>
> >>
> >> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
> >> index 2dfcbf9..82423e5 100644
> >> --- a/arch/x86/mm/srat_64.c
> >> +++ b/arch/x86/mm/srat_64.c
> >> @@ -172,11 +172,6 @@ acpi_numa_processor_affinity_init(struct
> >> acpi_srat_cpu_affinity *pa)
> >> ? ? ? ? ? ? ?pxm, apic_id, node);
> >> ?}
> >>
> >> -#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
> >> -static inline int save_add_info(void) {return 1;}
> >> -#else
> >> -static inline int save_add_info(void) {return 0;}
> >> -#endif
> >> ?/*
> >> ? * Update nodes_add[]
> >> ? * This code supports one contiguous hot add area per node
> >> @@ -249,9 +244,6 @@ acpi_numa_memory_affinity_init(struct
> >> acpi_srat_mem_affinity *ma)
> >> ? ? ? }
> >> ? ? ? if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
> >> ? ? ? ? ? ? ? return;
> >> -
> >> - ? ? if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
> >> - ? ? ? ? ? ? return;
> >> ? ? ? start = ma->base_address;
> >> ? ? ? end = start + ma->length;
> >> ? ? ? pxm = ma->proximity_domain;
> >> @@ -291,14 +283,6 @@ acpi_numa_memory_affinity_init(struct
> >> acpi_srat_mem_affinity *ma)
> >> ? ? ? e820_register_active_regions(node, start >> PAGE_SHIFT,
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?end >> PAGE_SHIFT);
> >>
> >> - ? ? if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> >> - ? ? ? ? ? ? update_nodes_add(node, start, end);
> >> - ? ? ? ? ? ? /* restore nodes[node] */
> >> - ? ? ? ? ? ? *nd = oldnode;
> >> - ? ? ? ? ? ? if ((nd->start | nd->end) == 0)
> >> - ? ? ? ? ? ? ? ? ? ? node_clear(node, nodes_parsed);
> >> - ? ? }
> >> -
> >> ? ? ? node_memblk_range[num_node_memblks].start = start;
> >> ? ? ? node_memblk_range[num_node_memblks].end = end;
> >> ? ? ? memblk_nodeid[num_node_memblks] = node;
> >
> > --
> > Yasunori Goto
> >
> >
> >

-- 
Yasunori Goto 

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Luming Yu]

On Fri, Jul 17, 2009 at 1:52 PM, Yasunori Goto<y-goto@jp.fujitsu.com> wrote:
>> Without a fix like my proposal, I have seen NUMA configure disabled by
>> kernel (due to the code the patch deletes) on a system with Enabled
>> bit set , and Hotplug-able bit cleared, and
>> CONFIG_MEMORY_HOTPLUG_SPARSE disabled.
>
> Ok. I guess that save_add_info() was to check percentage of
> reserve area when CONFIG_MEMORY_HOTPLUG_RESERVE is set.
> Its code was removed at 2.6.25, save_add_info() may be garbage now.
>
> However, I have one question now.
>
>>> -    if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
>>> -            update_nodes_add(node, start, end);
>>> -            /* restore nodes[node] */
>>> -            *nd = oldnode;
>>> -            if ((nd->start | nd->end) == 0)
>>> -                    node_clear(node, nodes_parsed);
>>> -     }
>
> I don't understand why you remove this code. could you tell me why?

Good question. This is exactly the place I'm puzzled too.
Without delete this code, I still see one fake node instead of 4 real node...
I think a flow up patch is needed here...

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Yasunori Goto]

> On Fri, Jul 17, 2009 at 1:52 PM, Yasunori Goto<y-goto@jp.fujitsu.com> wrote:
> >> Without a fix like my proposal, I have seen NUMA configure disabled by
> >> kernel (due to the code the patch deletes) on a system with Enabled
> >> bit set , and Hotplug-able bit cleared, and
> >> CONFIG_MEMORY_HOTPLUG_SPARSE disabled.
> >
> > Ok. I guess that save_add_info() was to check percentage of
> > reserve area when CONFIG_MEMORY_HOTPLUG_RESERVE is set.
> > Its code was removed at 2.6.25, save_add_info() may be garbage now.
> >
> > However, I have one question now.
> >
> >>> - ? ?if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> >>> - ? ? ? ? ? ?update_nodes_add(node, start, end);
> >>> - ? ? ? ? ? ?/* restore nodes[node] */
> >>> - ? ? ? ? ? ?*nd = oldnode;
> >>> - ? ? ? ? ? ?if ((nd->start | nd->end) == 0)
> >>> - ? ? ? ? ? ? ? ? ? ?node_clear(node, nodes_parsed);
> >>> - ? ? }
> >
> > I don't understand why you remove this code. could you tell me why?
> 
> Good question. This is exactly the place I'm puzzled too.
> Without delete this code, I still see one fake node instead of 4 real node...
> I think a flow up patch is needed here...

In my understanding, here is for the case when the SRAT entry is enabled,
but memory is not connected. (This is why I sent the first mail).
However, here may have a bug yet. 

I would like to test here, but I don't have a real x86-64 machine which can
hot add memory. So, I can just guess now.

Anyway, thanks for your notification. If you have progress about it,
please let me know.

Bye.

-- 
Yasunori Goto 

Re: [RFC patch] delete improper hot pluggable code of memory affinity

[Andi Kleen]

Luming Yu <luming.yu@gmail.com> writes:
>
> The downside of this patch is *some useful info* is lost and a follow
> up patch is needed.

A lot of the heuristics in the hotadd code are obsolete since x86-64
only supports sparsemem/vmememap

I posted another patch for this and also have a followup
series (not posted yet) to address some more problems.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.