Message ID | 20190801075258.19070-1-tao3.xu@intel.com |
---|---|
State | New |
Headers | show |
Series | [RFC] numa: add auto_enable_numa to fix broken check in spapr | expand |
On Thu, Aug 01, 2019 at 03:52:58PM +0800, Tao Xu wrote: > Introduce MachineClass::auto_enable_numa for one implicit NUMA node, > and enable it to fix broken check in spapr_validate_node_memory(), when > spapr_populate_memory() creates a implicit node and info then use > nb_numa_nodes which is 0. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> The change here looks fine so, Acked-by: David Gibson <david@gibson.dropbear.id.au> However, I'm not following what check in spapr is broken and why. > --- > > This patch has a dependency on > https://patchwork.kernel.org/cover/11063235/ > --- > hw/core/numa.c | 9 +++++++-- > hw/ppc/spapr.c | 9 +-------- > include/hw/boards.h | 1 + > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/hw/core/numa.c b/hw/core/numa.c > index 75db35ac19..756d243d3f 100644 > --- a/hw/core/numa.c > +++ b/hw/core/numa.c > @@ -580,9 +580,14 @@ void numa_complete_configuration(MachineState *ms) > * guest tries to use it with that drivers. > * > * Enable NUMA implicitly by adding a new NUMA node automatically. > + * > + * Or if MachineClass::auto_enable_numa is true and no NUMA nodes, > + * assume there is just one node with whole RAM. > */ > - if (ms->ram_slots > 0 && ms->numa_state->num_nodes == 0 && > - mc->auto_enable_numa_with_memhp) { > + if (ms->numa_state->num_nodes == 0 && > + ((ms->ram_slots > 0 && > + mc->auto_enable_numa_with_memhp) || > + mc->auto_enable_numa)) { > NumaNodeOptions node = { }; > parse_numa_node(ms, &node, &error_abort); > } > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f607ca567b..e50343f326 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -400,14 +400,6 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt) > hwaddr mem_start, node_size; > int i, nb_nodes = machine->numa_state->num_nodes; > NodeInfo *nodes = machine->numa_state->nodes; > - NodeInfo ramnode; > - > - /* No NUMA nodes, assume there is just one node with whole RAM */ > - if (!nb_nodes) { > - nb_nodes = 1; > - ramnode.node_mem = machine->ram_size; > - nodes = &ramnode; > - } > > for (i = 0, mem_start = 0; i < nb_nodes; ++i) { > if (!nodes[i].node_mem) { > @@ -4369,6 +4361,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > */ > mc->numa_mem_align_shift = 28; > mc->numa_mem_supported = true; > + mc->auto_enable_numa = true; > > smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF; > smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 2eb9a0b4e0..4a350b87d2 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -220,6 +220,7 @@ struct MachineClass { > bool smbus_no_migration_support; > bool nvdimm_supported; > bool numa_mem_supported; > + bool auto_enable_numa; > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev);
On 8/2/2019 2:55 PM, David Gibson wrote: > On Thu, Aug 01, 2019 at 03:52:58PM +0800, Tao Xu wrote: >> Introduce MachineClass::auto_enable_numa for one implicit NUMA node, >> and enable it to fix broken check in spapr_validate_node_memory(), when >> spapr_populate_memory() creates a implicit node and info then use >> nb_numa_nodes which is 0. >> >> Suggested-by: Igor Mammedov <imammedo@redhat.com> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Tao Xu <tao3.xu@intel.com> > > The change here looks fine so, > > Acked-by: David Gibson <david@gibson.dropbear.id.au> > > However, I'm not following what check in spapr is broken and why. > Sorry, may be I should update the commit message. Because in spapr_populate_memory(), if numa node is 0 if (!nb_nodes) { nb_nodes = 1; ramnode.node_mem = machine->ram_size; nodes = &ramnode; } it use a local 'nb_nodes' as 1 and update global nodes info, but inpapr_validate_node_memory(), use the global nb_numa_nodes for (i = 0; i < nb_numa_nodes; i++) { if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) { so the global is 0 and skip the node_mem check. >> --- >> >> This patch has a dependency on >> https://patchwork.kernel.org/cover/11063235/ >> --- >> hw/core/numa.c | 9 +++++++-- >> hw/ppc/spapr.c | 9 +-------- >> include/hw/boards.h | 1 + >> 3 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/hw/core/numa.c b/hw/core/numa.c >> index 75db35ac19..756d243d3f 100644 >> --- a/hw/core/numa.c >> +++ b/hw/core/numa.c >> @@ -580,9 +580,14 @@ void numa_complete_configuration(MachineState *ms) >> * guest tries to use it with that drivers. >> * >> * Enable NUMA implicitly by adding a new NUMA node automatically. >> + * >> + * Or if MachineClass::auto_enable_numa is true and no NUMA nodes, >> + * assume there is just one node with whole RAM. >> */ >> - if (ms->ram_slots > 0 && ms->numa_state->num_nodes == 0 && >> - mc->auto_enable_numa_with_memhp) { >> + if (ms->numa_state->num_nodes == 0 && >> + ((ms->ram_slots > 0 && >> + mc->auto_enable_numa_with_memhp) || >> + mc->auto_enable_numa)) { >> NumaNodeOptions node = { }; >> parse_numa_node(ms, &node, &error_abort); >> } >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index f607ca567b..e50343f326 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -400,14 +400,6 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt) >> hwaddr mem_start, node_size; >> int i, nb_nodes = machine->numa_state->num_nodes; >> NodeInfo *nodes = machine->numa_state->nodes; >> - NodeInfo ramnode; >> - >> - /* No NUMA nodes, assume there is just one node with whole RAM */ >> - if (!nb_nodes) { >> - nb_nodes = 1; >> - ramnode.node_mem = machine->ram_size; >> - nodes = &ramnode; >> - } >> >> for (i = 0, mem_start = 0; i < nb_nodes; ++i) { >> if (!nodes[i].node_mem) { >> @@ -4369,6 +4361,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) >> */ >> mc->numa_mem_align_shift = 28; >> mc->numa_mem_supported = true; >> + mc->auto_enable_numa = true; >> >> smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF; >> smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON; >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 2eb9a0b4e0..4a350b87d2 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -220,6 +220,7 @@ struct MachineClass { >> bool smbus_no_migration_support; >> bool nvdimm_supported; >> bool numa_mem_supported; >> + bool auto_enable_numa; >> >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >> DeviceState *dev); >
On Mon, Aug 05, 2019 at 08:56:40AM +0800, Tao Xu wrote: > On 8/2/2019 2:55 PM, David Gibson wrote: > > On Thu, Aug 01, 2019 at 03:52:58PM +0800, Tao Xu wrote: > > > Introduce MachineClass::auto_enable_numa for one implicit NUMA node, > > > and enable it to fix broken check in spapr_validate_node_memory(), when > > > spapr_populate_memory() creates a implicit node and info then use > > > nb_numa_nodes which is 0. > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > > Signed-off-by: Tao Xu <tao3.xu@intel.com> > > > > The change here looks fine so, > > > > Acked-by: David Gibson <david@gibson.dropbear.id.au> > > > > However, I'm not following what check in spapr is broken and why. > > > Sorry, may be I should update the commit message. > > Because in spapr_populate_memory(), if numa node is 0 > > if (!nb_nodes) { > nb_nodes = 1; > ramnode.node_mem = machine->ram_size; > nodes = &ramnode; > } > > it use a local 'nb_nodes' as 1 and update global nodes info, but > inpapr_validate_node_memory(), use the global nb_numa_nodes > > for (i = 0; i < nb_numa_nodes; i++) { > if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) { > > so the global is 0 and skip the node_mem check. Well, not really. That loop is that each node has memory size a multiple of 256MiB. But we've already checked that the whole memory size is a multiple of 256MiB, so in the case of one NUMA node, the per-node check doesn't actually do anything extra. And in the "non-NUMA" case, nb_numa_nodes == 0, then I don't believe numa_info[] is populated anyway, so we couldn't do the check like this. > > > --- > > > > > > This patch has a dependency on > > > https://patchwork.kernel.org/cover/11063235/ > > > --- > > > hw/core/numa.c | 9 +++++++-- > > > hw/ppc/spapr.c | 9 +-------- > > > include/hw/boards.h | 1 + > > > 3 files changed, 9 insertions(+), 10 deletions(-) > > > > > > diff --git a/hw/core/numa.c b/hw/core/numa.c > > > index 75db35ac19..756d243d3f 100644 > > > --- a/hw/core/numa.c > > > +++ b/hw/core/numa.c > > > @@ -580,9 +580,14 @@ void numa_complete_configuration(MachineState *ms) > > > * guest tries to use it with that drivers. > > > * > > > * Enable NUMA implicitly by adding a new NUMA node automatically. > > > + * > > > + * Or if MachineClass::auto_enable_numa is true and no NUMA nodes, > > > + * assume there is just one node with whole RAM. > > > */ > > > - if (ms->ram_slots > 0 && ms->numa_state->num_nodes == 0 && > > > - mc->auto_enable_numa_with_memhp) { > > > + if (ms->numa_state->num_nodes == 0 && > > > + ((ms->ram_slots > 0 && > > > + mc->auto_enable_numa_with_memhp) || > > > + mc->auto_enable_numa)) { > > > NumaNodeOptions node = { }; > > > parse_numa_node(ms, &node, &error_abort); > > > } > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index f607ca567b..e50343f326 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -400,14 +400,6 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt) > > > hwaddr mem_start, node_size; > > > int i, nb_nodes = machine->numa_state->num_nodes; > > > NodeInfo *nodes = machine->numa_state->nodes; > > > - NodeInfo ramnode; > > > - > > > - /* No NUMA nodes, assume there is just one node with whole RAM */ > > > - if (!nb_nodes) { > > > - nb_nodes = 1; > > > - ramnode.node_mem = machine->ram_size; > > > - nodes = &ramnode; > > > - } > > > for (i = 0, mem_start = 0; i < nb_nodes; ++i) { > > > if (!nodes[i].node_mem) { > > > @@ -4369,6 +4361,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > > > */ > > > mc->numa_mem_align_shift = 28; > > > mc->numa_mem_supported = true; > > > + mc->auto_enable_numa = true; > > > smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF; > > > smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON; > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > index 2eb9a0b4e0..4a350b87d2 100644 > > > --- a/include/hw/boards.h > > > +++ b/include/hw/boards.h > > > @@ -220,6 +220,7 @@ struct MachineClass { > > > bool smbus_no_migration_support; > > > bool nvdimm_supported; > > > bool numa_mem_supported; > > > + bool auto_enable_numa; > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > > DeviceState *dev); > > >
On 8/5/2019 10:58 AM, David Gibson wrote: > On Mon, Aug 05, 2019 at 08:56:40AM +0800, Tao Xu wrote: >> On 8/2/2019 2:55 PM, David Gibson wrote: >>> On Thu, Aug 01, 2019 at 03:52:58PM +0800, Tao Xu wrote: >>>> Introduce MachineClass::auto_enable_numa for one implicit NUMA node, >>>> and enable it to fix broken check in spapr_validate_node_memory(), when >>>> spapr_populate_memory() creates a implicit node and info then use >>>> nb_numa_nodes which is 0. >>>> >>>> Suggested-by: Igor Mammedov <imammedo@redhat.com> >>>> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >>>> Signed-off-by: Tao Xu <tao3.xu@intel.com> >>> >>> The change here looks fine so, >>> >>> Acked-by: David Gibson <david@gibson.dropbear.id.au> >>> >>> However, I'm not following what check in spapr is broken and why. >>> >> Sorry, may be I should update the commit message. >> >> Because in spapr_populate_memory(), if numa node is 0 >> >> if (!nb_nodes) { >> nb_nodes = 1; >> ramnode.node_mem = machine->ram_size; >> nodes = &ramnode; >> } >> >> it use a local 'nb_nodes' as 1 and update global nodes info, but >> inpapr_validate_node_memory(), use the global nb_numa_nodes >> >> for (i = 0; i < nb_numa_nodes; i++) { >> if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) { >> >> so the global is 0 and skip the node_mem check. > > Well, not really. That loop is that each node has memory size a > multiple of 256MiB. But we've already checked that the whole memory > size is a multiple of 256MiB, so in the case of one NUMA node, the > per-node check doesn't actually do anything extra. > > And in the "non-NUMA" case, nb_numa_nodes == 0, then I don't believe > numa_info[] is populated anyway, so we couldn't do the check like > this. > Thank you David. I understand. I will modify the commit message. So can I modify and keep this patch as a feature? Because it can reuse the generic numa code.
On Mon, Aug 05, 2019 at 11:37:14AM +0800, Tao Xu wrote: > On 8/5/2019 10:58 AM, David Gibson wrote: > > On Mon, Aug 05, 2019 at 08:56:40AM +0800, Tao Xu wrote: > > > On 8/2/2019 2:55 PM, David Gibson wrote: > > > > On Thu, Aug 01, 2019 at 03:52:58PM +0800, Tao Xu wrote: > > > > > Introduce MachineClass::auto_enable_numa for one implicit NUMA node, > > > > > and enable it to fix broken check in spapr_validate_node_memory(), when > > > > > spapr_populate_memory() creates a implicit node and info then use > > > > > nb_numa_nodes which is 0. > > > > > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > Signed-off-by: Tao Xu <tao3.xu@intel.com> > > > > > > > > The change here looks fine so, > > > > > > > > Acked-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > However, I'm not following what check in spapr is broken and why. > > > > > > > Sorry, may be I should update the commit message. > > > > > > Because in spapr_populate_memory(), if numa node is 0 > > > > > > if (!nb_nodes) { > > > nb_nodes = 1; > > > ramnode.node_mem = machine->ram_size; > > > nodes = &ramnode; > > > } > > > > > > it use a local 'nb_nodes' as 1 and update global nodes info, but > > > inpapr_validate_node_memory(), use the global nb_numa_nodes > > > > > > for (i = 0; i < nb_numa_nodes; i++) { > > > if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) { > > > > > > so the global is 0 and skip the node_mem check. > > > > Well, not really. That loop is that each node has memory size a > > multiple of 256MiB. But we've already checked that the whole memory > > size is a multiple of 256MiB, so in the case of one NUMA node, the > > per-node check doesn't actually do anything extra. > > > > And in the "non-NUMA" case, nb_numa_nodes == 0, then I don't believe > > numa_info[] is populated anyway, so we couldn't do the check like > > this. > > > Thank you David. I understand. I will modify the commit message. So can I > modify and keep this patch as a feature? Because it can reuse the generic > numa code. Yes, the patch itself looks fine, just the comment is misleading.
diff --git a/hw/core/numa.c b/hw/core/numa.c index 75db35ac19..756d243d3f 100644 --- a/hw/core/numa.c +++ b/hw/core/numa.c @@ -580,9 +580,14 @@ void numa_complete_configuration(MachineState *ms) * guest tries to use it with that drivers. * * Enable NUMA implicitly by adding a new NUMA node automatically. + * + * Or if MachineClass::auto_enable_numa is true and no NUMA nodes, + * assume there is just one node with whole RAM. */ - if (ms->ram_slots > 0 && ms->numa_state->num_nodes == 0 && - mc->auto_enable_numa_with_memhp) { + if (ms->numa_state->num_nodes == 0 && + ((ms->ram_slots > 0 && + mc->auto_enable_numa_with_memhp) || + mc->auto_enable_numa)) { NumaNodeOptions node = { }; parse_numa_node(ms, &node, &error_abort); } diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index f607ca567b..e50343f326 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -400,14 +400,6 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt) hwaddr mem_start, node_size; int i, nb_nodes = machine->numa_state->num_nodes; NodeInfo *nodes = machine->numa_state->nodes; - NodeInfo ramnode; - - /* No NUMA nodes, assume there is just one node with whole RAM */ - if (!nb_nodes) { - nb_nodes = 1; - ramnode.node_mem = machine->ram_size; - nodes = &ramnode; - } for (i = 0, mem_start = 0; i < nb_nodes; ++i) { if (!nodes[i].node_mem) { @@ -4369,6 +4361,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) */ mc->numa_mem_align_shift = 28; mc->numa_mem_supported = true; + mc->auto_enable_numa = true; smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF; smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON; diff --git a/include/hw/boards.h b/include/hw/boards.h index 2eb9a0b4e0..4a350b87d2 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -220,6 +220,7 @@ struct MachineClass { bool smbus_no_migration_support; bool nvdimm_supported; bool numa_mem_supported; + bool auto_enable_numa; HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev);
Introduce MachineClass::auto_enable_numa for one implicit NUMA node, and enable it to fix broken check in spapr_validate_node_memory(), when spapr_populate_memory() creates a implicit node and info then use nb_numa_nodes which is 0. Suggested-by: Igor Mammedov <imammedo@redhat.com> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Tao Xu <tao3.xu@intel.com> --- This patch has a dependency on https://patchwork.kernel.org/cover/11063235/ --- hw/core/numa.c | 9 +++++++-- hw/ppc/spapr.c | 9 +-------- include/hw/boards.h | 1 + 3 files changed, 9 insertions(+), 10 deletions(-)