Re: [PATCH 3/5] x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS <hoan@os.amperecomputing.com>]

Hi Thomas,

Thanks for you comments

On 6/25/19 3:45 PM, Thomas Gleixner wrote:
> Hoan,
> 
> On Tue, 25 Jun 2019, Hoan Tran OS wrote:
> 
> Please use 'x86/Kconfig: ' as prefix.
> 
>> This patch removes CONFIG_NODES_SPAN_OTHER_NODES as it's
>> enabled by default with NUMA.
> 
> Please do not use 'This patch' in changelogs. It's pointless because we
> already know that this is a patch.
> 
> See also Documentation/process/submitting-patches.rst and search for 'This
> patch'
> 
> Simply say:
> 
>    Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled by default with
>    NUMA.
> 

Got it, will fix

> But .....
> 
>> @@ -1567,15 +1567,6 @@ config X86_64_ACPI_NUMA
>>   	---help---
>>   	  Enable ACPI SRAT based node topology detection.
>>   
>> -# Some NUMA nodes have memory ranges that span
>> -# other nodes.  Even though a pfn is valid and
>> -# between a node's start and end pfns, it may not
>> -# reside on that node.  See memmap_init_zone()
>> -# for details.
>> -config NODES_SPAN_OTHER_NODES
>> -	def_bool y
>> -	depends on X86_64_ACPI_NUMA
> 
> the changelog does not mention that this lifts the dependency on
> X86_64_ACPI_NUMA and therefore enables that functionality for anything
> which has NUMA enabled including 32bit.
> 

I think this config is used for a NUMA layout which NUMA nodes addresses 
are spanned to other nodes. I think 32bit NUMA also have the same issue 
with that layout. Please correct me if I'm wrong.

> The core mm change gives no helpful information either. You just copied the
> above comment text from some random Kconfig.

Yes, as it's a correct comment and is used at multiple places.

Thanks
Hoan

> 
> This needs a bit more data in the changelogs and the cover letter:
> 
>       - Why is it useful to enable it unconditionally
> 
>       - Why is it safe to do so, even if the architecture had constraints on
>         it
> 
>       - What's the potential impact
> 
> Thanks,
> 
> 	tglx
>