| Message ID | 1561501810-25163-4-git-send-email-Hoan@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA | expand |
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. 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. The core mm change gives no helpful information either. You just copied the above comment text from some random Kconfig. 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
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 >
Hoan, On Wed, 10 Jul 2019, Hoan Tran OS wrote: > On 6/25/19 3:45 PM, Thomas Gleixner wrote: > > On Tue, 25 Jun 2019, Hoan Tran OS wrote: > >> @@ -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. I'm not saying you're wrong, but it's your duty to provide the analysis why this is correct for everything which has NUMA enabled. > > 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. Well it maybe correct in terms of explaining what this is about, it still does not explain why this is needed by default on everything which has NUMA enabled. Thanks, tglx
Hi Thomas, On 7/10/19 12:58 PM, Thomas Gleixner wrote: > Hoan, > > On Wed, 10 Jul 2019, Hoan Tran OS wrote: >> On 6/25/19 3:45 PM, Thomas Gleixner wrote: >>> On Tue, 25 Jun 2019, Hoan Tran OS wrote: >>>> @@ -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. > > I'm not saying you're wrong, but it's your duty to provide the analysis why > this is correct for everything which has NUMA enabled. > >>> 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. > > Well it maybe correct in terms of explaining what this is about, it still > does not explain why this is needed by default on everything which has NUMA > enabled. Let me send another patch with the detail explanation. Thanks Hoan > > Thanks, > > tglx >
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 2bbbd4d..fa9318c 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -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 - config NUMA_EMU bool "NUMA emulation" depends on NUMA
This patch removes CONFIG_NODES_SPAN_OTHER_NODES as it's enabled by default with NUMA. Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com> --- arch/x86/Kconfig | 9 --------- 1 file changed, 9 deletions(-)