* Re: [tip:x86/mm] x86-64, NUMA: Restructure initmem_init()
[not found] <tip-ffe77a4605fb2588f8666850ad3e3b196241658f@git.kernel.org>
@ 2011-02-20 3:34 ` David Rientjes
2011-02-21 8:35 ` Tejun Heo
0 siblings, 1 reply; 13+ messages in thread
From: David Rientjes @ 2011-02-20 3:34 UTC (permalink / raw)
To: Ingo Molnar, H. Peter Anvin, tglx, Tejun Heo
Cc: yinghai, brgerst, gorcunov, shaohui.zheng, linux-kernel
On Wed, 16 Feb 2011, tip-bot for Tejun Heo wrote:
> diff --git a/arch/x86/mm/numa_64.c b/arch/x86/mm/numa_64.c
> index 656b0cf..c984e34 100644
> --- a/arch/x86/mm/numa_64.c
> +++ b/arch/x86/mm/numa_64.c
> @@ -580,65 +580,73 @@ static int __init numa_emulation(unsigned long start_pfn,
> }
> #endif /* CONFIG_NUMA_EMU */
>
> -void __init initmem_init(void)
> +static int dummy_numa_init(void)
> {
> - int acpi = 0, amd = 0;
> - int i;
> -
> -#ifdef CONFIG_ACPI_NUMA
> - /*
> - * Parse SRAT to discover nodes.
> - */
> - acpi = !x86_acpi_numa_init();
> -#endif
> -
> -#ifdef CONFIG_AMD_NUMA
> - if (!acpi)
> - amd = !amd_numa_init();
> -#endif
> -
> - nodes_clear(node_possible_map);
> - nodes_clear(node_online_map);
> -
> -#ifdef CONFIG_NUMA_EMU
> - setup_physnodes(0, max_pfn << PAGE_SHIFT, acpi, amd);
> - if (cmdline && !numa_emulation(0, max_pfn, acpi, amd))
> - return;
> - setup_physnodes(0, max_pfn << PAGE_SHIFT, acpi, amd);
> - nodes_clear(node_possible_map);
> - nodes_clear(node_online_map);
> -#endif
> -
> -#ifdef CONFIG_ACPI_NUMA
> - if (!numa_off && acpi && !acpi_scan_nodes())
> - return;
> - nodes_clear(node_possible_map);
> - nodes_clear(node_online_map);
> -#endif
> + return 0;
> +}
>
> -#ifdef CONFIG_AMD_NUMA
> - if (!numa_off && amd && !amd_scan_nodes())
> - return;
> - nodes_clear(node_possible_map);
> - nodes_clear(node_online_map);
> -#endif
> +static int dummy_scan_nodes(void)
> +{
> printk(KERN_INFO "%s\n",
> numa_off ? "NUMA turned off" : "No NUMA configuration found");
> -
> printk(KERN_INFO "Faking a node at %016lx-%016lx\n",
> 0LU, max_pfn << PAGE_SHIFT);
> +
> /* setup dummy node covering all memory */
> memnode_shift = 63;
> memnodemap = memnode.embedded_map;
> memnodemap[0] = 0;
> node_set_online(0);
> node_set(0, node_possible_map);
> - for (i = 0; i < MAX_LOCAL_APIC; i++)
> - set_apicid_to_node(i, NUMA_NO_NODE);
> memblock_x86_register_active_regions(0, 0, max_pfn);
> init_memory_mapping_high();
> setup_node_bootmem(0, 0, max_pfn << PAGE_SHIFT);
> numa_init_array();
> +
> + return 0;
> +}
> +
> +void __init initmem_init(void)
> +{
> + int (*numa_init[])(void) = { [2] = dummy_numa_init };
> + int (*scan_nodes[])(void) = { [2] = dummy_scan_nodes };
> + int i, j;
> +
> + if (!numa_off) {
> +#ifdef CONFIG_ACPI_NUMA
> + numa_init[0] = x86_acpi_numa_init;
> + scan_nodes[0] = acpi_scan_nodes;
> +#endif
> +#ifdef CONFIG_AMD_NUMA
> + numa_init[1] = amd_numa_init;
> + scan_nodes[1] = amd_scan_nodes;
> +#endif
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(numa_init); i++) {
> + if (!numa_init[i])
> + continue;
> +
> + for (j = 0; j < MAX_LOCAL_APIC; j++)
> + set_apicid_to_node(j, NUMA_NO_NODE);
> +
> + nodes_clear(node_possible_map);
> + nodes_clear(node_online_map);
> +
> + if (numa_init[i]() < 0)
> + continue;
> +#ifdef CONFIG_NUMA_EMU
> + setup_physnodes(0, max_pfn << PAGE_SHIFT, i == 0, i == 1);
> + if (cmdline && !numa_emulation(0, max_pfn, i == 0, i == 1))
> + return;
> + setup_physnodes(0, max_pfn << PAGE_SHIFT, i == 0, i == 1);
> + nodes_clear(node_possible_map);
> + nodes_clear(node_online_map);
> +#endif
> + if (!scan_nodes[i]())
> + return;
> + }
> + BUG();
> }
This doesn't look very clean, I think it would be much better to extract
the iteration out to a function of its own which takes an init and scan
function as arguments and then deal only with numa_off, CONFIG_ACPI_NUMA,
and CONFIG_AMD_NUMA callers in initmem_init(). If that's done, it's clear
that the dummy implementation will never fail and so the BUG() in this
case is never possible (which is intended if all else fails).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [tip:x86/mm] x86-64, NUMA: Restructure initmem_init()
2011-02-20 3:34 ` [tip:x86/mm] x86-64, NUMA: Restructure initmem_init() David Rientjes
@ 2011-02-21 8:35 ` Tejun Heo
2011-03-03 21:15 ` [patch] x86, mm: Clean up initmem_init David Rientjes
0 siblings, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2011-02-21 8:35 UTC (permalink / raw)
To: David Rientjes
Cc: Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst, gorcunov,
shaohui.zheng, linux-kernel
On Sat, Feb 19, 2011 at 07:34:57PM -0800, David Rientjes wrote:
> This doesn't look very clean, I think it would be much better to extract
> the iteration out to a function of its own which takes an init and scan
> function as arguments and then deal only with numa_off, CONFIG_ACPI_NUMA,
> and CONFIG_AMD_NUMA callers in initmem_init(). If that's done, it's clear
> that the dummy implementation will never fail and so the BUG() in this
> case is never possible (which is intended if all else fails).
I don't know. Maybe. Care to send a patch?
--
tejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch] x86, mm: Clean up initmem_init
2011-02-21 8:35 ` Tejun Heo
@ 2011-03-03 21:15 ` David Rientjes
2011-03-03 21:43 ` Cyrill Gorcunov
2011-03-04 10:16 ` Tejun Heo
0 siblings, 2 replies; 13+ messages in thread
From: David Rientjes @ 2011-03-03 21:15 UTC (permalink / raw)
To: Tejun Heo
Cc: Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst, gorcunov,
shaohui.zheng, linux-kernel
This patch cleans initmem_init() so that it is more readable and doesn't
use an unnecessary array of function pointers to convolute the flow of
the code. It also makes it obvious that dummy_numa_init() will always
succeed (and documents that requirement) so that the existing BUG() is
never actually reached.
No functional change.
Signed-off-by: David Rientjes <rientjes@google.com>
---
arch/x86/mm/numa_64.c | 92 ++++++++++++++++++++++++++++---------------------
1 files changed, 53 insertions(+), 39 deletions(-)
diff --git a/arch/x86/mm/numa_64.c b/arch/x86/mm/numa_64.c
--- a/arch/x86/mm/numa_64.c
+++ b/arch/x86/mm/numa_64.c
@@ -564,6 +564,13 @@ static int __init numa_register_memblks(struct numa_meminfo *mi)
return 0;
}
+/*
+ * Used if there's no underlying NUMA architecture, NUMA initialization fails,
+ * or NUMA is disabled on the command line.
+ *
+ * Must online at least one node and add memory blocks that cover all allowed
+ * memory.
+ */
static int __init dummy_numa_init(void)
{
printk(KERN_INFO "%s\n",
@@ -577,57 +584,64 @@ static int __init dummy_numa_init(void)
return 0;
}
-void __init initmem_init(void)
+static int __init numa_init(int (*init_func)(void))
{
- int (*numa_init[])(void) = { [2] = dummy_numa_init };
- int i, j;
-
- if (!numa_off) {
-#ifdef CONFIG_ACPI_NUMA
- numa_init[0] = x86_acpi_numa_init;
-#endif
-#ifdef CONFIG_AMD_NUMA
- numa_init[1] = amd_numa_init;
-#endif
- }
+ int i;
+ int ret;
- for (i = 0; i < ARRAY_SIZE(numa_init); i++) {
- if (!numa_init[i])
- continue;
+ for (i = 0; i < MAX_LOCAL_APIC; i++)
+ set_apicid_to_node(i, NUMA_NO_NODE);
- for (j = 0; j < MAX_LOCAL_APIC; j++)
- set_apicid_to_node(j, NUMA_NO_NODE);
+ nodes_clear(numa_nodes_parsed);
+ nodes_clear(node_possible_map);
+ nodes_clear(node_online_map);
+ memset(&numa_meminfo, 0, sizeof(numa_meminfo));
+ remove_all_active_ranges();
+ numa_reset_distance();
- nodes_clear(numa_nodes_parsed);
- nodes_clear(node_possible_map);
- nodes_clear(node_online_map);
- memset(&numa_meminfo, 0, sizeof(numa_meminfo));
- remove_all_active_ranges();
- numa_reset_distance();
+ ret = init_func();
+ if (ret < 0)
+ return ret;
+ ret = numa_cleanup_meminfo(&numa_meminfo);
+ if (ret < 0)
+ return ret;
- if (numa_init[i]() < 0)
- continue;
+ numa_emulation(&numa_meminfo, numa_distance_cnt);
- if (numa_cleanup_meminfo(&numa_meminfo) < 0)
- continue;
+ ret = numa_register_memblks(&numa_meminfo);
+ if (ret < 0)
+ return ret;
- numa_emulation(&numa_meminfo, numa_distance_cnt);
+ for (i = 0; i < nr_cpu_ids; i++) {
+ int nid = early_cpu_to_node(i);
- if (numa_register_memblks(&numa_meminfo) < 0)
+ if (nid == NUMA_NO_NODE)
continue;
+ if (!node_online(nid))
+ numa_clear_node(i);
+ }
+ numa_init_array();
+ return 0;
+}
- for (j = 0; j < nr_cpu_ids; j++) {
- int nid = early_cpu_to_node(j);
+void __init initmem_init(void)
+{
+ int ret;
- if (nid == NUMA_NO_NODE)
- continue;
- if (!node_online(nid))
- numa_clear_node(j);
- }
- numa_init_array();
- return;
+ if (!numa_off) {
+#ifdef CONFIG_ACPI_NUMA
+ ret = numa_init(x86_acpi_numa_init);
+ if (!ret)
+ return;
+#endif
+#ifdef CONFIG_AMD_NUMA
+ ret = numa_init(amd_numa_init);
+ if (!ret)
+ return;
+#endif
}
- BUG();
+
+ numa_init(dummy_numa_init);
}
unsigned long __init numa_free_all_bootmem(void)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 21:15 ` [patch] x86, mm: Clean up initmem_init David Rientjes
@ 2011-03-03 21:43 ` Cyrill Gorcunov
2011-03-03 22:00 ` David Rientjes
2011-03-04 10:16 ` Tejun Heo
1 sibling, 1 reply; 13+ messages in thread
From: Cyrill Gorcunov @ 2011-03-03 21:43 UTC (permalink / raw)
To: David Rientjes
Cc: Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst,
shaohui.zheng, linux-kernel
On 03/04/2011 12:15 AM, David Rientjes wrote:
> This patch cleans initmem_init() so that it is more readable and doesn't
> use an unnecessary array of function pointers to convolute the flow of
> the code. It also makes it obvious that dummy_numa_init() will always
> succeed (and documents that requirement) so that the existing BUG() is
> never actually reached.
>
> No functional change.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
> arch/x86/mm/numa_64.c | 92 ++++++++++++++++++++++++++++---------------------
> 1 files changed, 53 insertions(+), 39 deletions(-)
>
...
> +void __init initmem_init(void)
> +{
> + int ret;
>
> - if (nid == NUMA_NO_NODE)
> - continue;
> - if (!node_online(nid))
> - numa_clear_node(j);
> - }
> - numa_init_array();
> - return;
> + if (!numa_off) {
> +#ifdef CONFIG_ACPI_NUMA
> + ret = numa_init(x86_acpi_numa_init);
> + if (!ret)
> + return;
> +#endif
> +#ifdef CONFIG_AMD_NUMA
> + ret = numa_init(amd_numa_init);
> + if (!ret)
> + return;
> +#endif
> }
> - BUG();
> +
> + numa_init(dummy_numa_init);
> }
>
> unsigned long __init numa_free_all_bootmem(void)
Divid, I suspect it's due to diff format and we still need "ret" here, right?
--
Cyrill
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 21:43 ` Cyrill Gorcunov
@ 2011-03-03 22:00 ` David Rientjes
2011-03-03 22:03 ` Yinghai Lu
0 siblings, 1 reply; 13+ messages in thread
From: David Rientjes @ 2011-03-03 22:00 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst,
shaohui.zheng, linux-kernel
On Fri, 4 Mar 2011, Cyrill Gorcunov wrote:
> > This patch cleans initmem_init() so that it is more readable and doesn't
> > use an unnecessary array of function pointers to convolute the flow of
> > the code. It also makes it obvious that dummy_numa_init() will always
> > succeed (and documents that requirement) so that the existing BUG() is
> > never actually reached.
> >
> > No functional change.
> >
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> > arch/x86/mm/numa_64.c | 92 ++++++++++++++++++++++++++++---------------------
> > 1 files changed, 53 insertions(+), 39 deletions(-)
> >
> ...
> > +void __init initmem_init(void)
> > +{
> > + int ret;
> >
> > - if (nid == NUMA_NO_NODE)
> > - continue;
> > - if (!node_online(nid))
> > - numa_clear_node(j);
> > - }
> > - numa_init_array();
> > - return;
> > + if (!numa_off) {
> > +#ifdef CONFIG_ACPI_NUMA
> > + ret = numa_init(x86_acpi_numa_init);
> > + if (!ret)
> > + return;
> > +#endif
> > +#ifdef CONFIG_AMD_NUMA
> > + ret = numa_init(amd_numa_init);
> > + if (!ret)
> > + return;
> > +#endif
> > }
> > - BUG();
> > +
> > + numa_init(dummy_numa_init);
> > }
> >
> > unsigned long __init numa_free_all_bootmem(void)
>
> Divid, I suspect it's due to diff format and we still need "ret" here, right?
>
Not sure what you mean. "ret" is a temporary variable to test the return
values of numa_init() and return from initmem_init() on success.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 22:00 ` David Rientjes
@ 2011-03-03 22:03 ` Yinghai Lu
2011-03-03 22:05 ` Cyrill Gorcunov
2011-03-03 22:05 ` David Rientjes
0 siblings, 2 replies; 13+ messages in thread
From: Yinghai Lu @ 2011-03-03 22:03 UTC (permalink / raw)
To: David Rientjes
Cc: Cyrill Gorcunov, Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx,
brgerst, shaohui.zheng, linux-kernel
On 03/03/2011 02:00 PM, David Rientjes wrote:
he want
> +#ifdef CONFIG_ACPI_NUMA
>> > + ret = numa_init(x86_acpi_numa_init);
>> > + if (!ret)
>> > + return;
>> > +#endif
>> > +#ifdef CONFIG_AMD_NUMA
>> > + ret = numa_init(amd_numa_init);
>> > + if (!ret)
>> > + return;
>> > +#endif
to be replaced by:
> +#ifdef CONFIG_ACPI_NUMA
>> > + if (!numa_init(x86_acpi_numa_init))
>> > + return;
>> > +#endif
>> > +#ifdef CONFIG_AMD_NUMA
>> > + if (!numa_init(amd_numa_init))
>> > + return;
>> > +#endif
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 22:03 ` Yinghai Lu
@ 2011-03-03 22:05 ` Cyrill Gorcunov
2011-03-03 22:05 ` David Rientjes
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2011-03-03 22:05 UTC (permalink / raw)
To: Yinghai Lu
Cc: David Rientjes, Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx,
brgerst, shaohui.zheng, linux-kernel
On 03/04/2011 01:03 AM, Yinghai Lu wrote:
> On 03/03/2011 02:00 PM, David Rientjes wrote:
>
> he want
>
yeah, if only i didn't miss something
--
Cyrill
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 22:03 ` Yinghai Lu
2011-03-03 22:05 ` Cyrill Gorcunov
@ 2011-03-03 22:05 ` David Rientjes
2011-03-03 22:10 ` Cyrill Gorcunov
2011-03-04 7:08 ` Ingo Molnar
1 sibling, 2 replies; 13+ messages in thread
From: David Rientjes @ 2011-03-03 22:05 UTC (permalink / raw)
To: Yinghai Lu
Cc: Cyrill Gorcunov, Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx,
brgerst, shaohui.zheng, linux-kernel
On Thu, 3 Mar 2011, Yinghai Lu wrote:
> he want
>
> > +#ifdef CONFIG_ACPI_NUMA
> >> > + ret = numa_init(x86_acpi_numa_init);
> >> > + if (!ret)
> >> > + return;
> >> > +#endif
> >> > +#ifdef CONFIG_AMD_NUMA
> >> > + ret = numa_init(amd_numa_init);
> >> > + if (!ret)
> >> > + return;
> >> > +#endif
>
> to be replaced by:
>
> > +#ifdef CONFIG_ACPI_NUMA
> >> > + if (!numa_init(x86_acpi_numa_init))
> >> > + return;
> >> > +#endif
> >> > +#ifdef CONFIG_AMD_NUMA
> >> > + if (!numa_init(amd_numa_init))
> >> > + return;
> >> > +#endif
It's a matter of style and I think it's up to Ingo what he'd prefer to
see.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 22:05 ` David Rientjes
@ 2011-03-03 22:10 ` Cyrill Gorcunov
2011-03-04 7:08 ` Ingo Molnar
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2011-03-03 22:10 UTC (permalink / raw)
To: David Rientjes
Cc: Yinghai Lu, Tejun Heo, Ingo Molnar, H. Peter Anvin, tglx, brgerst,
linux-kernel
On 03/04/2011 01:05 AM, David Rientjes wrote:
> On Thu, 3 Mar 2011, Yinghai Lu wrote:
>
>> he want
>>
>>> +#ifdef CONFIG_ACPI_NUMA
>>>>> + ret = numa_init(x86_acpi_numa_init);
>>>>> + if (!ret)
>>>>> + return;
>>>>> +#endif
>>>>> +#ifdef CONFIG_AMD_NUMA
>>>>> + ret = numa_init(amd_numa_init);
>>>>> + if (!ret)
>>>>> + return;
>>>>> +#endif
>>
>> to be replaced by:
>>
>>> +#ifdef CONFIG_ACPI_NUMA
>>>>> + if (!numa_init(x86_acpi_numa_init))
>>>>> + return;
>>>>> +#endif
>>>>> +#ifdef CONFIG_AMD_NUMA
>>>>> + if (!numa_init(amd_numa_init))
>>>>> + return;
>>>>> +#endif
>
> It's a matter of style and I think it's up to Ingo what he'd prefer to
> see.
i'm fine with either, just though it would shrink the code a bit (since there
is no need to track var witch i guess will be eliminated by gcc anyway).
nb: am i only the one who obtain bounce from shaohui.zheng@intel.com (so i'd to remove
him from cc'ing)?
--
Cyrill
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 22:05 ` David Rientjes
2011-03-03 22:10 ` Cyrill Gorcunov
@ 2011-03-04 7:08 ` Ingo Molnar
2011-03-04 12:38 ` Cyrill Gorcunov
1 sibling, 1 reply; 13+ messages in thread
From: Ingo Molnar @ 2011-03-04 7:08 UTC (permalink / raw)
To: David Rientjes
Cc: Yinghai Lu, Cyrill Gorcunov, Tejun Heo, Ingo Molnar,
H. Peter Anvin, tglx, brgerst, shaohui.zheng, linux-kernel
* David Rientjes <rientjes@google.com> wrote:
> On Thu, 3 Mar 2011, Yinghai Lu wrote:
>
> > he want
> >
> > > +#ifdef CONFIG_ACPI_NUMA
> > >> > + ret = numa_init(x86_acpi_numa_init);
> > >> > + if (!ret)
> > >> > + return;
> > >> > +#endif
> > >> > +#ifdef CONFIG_AMD_NUMA
> > >> > + ret = numa_init(amd_numa_init);
> > >> > + if (!ret)
> > >> > + return;
> > >> > +#endif
> >
> > to be replaced by:
> >
> > > +#ifdef CONFIG_ACPI_NUMA
> > >> > + if (!numa_init(x86_acpi_numa_init))
> > >> > + return;
> > >> > +#endif
> > >> > +#ifdef CONFIG_AMD_NUMA
> > >> > + if (!numa_init(amd_numa_init))
> > >> > + return;
> > >> > +#endif
>
> It's a matter of style and I think it's up to Ingo what he'd prefer to
> see.
I think your variant is cleaner: hiding function call side-effects in conditionscan
be a fragile thing to do. We want constant expressions with no side-effects - so if
functions are called they should be constant functions as well.
Code compactness isn't everything - if it was we'd be using C to the max to create
unreadable compound expressions all the time.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-03 21:15 ` [patch] x86, mm: Clean up initmem_init David Rientjes
2011-03-03 21:43 ` Cyrill Gorcunov
@ 2011-03-04 10:16 ` Tejun Heo
2011-03-04 14:19 ` [PATCH x86/mm] x86-64, NUMA: Clean up initmem_init() Tejun Heo
1 sibling, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2011-03-04 10:16 UTC (permalink / raw)
To: David Rientjes
Cc: Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst, gorcunov,
shaohui.zheng, linux-kernel
Hello,
On Thu, Mar 03, 2011 at 01:15:04PM -0800, David Rientjes wrote:
> This patch cleans initmem_init() so that it is more readable and doesn't
> use an unnecessary array of function pointers to convolute the flow of
> the code. It also makes it obvious that dummy_numa_init() will always
> succeed (and documents that requirement) so that the existing BUG() is
> never actually reached.
>
> No functional change.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
Just a nitpick.
> +/*
> + * Used if there's no underlying NUMA architecture, NUMA initialization fails,
> + * or NUMA is disabled on the command line.
> + *
> + * Must online at least one node and add memory blocks that cover all allowed
> + * memory.
> + */
Can you please reformat to docbook style?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] x86, mm: Clean up initmem_init
2011-03-04 7:08 ` Ingo Molnar
@ 2011-03-04 12:38 ` Cyrill Gorcunov
0 siblings, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2011-03-04 12:38 UTC (permalink / raw)
To: Ingo Molnar
Cc: David Rientjes, Yinghai Lu, Tejun Heo, Ingo Molnar,
H. Peter Anvin, tglx, brgerst, shaohui.zheng, linux-kernel
On Fri, Mar 4, 2011 at 10:08 AM, Ingo Molnar <mingo@elte.hu> wrote:
...
>>
>> It's a matter of style and I think it's up to Ingo what he'd prefer to
>> see.
>
> I think your variant is cleaner: hiding function call side-effects in conditionscan
> be a fragile thing to do. We want constant expressions with no side-effects - so if
> functions are called they should be constant functions as well.
Well, we do it a lot with say strcmp and others so I would use rather
semantic argument,
but I agree that David's version is easier to read and mark same time
that these functions
are important steps in bootup procedure. So I'm sorry for proposing
this change in first place.
>
> Code compactness isn't everything - if it was we'd be using C to the max to create
> unreadable compound expressions all the time.
>
> Thanks,
>
> Ingo
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH x86/mm] x86-64, NUMA: Clean up initmem_init()
2011-03-04 10:16 ` Tejun Heo
@ 2011-03-04 14:19 ` Tejun Heo
0 siblings, 0 replies; 13+ messages in thread
From: Tejun Heo @ 2011-03-04 14:19 UTC (permalink / raw)
To: David Rientjes
Cc: Ingo Molnar, H. Peter Anvin, tglx, yinghai, brgerst, gorcunov,
shaohui.zheng, linux-kernel
>From c09cedf4f75f1e47ea17f55e18e9cfb81bec8575 Mon Sep 17 00:00:00 2001
From: David Rientjes <rientjes@google.com>
Date: Fri, 4 Mar 2011 15:17:21 +0100
This patch cleans initmem_init() so that it is more readable and doesn't
use an unnecessary array of function pointers to convolute the flow of
the code. It also makes it obvious that dummy_numa_init() will always
succeed (and documents that requirement) so that the existing BUG() is
never actually reached.
No functional change.
-tj: Updated comment for dummy_numa_init() slightly.
Signed-off-by: David Rientjes <rientjes@google.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
Never mind the previous message. I just updated and applied it.
Thanks.
arch/x86/mm/numa_64.c | 94 ++++++++++++++++++++++++++++--------------------
1 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/arch/x86/mm/numa_64.c b/arch/x86/mm/numa_64.c
index 86491ba..9ec0f20 100644
--- a/arch/x86/mm/numa_64.c
+++ b/arch/x86/mm/numa_64.c
@@ -562,6 +562,15 @@ static int __init numa_register_memblks(struct numa_meminfo *mi)
return 0;
}
+/**
+ * dummy_numma_init - Fallback dummy NUMA init
+ *
+ * Used if there's no underlying NUMA architecture, NUMA initialization
+ * fails, or NUMA is disabled on the command line.
+ *
+ * Must online at least one node and add memory blocks that cover all
+ * allowed memory. This function must not fail.
+ */
static int __init dummy_numa_init(void)
{
printk(KERN_INFO "%s\n",
@@ -575,57 +584,64 @@ static int __init dummy_numa_init(void)
return 0;
}
-void __init initmem_init(void)
+static int __init numa_init(int (*init_func)(void))
{
- int (*numa_init[])(void) = { [2] = dummy_numa_init };
- int i, j;
-
- if (!numa_off) {
-#ifdef CONFIG_ACPI_NUMA
- numa_init[0] = x86_acpi_numa_init;
-#endif
-#ifdef CONFIG_AMD_NUMA
- numa_init[1] = amd_numa_init;
-#endif
- }
+ int i;
+ int ret;
- for (i = 0; i < ARRAY_SIZE(numa_init); i++) {
- if (!numa_init[i])
- continue;
+ for (i = 0; i < MAX_LOCAL_APIC; i++)
+ set_apicid_to_node(i, NUMA_NO_NODE);
- for (j = 0; j < MAX_LOCAL_APIC; j++)
- set_apicid_to_node(j, NUMA_NO_NODE);
+ nodes_clear(numa_nodes_parsed);
+ nodes_clear(node_possible_map);
+ nodes_clear(node_online_map);
+ memset(&numa_meminfo, 0, sizeof(numa_meminfo));
+ remove_all_active_ranges();
+ numa_reset_distance();
- nodes_clear(numa_nodes_parsed);
- nodes_clear(node_possible_map);
- nodes_clear(node_online_map);
- memset(&numa_meminfo, 0, sizeof(numa_meminfo));
- remove_all_active_ranges();
- numa_reset_distance();
+ ret = init_func();
+ if (ret < 0)
+ return ret;
+ ret = numa_cleanup_meminfo(&numa_meminfo);
+ if (ret < 0)
+ return ret;
- if (numa_init[i]() < 0)
- continue;
+ numa_emulation(&numa_meminfo, numa_distance_cnt);
- if (numa_cleanup_meminfo(&numa_meminfo) < 0)
- continue;
+ ret = numa_register_memblks(&numa_meminfo);
+ if (ret < 0)
+ return ret;
- numa_emulation(&numa_meminfo, numa_distance_cnt);
+ for (i = 0; i < nr_cpu_ids; i++) {
+ int nid = early_cpu_to_node(i);
- if (numa_register_memblks(&numa_meminfo) < 0)
+ if (nid == NUMA_NO_NODE)
continue;
+ if (!node_online(nid))
+ numa_clear_node(i);
+ }
+ numa_init_array();
+ return 0;
+}
- for (j = 0; j < nr_cpu_ids; j++) {
- int nid = early_cpu_to_node(j);
+void __init initmem_init(void)
+{
+ int ret;
- if (nid == NUMA_NO_NODE)
- continue;
- if (!node_online(nid))
- numa_clear_node(j);
- }
- numa_init_array();
- return;
+ if (!numa_off) {
+#ifdef CONFIG_ACPI_NUMA
+ ret = numa_init(x86_acpi_numa_init);
+ if (!ret)
+ return;
+#endif
+#ifdef CONFIG_AMD_NUMA
+ ret = numa_init(amd_numa_init);
+ if (!ret)
+ return;
+#endif
}
- BUG();
+
+ numa_init(dummy_numa_init);
}
unsigned long __init numa_free_all_bootmem(void)
--
1.7.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-03-04 14:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <tip-ffe77a4605fb2588f8666850ad3e3b196241658f@git.kernel.org>
2011-02-20 3:34 ` [tip:x86/mm] x86-64, NUMA: Restructure initmem_init() David Rientjes
2011-02-21 8:35 ` Tejun Heo
2011-03-03 21:15 ` [patch] x86, mm: Clean up initmem_init David Rientjes
2011-03-03 21:43 ` Cyrill Gorcunov
2011-03-03 22:00 ` David Rientjes
2011-03-03 22:03 ` Yinghai Lu
2011-03-03 22:05 ` Cyrill Gorcunov
2011-03-03 22:05 ` David Rientjes
2011-03-03 22:10 ` Cyrill Gorcunov
2011-03-04 7:08 ` Ingo Molnar
2011-03-04 12:38 ` Cyrill Gorcunov
2011-03-04 10:16 ` Tejun Heo
2011-03-04 14:19 ` [PATCH x86/mm] x86-64, NUMA: Clean up initmem_init() Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox