diff mbox series

[v2,82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

Message ID 1579100861-73692-83-git-send-email-imammedo@redhat.com
State New
Headers show
Series refactor main RAM allocation to use hostmem backend | expand

Commit Message

Igor Mammedov Jan. 15, 2020, 3:07 p.m. UTC
Deprecation period is ran out and it's a time to flip the switch
introduced by cd5ff8333a.
Disable legacy option for new machine types and amend documentation.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: ehabkost@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
CC: rth@twiddle.net
CC: david@gibson.dropbear.id.au
CC: libvir-list@redhat.com
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
---
 hw/arm/virt.c        |  2 +-
 hw/core/numa.c       |  6 ++++++
 hw/i386/pc.c         |  1 -
 hw/i386/pc_piix.c    |  1 +
 hw/i386/pc_q35.c     |  1 +
 hw/ppc/spapr.c       |  2 +-
 qemu-deprecated.texi | 16 ----------------
 qemu-options.hx      |  8 ++++----
 8 files changed, 14 insertions(+), 23 deletions(-)

Comments

Peter Krempa Jan. 15, 2020, 3:34 p.m. UTC | #1
On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
> Deprecation period is ran out and it's a time to flip the switch
> introduced by cd5ff8333a.
> Disable legacy option for new machine types and amend documentation.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: ehabkost@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: rth@twiddle.net
> CC: david@gibson.dropbear.id.au
> CC: libvir-list@redhat.com
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> ---
>  hw/arm/virt.c        |  2 +-
>  hw/core/numa.c       |  6 ++++++
>  hw/i386/pc.c         |  1 -
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  hw/ppc/spapr.c       |  2 +-
>  qemu-deprecated.texi | 16 ----------------
>  qemu-options.hx      |  8 ++++----
>  8 files changed, 14 insertions(+), 23 deletions(-)

I'm afraid nobody bothered to fix it yet:

https://bugzilla.redhat.com/show_bug.cgi?id=1783355
Igor Mammedov Jan. 15, 2020, 4:52 p.m. UTC | #2
On Wed, 15 Jan 2020 16:34:53 +0100
Peter Krempa <pkrempa@redhat.com> wrote:

> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
> > Deprecation period is ran out and it's a time to flip the switch
> > introduced by cd5ff8333a.
> > Disable legacy option for new machine types and amend documentation.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: peter.maydell@linaro.org
> > CC: ehabkost@redhat.com
> > CC: marcel.apfelbaum@gmail.com
> > CC: mst@redhat.com
> > CC: pbonzini@redhat.com
> > CC: rth@twiddle.net
> > CC: david@gibson.dropbear.id.au
> > CC: libvir-list@redhat.com
> > CC: qemu-arm@nongnu.org
> > CC: qemu-ppc@nongnu.org
> > ---
> >  hw/arm/virt.c        |  2 +-
> >  hw/core/numa.c       |  6 ++++++
> >  hw/i386/pc.c         |  1 -
> >  hw/i386/pc_piix.c    |  1 +
> >  hw/i386/pc_q35.c     |  1 +
> >  hw/ppc/spapr.c       |  2 +-
> >  qemu-deprecated.texi | 16 ----------------
> >  qemu-options.hx      |  8 ++++----
> >  8 files changed, 14 insertions(+), 23 deletions(-)  
> 
> I'm afraid nobody bothered to fix it yet:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1783355

It's time to start working on it :)
(looks like just deprecating stuff isn't sufficient motivation,
maybe actual switch flipping would work out better)
David Gibson Jan. 16, 2020, 4:36 a.m. UTC | #3
On Wed, Jan 15, 2020 at 04:07:37PM +0100, Igor Mammedov wrote:
> Deprecation period is ran out and it's a time to flip the switch
> introduced by cd5ff8333a.
> Disable legacy option for new machine types and amend documentation.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> CC: peter.maydell@linaro.org
> CC: ehabkost@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: rth@twiddle.net
> CC: david@gibson.dropbear.id.au
> CC: libvir-list@redhat.com
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> ---
>  hw/arm/virt.c        |  2 +-
>  hw/core/numa.c       |  6 ++++++
>  hw/i386/pc.c         |  1 -
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  hw/ppc/spapr.c       |  2 +-
>  qemu-deprecated.texi | 16 ----------------
>  qemu-options.hx      |  8 ++++----
>  8 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e2fbca3..49de0d8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2049,7 +2049,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      hc->pre_plug = virt_machine_device_pre_plug_cb;
>      hc->plug = virt_machine_device_plug_cb;
>      hc->unplug_request = virt_machine_device_unplug_request_cb;
> -    mc->numa_mem_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->default_ram_id = "mach-virt.ram";
>  }
> @@ -2153,6 +2152,7 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
>  static void virt_machine_4_2_options(MachineClass *mc)
>  {
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    mc->numa_mem_supported = true;
>  }
>  DEFINE_VIRT_MACHINE(4, 2)
>  
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 0970a30..3177066 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -117,6 +117,12 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>      }
>  
>      if (node->has_mem) {
> +        if (!mc->numa_mem_supported) {
> +            error_setg(errp, "Parameter -numa node,mem is not supported by this"
> +                      " machine type. Use -numa node,memdev instead");
> +            return;
> +        }
> +
>          numa_info[nodenr].node_mem = node->mem;
>          if (!qtest_enabled()) {
>              warn_report("Parameter -numa node,mem is deprecated,"
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 21b8290..fa8d024 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1947,7 +1947,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>      mc->nvdimm_supported = true;
> -    mc->numa_mem_supported = true;
>      mc->default_ram_id = "pc.ram";
>  
>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index fa12203..0a9b9e0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -435,6 +435,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
>      pc_i440fx_5_0_machine_options(m);
>      m->alias = NULL;
>      m->is_default = 0;
> +    m->numa_mem_supported = true;
>      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 84cf925..4d6e2be 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -363,6 +363,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
>  {
>      pc_q35_5_0_machine_options(m);
>      m->alias = NULL;
> +    m->numa_mem_supported = true;
>      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcbe1f1..2686b73 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4383,7 +4383,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       * in which LMBs are represented and hot-added
>       */
>      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;
> @@ -4465,6 +4464,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
>  {
>      spapr_machine_5_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    mc->numa_mem_supported = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 982af95..17a0e1d 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -89,22 +89,6 @@ error in the future.
>  The @code{-realtime mlock=on|off} argument has been replaced by the
>  @code{-overcommit mem-lock=on|off} argument.
>  
> -@subsection -numa node,mem=@var{size} (since 4.1)
> -
> -The parameter @option{mem} of @option{-numa node} is used to assign a part of
> -guest RAM to a NUMA node. But when using it, it's impossible to manage specified
> -RAM chunk on the host side (like bind it to a host node, setting bind policy, ...),
> -so guest end-ups with the fake NUMA configuration with suboptiomal performance.
> -However since 2014 there is an alternative way to assign RAM to a NUMA node
> -using parameter @option{memdev}, which does the same as @option{mem} and adds
> -means to actualy manage node RAM on the host side. Use parameter @option{memdev}
> -with @var{memory-backend-ram} backend as an replacement for parameter @option{mem}
> -to achieve the same fake NUMA effect or a properly configured
> -@var{memory-backend-file} backend to actually benefit from NUMA configuration.
> -In future new machine versions will not accept the option but it will still
> -work with old machine types. User can check QAPI schema to see if the legacy
> -option is supported by looking at MachineInfo::numa-mem-supported property.
> -
>  @subsection -numa node (without memory specified) (since 4.1)
>  
>  Splitting RAM by default between NUMA nodes has the same issues as @option{mem}
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 709162c..55500bd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -223,10 +223,10 @@ For example:
>  -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
>  @end example
>  
> -@samp{mem} assigns a given RAM amount to a node. @samp{memdev}
> -assigns RAM from a given memory backend device to a node. If
> -@samp{mem} and @samp{memdev} are omitted in all nodes, RAM is
> -split equally between them.
> +Legacy @samp{mem} assigns a given RAM amount to a node (not supported for 5.0
> +and newer machine types). @samp{memdev} assigns RAM from a given memory backend
> +device to a node. If @samp{mem} and @samp{memdev} are omitted in all nodes, RAM
> +is split equally between them.
>  
>  @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,
>  if one node uses @samp{memdev}, all of them have to use it.
Michal Prívozník Jan. 16, 2020, 10:42 a.m. UTC | #4
On 1/15/20 5:52 PM, Igor Mammedov wrote:
> On Wed, 15 Jan 2020 16:34:53 +0100
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
>>> Deprecation period is ran out and it's a time to flip the switch
>>> introduced by cd5ff8333a.
>>> Disable legacy option for new machine types and amend documentation.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> CC: peter.maydell@linaro.org
>>> CC: ehabkost@redhat.com
>>> CC: marcel.apfelbaum@gmail.com
>>> CC: mst@redhat.com
>>> CC: pbonzini@redhat.com
>>> CC: rth@twiddle.net
>>> CC: david@gibson.dropbear.id.au
>>> CC: libvir-list@redhat.com
>>> CC: qemu-arm@nongnu.org
>>> CC: qemu-ppc@nongnu.org
>>> ---
>>>   hw/arm/virt.c        |  2 +-
>>>   hw/core/numa.c       |  6 ++++++
>>>   hw/i386/pc.c         |  1 -
>>>   hw/i386/pc_piix.c    |  1 +
>>>   hw/i386/pc_q35.c     |  1 +
>>>   hw/ppc/spapr.c       |  2 +-
>>>   qemu-deprecated.texi | 16 ----------------
>>>   qemu-options.hx      |  8 ++++----
>>>   8 files changed, 14 insertions(+), 23 deletions(-)
>>
>> I'm afraid nobody bothered to fix it yet:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355
> 
> It's time to start working on it :)
> (looks like just deprecating stuff isn't sufficient motivation,
> maybe actual switch flipping would work out better)
> 

So how was the upgrade from older to newer version resolved? I mean, if 
the old qemu used -numa node,mem=XXX and it is migrated to a host with 
newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
can it? I'm asking because I've just started working on this.

Michal
Igor Mammedov Jan. 16, 2020, 12:37 p.m. UTC | #5
On Thu, 16 Jan 2020 11:42:09 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/15/20 5:52 PM, Igor Mammedov wrote:
> > On Wed, 15 Jan 2020 16:34:53 +0100
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> >>> Deprecation period is ran out and it's a time to flip the switch
> >>> introduced by cd5ff8333a.
> >>> Disable legacy option for new machine types and amend documentation.
> >>>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> CC: peter.maydell@linaro.org
> >>> CC: ehabkost@redhat.com
> >>> CC: marcel.apfelbaum@gmail.com
> >>> CC: mst@redhat.com
> >>> CC: pbonzini@redhat.com
> >>> CC: rth@twiddle.net
> >>> CC: david@gibson.dropbear.id.au
> >>> CC: libvir-list@redhat.com
> >>> CC: qemu-arm@nongnu.org
> >>> CC: qemu-ppc@nongnu.org
> >>> ---
> >>>   hw/arm/virt.c        |  2 +-
> >>>   hw/core/numa.c       |  6 ++++++
> >>>   hw/i386/pc.c         |  1 -
> >>>   hw/i386/pc_piix.c    |  1 +
> >>>   hw/i386/pc_q35.c     |  1 +
> >>>   hw/ppc/spapr.c       |  2 +-
> >>>   qemu-deprecated.texi | 16 ----------------
> >>>   qemu-options.hx      |  8 ++++----
> >>>   8 files changed, 14 insertions(+), 23 deletions(-)  
> >>
> >> I'm afraid nobody bothered to fix it yet:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> > 
> > It's time to start working on it :)
> > (looks like just deprecating stuff isn't sufficient motivation,
> > maybe actual switch flipping would work out better)
> >   
> 
> So how was the upgrade from older to newer version resolved? I mean, if 
> the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> can it? I'm asking because I've just started working on this.

see commit cd5ff8333a3c87 for detailed info.
Short answer is it's not really resolved [*],
-numa node,mem will keep working on newer QEMU but only for old machine types
new machine types will accept only -numa node,memdev.

One can check if "mem=' is supported by using QAPI query-machines
and checking numa-mem-supported field. That field is flipped to false
for 5.0 and later machine types in this patch.


*) I might give another try to removing 'mem' completely in migration
compatible manner but that's well beyond the scope of this series
So far I hasn't been able to convince myself that previous attempts
to do it were absolutely correct for all corner cases that are there.

> Michal
Michal Prívozník Jan. 16, 2020, 1:03 p.m. UTC | #6
On 1/16/20 1:37 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 11:42:09 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> On 1/15/20 5:52 PM, Igor Mammedov wrote:
>>> On Wed, 15 Jan 2020 16:34:53 +0100
>>> Peter Krempa <pkrempa@redhat.com> wrote:
>>>    
>>>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
>>>>> Deprecation period is ran out and it's a time to flip the switch
>>>>> introduced by cd5ff8333a.
>>>>> Disable legacy option for new machine types and amend documentation.
>>>>>
>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>> ---
>>>>> CC: peter.maydell@linaro.org
>>>>> CC: ehabkost@redhat.com
>>>>> CC: marcel.apfelbaum@gmail.com
>>>>> CC: mst@redhat.com
>>>>> CC: pbonzini@redhat.com
>>>>> CC: rth@twiddle.net
>>>>> CC: david@gibson.dropbear.id.au
>>>>> CC: libvir-list@redhat.com
>>>>> CC: qemu-arm@nongnu.org
>>>>> CC: qemu-ppc@nongnu.org
>>>>> ---
>>>>>    hw/arm/virt.c        |  2 +-
>>>>>    hw/core/numa.c       |  6 ++++++
>>>>>    hw/i386/pc.c         |  1 -
>>>>>    hw/i386/pc_piix.c    |  1 +
>>>>>    hw/i386/pc_q35.c     |  1 +
>>>>>    hw/ppc/spapr.c       |  2 +-
>>>>>    qemu-deprecated.texi | 16 ----------------
>>>>>    qemu-options.hx      |  8 ++++----
>>>>>    8 files changed, 14 insertions(+), 23 deletions(-)
>>>>
>>>> I'm afraid nobody bothered to fix it yet:
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355
>>>
>>> It's time to start working on it :)
>>> (looks like just deprecating stuff isn't sufficient motivation,
>>> maybe actual switch flipping would work out better)
>>>    
>>
>> So how was the upgrade from older to newer version resolved? I mean, if
>> the old qemu used -numa node,mem=XXX and it is migrated to a host with
>> newer qemu, the cmd line can't be switched to -numa node,memdev=node0,
>> can it? I'm asking because I've just started working on this.
> 
> see commit cd5ff8333a3c87 for detailed info.
> Short answer is it's not really resolved [*],
> -numa node,mem will keep working on newer QEMU but only for old machine types
> new machine types will accept only -numa node,memdev.
> 
> One can check if "mem=' is supported by using QAPI query-machines
> and checking numa-mem-supported field. That field is flipped to false
> for 5.0 and later machine types in this patch.

Alright, so what we can do is the following:

1) For new machine types (pc-5.0/q35-5.0 and newer) use memdev= always.

2) For older machine types, we are stuck with mem= until qemu is capable 
of migrating from mem= to memdev=

I think this is a safe thing to do since migrating from one version of a 
machine type to another is not supported (since it can change guest 
ABI). And we will see how much 2) bothers us. Does this sound reasonable?

Michal
Daniel P. Berrangé Jan. 16, 2020, 1:06 p.m. UTC | #7
On Thu, Jan 16, 2020 at 01:37:03PM +0100, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 11:42:09 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
> > On 1/15/20 5:52 PM, Igor Mammedov wrote:
> > > On Wed, 15 Jan 2020 16:34:53 +0100
> > > Peter Krempa <pkrempa@redhat.com> wrote:
> > >   
> > >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> > >>> Deprecation period is ran out and it's a time to flip the switch
> > >>> introduced by cd5ff8333a.
> > >>> Disable legacy option for new machine types and amend documentation.
> > >>>
> > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > >>> ---
> > >>> CC: peter.maydell@linaro.org
> > >>> CC: ehabkost@redhat.com
> > >>> CC: marcel.apfelbaum@gmail.com
> > >>> CC: mst@redhat.com
> > >>> CC: pbonzini@redhat.com
> > >>> CC: rth@twiddle.net
> > >>> CC: david@gibson.dropbear.id.au
> > >>> CC: libvir-list@redhat.com
> > >>> CC: qemu-arm@nongnu.org
> > >>> CC: qemu-ppc@nongnu.org
> > >>> ---
> > >>>   hw/arm/virt.c        |  2 +-
> > >>>   hw/core/numa.c       |  6 ++++++
> > >>>   hw/i386/pc.c         |  1 -
> > >>>   hw/i386/pc_piix.c    |  1 +
> > >>>   hw/i386/pc_q35.c     |  1 +
> > >>>   hw/ppc/spapr.c       |  2 +-
> > >>>   qemu-deprecated.texi | 16 ----------------
> > >>>   qemu-options.hx      |  8 ++++----
> > >>>   8 files changed, 14 insertions(+), 23 deletions(-)  
> > >>
> > >> I'm afraid nobody bothered to fix it yet:
> > >>
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> > > 
> > > It's time to start working on it :)
> > > (looks like just deprecating stuff isn't sufficient motivation,
> > > maybe actual switch flipping would work out better)
> > >   
> > 
> > So how was the upgrade from older to newer version resolved? I mean, if 
> > the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> > newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> > can it? I'm asking because I've just started working on this.
> 
> see commit cd5ff8333a3c87 for detailed info.
> Short answer is it's not really resolved [*],
> -numa node,mem will keep working on newer QEMU but only for old machine types
> new machine types will accept only -numa node,memdev.
> 
> One can check if "mem=' is supported by using QAPI query-machines
> and checking numa-mem-supported field. That field is flipped to false
> for 5.0 and later machine types in this patch.

Since libvirt droppped the ball here, can we postpone this change
to machine types until a later release. 


Regards,
Daniel
Igor Mammedov Jan. 16, 2020, 1:49 p.m. UTC | #8
On Thu, 16 Jan 2020 14:03:12 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/16/20 1:37 PM, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 11:42:09 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> On 1/15/20 5:52 PM, Igor Mammedov wrote:  
> >>> On Wed, 15 Jan 2020 16:34:53 +0100
> >>> Peter Krempa <pkrempa@redhat.com> wrote:
> >>>      
> >>>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> >>>>> Deprecation period is ran out and it's a time to flip the switch
> >>>>> introduced by cd5ff8333a.
> >>>>> Disable legacy option for new machine types and amend documentation.
> >>>>>
> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>>> ---
> >>>>> CC: peter.maydell@linaro.org
> >>>>> CC: ehabkost@redhat.com
> >>>>> CC: marcel.apfelbaum@gmail.com
> >>>>> CC: mst@redhat.com
> >>>>> CC: pbonzini@redhat.com
> >>>>> CC: rth@twiddle.net
> >>>>> CC: david@gibson.dropbear.id.au
> >>>>> CC: libvir-list@redhat.com
> >>>>> CC: qemu-arm@nongnu.org
> >>>>> CC: qemu-ppc@nongnu.org
> >>>>> ---
> >>>>>    hw/arm/virt.c        |  2 +-
> >>>>>    hw/core/numa.c       |  6 ++++++
> >>>>>    hw/i386/pc.c         |  1 -
> >>>>>    hw/i386/pc_piix.c    |  1 +
> >>>>>    hw/i386/pc_q35.c     |  1 +
> >>>>>    hw/ppc/spapr.c       |  2 +-
> >>>>>    qemu-deprecated.texi | 16 ----------------
> >>>>>    qemu-options.hx      |  8 ++++----
> >>>>>    8 files changed, 14 insertions(+), 23 deletions(-)  
> >>>>
> >>>> I'm afraid nobody bothered to fix it yet:
> >>>>
> >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> >>>
> >>> It's time to start working on it :)
> >>> (looks like just deprecating stuff isn't sufficient motivation,
> >>> maybe actual switch flipping would work out better)
> >>>      
> >>
> >> So how was the upgrade from older to newer version resolved? I mean, if
> >> the old qemu used -numa node,mem=XXX and it is migrated to a host with
> >> newer qemu, the cmd line can't be switched to -numa node,memdev=node0,
> >> can it? I'm asking because I've just started working on this.  
> > 
> > see commit cd5ff8333a3c87 for detailed info.
> > Short answer is it's not really resolved [*],
> > -numa node,mem will keep working on newer QEMU but only for old machine types
> > new machine types will accept only -numa node,memdev.
> > 
> > One can check if "mem=' is supported by using QAPI query-machines
> > and checking numa-mem-supported field. That field is flipped to false
> > for 5.0 and later machine types in this patch.  
> 
> Alright, so what we can do is the following:
> 
> 1) For new machine types (pc-5.0/q35-5.0 and newer) use memdev= always.
it's not only x86, it's for all machines that support numa
hence numa-mem-supported was introduced to make it easier for libvirt
to figure out when to use which syntax.

The plan was to release libvirt with support for numa-mem-supported and
then when newer QEMU forbids 'mem=' it change will be transparent for
relatively fresh livirt.

Whether it still does make sense though.

We could go with your suggestion in which case libvirt unilaterally
switches to using only 'memdev' for 5.0 machine types and then later
(5.1..) we release QEMU that enforces it.
In this case we can axe numa-mem-supported (I'd volunteer) to avoid
supporting yet another ABI/smart logic where your way could be sufficient.

Daniel,
what's your take on Michal's approach?

> 2) For older machine types, we are stuck with mem= until qemu is capable 
> of migrating from mem= to memdev=
> 
> I think this is a safe thing to do since migrating from one version of a 
> machine type to another is not supported (since it can change guest 
> ABI). And we will see how much 2) bothers us. Does this sound reasonable?\


> 
> Michal
> 
>
Igor Mammedov Jan. 16, 2020, 1:58 p.m. UTC | #9
On Thu, 16 Jan 2020 13:06:28 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Jan 16, 2020 at 01:37:03PM +0100, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 11:42:09 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> > > On 1/15/20 5:52 PM, Igor Mammedov wrote:  
> > > > On Wed, 15 Jan 2020 16:34:53 +0100
> > > > Peter Krempa <pkrempa@redhat.com> wrote:
> > > >     
> > > >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:    
> > > >>> Deprecation period is ran out and it's a time to flip the switch
> > > >>> introduced by cd5ff8333a.
> > > >>> Disable legacy option for new machine types and amend documentation.
> > > >>>
> > > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > >>> ---
> > > >>> CC: peter.maydell@linaro.org
> > > >>> CC: ehabkost@redhat.com
> > > >>> CC: marcel.apfelbaum@gmail.com
> > > >>> CC: mst@redhat.com
> > > >>> CC: pbonzini@redhat.com
> > > >>> CC: rth@twiddle.net
> > > >>> CC: david@gibson.dropbear.id.au
> > > >>> CC: libvir-list@redhat.com
> > > >>> CC: qemu-arm@nongnu.org
> > > >>> CC: qemu-ppc@nongnu.org
> > > >>> ---
> > > >>>   hw/arm/virt.c        |  2 +-
> > > >>>   hw/core/numa.c       |  6 ++++++
> > > >>>   hw/i386/pc.c         |  1 -
> > > >>>   hw/i386/pc_piix.c    |  1 +
> > > >>>   hw/i386/pc_q35.c     |  1 +
> > > >>>   hw/ppc/spapr.c       |  2 +-
> > > >>>   qemu-deprecated.texi | 16 ----------------
> > > >>>   qemu-options.hx      |  8 ++++----
> > > >>>   8 files changed, 14 insertions(+), 23 deletions(-)    
> > > >>
> > > >> I'm afraid nobody bothered to fix it yet:
> > > >>
> > > >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355    
> > > > 
> > > > It's time to start working on it :)
> > > > (looks like just deprecating stuff isn't sufficient motivation,
> > > > maybe actual switch flipping would work out better)
> > > >     
> > > 
> > > So how was the upgrade from older to newer version resolved? I mean, if 
> > > the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> > > newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> > > can it? I'm asking because I've just started working on this.  
> > 
> > see commit cd5ff8333a3c87 for detailed info.
> > Short answer is it's not really resolved [*],
> > -numa node,mem will keep working on newer QEMU but only for old machine types
> > new machine types will accept only -numa node,memdev.
> > 
> > One can check if "mem=' is supported by using QAPI query-machines
> > and checking numa-mem-supported field. That field is flipped to false
> > for 5.0 and later machine types in this patch.  
> 
> Since libvirt droppped the ball here, can we postpone this change
> to machine types until a later release. 

Looks like we have to at this point.
We can do this for [82-86/86] patches which are mostly numa
related changes.

The rest could go in this release as it is in-depended of
numa, it mainly introduces memdev backend for main RAM
and consolidates twisted main RAM allocation logic.

> 
> Regards,
> Daniel
diff mbox series

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e2fbca3..49de0d8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2049,7 +2049,6 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->pre_plug = virt_machine_device_pre_plug_cb;
     hc->plug = virt_machine_device_plug_cb;
     hc->unplug_request = virt_machine_device_unplug_request_cb;
-    mc->numa_mem_supported = true;
     mc->auto_enable_numa_with_memhp = true;
     mc->default_ram_id = "mach-virt.ram";
 }
@@ -2153,6 +2152,7 @@  DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
 static void virt_machine_4_2_options(MachineClass *mc)
 {
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    mc->numa_mem_supported = true;
 }
 DEFINE_VIRT_MACHINE(4, 2)
 
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 0970a30..3177066 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -117,6 +117,12 @@  static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
     }
 
     if (node->has_mem) {
+        if (!mc->numa_mem_supported) {
+            error_setg(errp, "Parameter -numa node,mem is not supported by this"
+                      " machine type. Use -numa node,memdev instead");
+            return;
+        }
+
         numa_info[nodenr].node_mem = node->mem;
         if (!qtest_enabled()) {
             warn_report("Parameter -numa node,mem is deprecated,"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 21b8290..fa8d024 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1947,7 +1947,6 @@  static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
     mc->nvdimm_supported = true;
-    mc->numa_mem_supported = true;
     mc->default_ram_id = "pc.ram";
 
     object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fa12203..0a9b9e0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,6 +435,7 @@  static void pc_i440fx_4_2_machine_options(MachineClass *m)
     pc_i440fx_5_0_machine_options(m);
     m->alias = NULL;
     m->is_default = 0;
+    m->numa_mem_supported = true;
     compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 84cf925..4d6e2be 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -363,6 +363,7 @@  static void pc_q35_4_2_machine_options(MachineClass *m)
 {
     pc_q35_5_0_machine_options(m);
     m->alias = NULL;
+    m->numa_mem_supported = true;
     compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bcbe1f1..2686b73 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4383,7 +4383,6 @@  static void spapr_machine_class_init(ObjectClass *oc, void *data)
      * in which LMBs are represented and hot-added
      */
     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;
@@ -4465,6 +4464,7 @@  static void spapr_machine_4_2_class_options(MachineClass *mc)
 {
     spapr_machine_5_0_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    mc->numa_mem_supported = true;
 }
 
 DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 982af95..17a0e1d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -89,22 +89,6 @@  error in the future.
 The @code{-realtime mlock=on|off} argument has been replaced by the
 @code{-overcommit mem-lock=on|off} argument.
 
-@subsection -numa node,mem=@var{size} (since 4.1)
-
-The parameter @option{mem} of @option{-numa node} is used to assign a part of
-guest RAM to a NUMA node. But when using it, it's impossible to manage specified
-RAM chunk on the host side (like bind it to a host node, setting bind policy, ...),
-so guest end-ups with the fake NUMA configuration with suboptiomal performance.
-However since 2014 there is an alternative way to assign RAM to a NUMA node
-using parameter @option{memdev}, which does the same as @option{mem} and adds
-means to actualy manage node RAM on the host side. Use parameter @option{memdev}
-with @var{memory-backend-ram} backend as an replacement for parameter @option{mem}
-to achieve the same fake NUMA effect or a properly configured
-@var{memory-backend-file} backend to actually benefit from NUMA configuration.
-In future new machine versions will not accept the option but it will still
-work with old machine types. User can check QAPI schema to see if the legacy
-option is supported by looking at MachineInfo::numa-mem-supported property.
-
 @subsection -numa node (without memory specified) (since 4.1)
 
 Splitting RAM by default between NUMA nodes has the same issues as @option{mem}
diff --git a/qemu-options.hx b/qemu-options.hx
index 709162c..55500bd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -223,10 +223,10 @@  For example:
 -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
 @end example
 
-@samp{mem} assigns a given RAM amount to a node. @samp{memdev}
-assigns RAM from a given memory backend device to a node. If
-@samp{mem} and @samp{memdev} are omitted in all nodes, RAM is
-split equally between them.
+Legacy @samp{mem} assigns a given RAM amount to a node (not supported for 5.0
+and newer machine types). @samp{memdev} assigns RAM from a given memory backend
+device to a node. If @samp{mem} and @samp{memdev} are omitted in all nodes, RAM
+is split equally between them.
 
 @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,
 if one node uses @samp{memdev}, all of them have to use it.