Re: [Qemu-devel] [RFC PATCH] NUMA: Enable adding NUMA node implicitly

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 21 Sep 2017 12:19:17 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> I am sorry I missed some comments you gave to me.
> 
> my reply is below.
> At 09/18/2017 05:24 PM, Dou Liyang wrote:
> [...]
> >>> ranges where
> >>>   *    the guest will attempt to probe for a device that QEMU doesn't
> >>>   *    implement and a stub device is required.
> >>> + * @numa_implicit_add_node0:
> >>> + *    Enable NUMA implicitly by add a NUMA node.  
> >> how about:
> >> s/auto_enable_numa_with_memhp/  
> 
> Yes, really better than me, will do it.
> 
> >> boolean instead, see below how it could improve patch.
> >>  
> 
> I am not really sure why do we want to make this function boolean.
> 
> >>>   */
> >>>  struct MachineClass {
> >>>      /*< private >*/
> >>> @@ -191,6 +193,8 @@ struct MachineClass {
> >>>      CpuInstanceProperties
> >>> (*cpu_index_to_instance_props)(MachineState *machine,
> >>>                                                           unsigned
> >>> cpu_index);
> >>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState
> >>> *machine);
> >>> +
> >>> +    void (*numa_implicit_add_node0)(void);
> >>>  };
> >>>
> >>>  /**
> >>> diff --git a/vl.c b/vl.c
> >>> index fb1f05b..814a5fa 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -3030,6 +3030,7 @@ int main(int argc, char **argv, char **envp)
> >>>      Error *main_loop_err = NULL;
> >>>      Error *err = NULL;
> >>>      bool list_data_dirs = false;
> >>> +    bool has_numa_config_in_CLI = false;
> >>>      typedef struct BlockdevOptions_queue {
> >>>          BlockdevOptions *bdo;
> >>>          Location loc;
> >>> @@ -3293,6 +3294,7 @@ int main(int argc, char **argv, char **envp)
> >>>                  if (!opts) {
> >>>                      exit(1);
> >>>                  }
> >>> +                has_numa_config_in_CLI = true;
> >>>                  break;
> >>>              case QEMU_OPTION_display:
> >>>                  display_type = select_display(optarg);
> >>> @@ -4585,6 +4587,18 @@ int main(int argc, char **argv, char **envp)
> >>>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> >>>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> >>>
> >>> +    /*
> >>> +     * If memory hotplug is enabled i.e. slots > 0 and user hasn't add
> >>> +     * NUMA nodes explicitly on CLI
> >>> +     *
> >>> +     * Enable NUMA implicitly for guest to know the maximum memory
> >>> +     * from ACPI SRAT table, which is used for SWIOTLB.
> >>> +     */
> >>> +    if (ram_slots > 0 && !has_numa_config_in_CLI) {
> >>> +        if (machine_class->numa_implicit_add_node0) {
> >>> +            machine_class->numa_implicit_add_node0();
> >>> +        }
> >>> +    }
> >>>      parse_numa_opts(current_machine);  
> >> it would be better to put this logic inside of parse_numa_opts()
> >> I'd suggest to move:
> >>
> >>     current_machine->ram_size = ram_size;
> >>     current_machine->maxram_size = maxram_size;
> >>     current_machine->ram_slots = ram_slots;
> >>
> >> before parse_numa_opts() is called, and then
> >> handle 'memhp present+no numa on CLI" logic inside of
> >> parse_numa_opts(). With this you won't have to track
> >> 'has_numa_config_in_CLI', drop callback numa_implicit_add_node0()
> >> and numa nuances would be in place they are supposed to be: numa.c
> >>  
> 
> Is "dropping the callback..." means :
> 
>      static void auto_enable_numa_with_memhp(QemuOptsList *list)
>      {
>          ...
>      }
> 
>      void parse_numa_opts(MachineState *ms, uint64_t ram_slots)
>      {
>          QemuOptsList *numa_opts = qemu_find_opts("numa");
>          ...
>          auto_enable_numa_with_memhp(numa_opts);
>          ...
>      }
> 
> So, No matter what arch it is, if it support NUMA, we will enable NUMA
> implicitly when it has already enabled memory hotplug by 
> "slot=xx,maxmem=xx" CLI explicitly.
> 
> I am not sure that, but this bug only affects x86 as I know, seems no
> need to affect other arches which support NUMA as well.

I've meant something like that:

parse_numa_opts() {
    if (mc->auto_enable_numa_with_memhp == true) {
        qemu_opts_parse_noisily(qemu_find_opts("numa"), "node", true);
    }
}

where x86 sets mc->auto_enable_numa_with_memhp to true by default
and sets it to false for 2.10 and older machine types.
It will allow individual machines to enable feature if they need it
but prevent guest ABI breakage for old machine types
(i.e. won't break migration)

grep source for 'pcmc->legacy_cpu_hotplug = true' to see how
this compat stuff works.

> 
> Thanks,
> 	dou.
> >>>
> >>>      if (qemu_opts_foreach(qemu_find_opts("mon"),  
> >>
> >>
> >>
> >>  
> 
>