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 <imamm...@redhat.com>
> > > Suggested-by: Eduardo Habkost <ehabk...@redhat.com>
> > > Signed-off-by: Tao Xu <tao3...@intel.com>
> >
> > The change here looks fine so,
> >
> > Acked-by: David Gibson <da...@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); > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
