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

[Daniel Henrique Barboza]

On 8/20/20 1:15 AM, David Gibson wrote:
> 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.



To provide context, what I did here was inspired by this commit:

commit 0533ef5f2089f4f12a0ec5c8035e5e15ba0b5556
Author: Tao Xu <tao3.xu@intel.com>
Date:   Thu Sep 5 16:32:38 2019 +0800

    numa: Introduce MachineClass::auto_enable_numa for implicit NUMA node


In this commit, exclusive NUMA code from spapr.c was taken and put it
into numa.c, with a flag being set in spapr machine_init.


Thanks,


DHB


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