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

[Igor Mammedov]

On Thu, 20 Aug 2020 07:33:00 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

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

We have too many auto_enable_numa*, we probably should merge them into
just one auto_enable_numa if it's possible.

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

If it's guest limitiation, then it doesn't sound like QEMU material to me.
It would be more appropriate to validate at mgmt level which knows what
guest is being used.

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