Re: [PATCH 02/10] numa: introduce MachineClass::forbid_asymmetrical_numa

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]

On Wed, Aug 19, 2020 at 10:11:28PM -0400, Eduardo Habkost wrote:
> On Thu, Aug 20, 2020 at 11:17:26AM +1000, David Gibson wrote:
> > On Fri, Aug 14, 2020 at 05:54:16PM -0300, Daniel Henrique Barboza wrote:
> > > The pSeries machine does not support asymmetrical NUMA
> > > configurations.
> > 
> > This seems a bit oddly specific to have as a global machine class
> > property.
> > 
> > Would it make more sense for machines with specific NUMA constraints
> > to just verify those during their initialization?
> 
> This would be much simpler.  However, I like the idea of
> representing machine-specific configuration validation rules as
> data that can eventually be exported to management software.

Ah, ok, so basically the usual tradeoff between flexibility and
advertisability.

So, in that case, I guess the question is whether we envisage "no
assymmetry" as a constraint common enough that it's worth creating an
advertisable rule or not.  If we only ever have one user, then we
haven't really done any better than hard coding the constraint in the
manageent software.

Of course to complicate matters, in the longer term we're looking at
removing that constraint from pseries - but doing so will be dependent
on the guest kernel understanding a new format for the NUMA
information in the device tree.  So qemu alone won't have enough
information to tell if such a configuration is possible or not.

> (CCing John Snow, who had spent some time thinking about
> configuration validation recently.)
> 
> 
> > > 
> > > CC: Eduardo Habkost <ehabkost@redhat.com>
> > > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >  hw/core/numa.c      | 7 +++++++
> > >  hw/ppc/spapr.c      | 1 +
> > >  include/hw/boards.h | 1 +
> > >  3 files changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/core/numa.c b/hw/core/numa.c
> > > index d1a94a14f8..1e81233c1d 100644
> > > --- a/hw/core/numa.c
> > > +++ b/hw/core/numa.c
> > > @@ -547,6 +547,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > >   */
> > >  static void validate_numa_distance(MachineState *ms)
> > >  {
> > > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > >      int src, dst;
> > >      bool is_asymmetrical = false;
> > >      int nb_numa_nodes = ms->numa_state->num_nodes;
> > > @@ -575,6 +576,12 @@ static void validate_numa_distance(MachineState *ms)
> > >      }
> > >  
> > >      if (is_asymmetrical) {
> > > +        if (mc->forbid_asymmetrical_numa) {
> > > +            error_report("This machine type does not support "
> > > +                         "asymmetrical numa distances.");
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +
> > >          for (src = 0; src < nb_numa_nodes; src++) {
> > >              for (dst = 0; dst < nb_numa_nodes; dst++) {
> > >                  if (src != dst && numa_info[src].distance[dst] == 0) {
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index dd2fa4826b..3b16edaf4c 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -4512,6 +4512,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >       */
> > >      mc->numa_mem_align_shift = 28;
> > >      mc->auto_enable_numa = true;
> > > +    mc->forbid_asymmetrical_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 bc5b82ad20..dc6cdd1c53 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -215,6 +215,7 @@ struct MachineClass {
> > >      bool nvdimm_supported;
> > >      bool numa_mem_supported;
> > >      bool auto_enable_numa;
> > > +    bool forbid_asymmetrical_numa;
> > >      const char *default_ram_id;
> > >  
> > >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > 
> 
> 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]