lkml.org 
[lkml]   [2019]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/5] x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
Date
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
>
\
 
 \ /
  Last update: 2019-07-10 02:36    [W:0.066 / U:1.200 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site