Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
.dynamic-memslots attribute for virtio-mem-pci devices. When
turned on, it allows memory exposed to guest to be split into
multiple memslots and thus smaller memory footprint (see the
original commit for detailed explanation).

Therefore, introduce new <target/> attribute which will control
that QEMU knob.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/formatdomain.rst                          |  6 ++++++
 src/conf/domain_conf.c                         | 18 +++++++++++++++++-
 src/conf/domain_conf.h                         |  1 +
 src/conf/schemas/domaincommon.rng              |  5 +++++
 .../memory-hotplug-virtio-mem.xml              |  2 +-
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 96e03a3807..57974de9f4 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -8395,6 +8395,12 @@ Example: usage of the memory devices
    The ``node`` subelement configures the guest NUMA node to attach the memory
    to. The element shall be used only if the guest has NUMA nodes configured.
 
+   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
+   (accepted values "yes"/"no") which allows hypervisor to spread memory into
+   multiple memory slots (allocate them dynamically based on the amount of
+   memory exposed to the guest), resulting in smaller memory footprint.
+   :since:`Since 10.0.0 and QEMU 8.2.0`
+
    The following optional elements may be used:
 
    ``label``
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 39e9879a8a..e677023b17 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13499,6 +13499,10 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
                                  &def->target.virtio_mem.requestedsize, false, false) < 0)
             return -1;
 
+        if (virXMLPropTristateBool(node, "dynamicMemslots", VIR_XML_PROP_NONE,
+                                   &def->target.virtio_mem.dynamicMemslots) < 0)
+            return -1;
+
         addrNode = virXPathNode("./address", ctxt);
         addr = &def->target.virtio_mem.address;
         break;
@@ -21173,6 +21177,12 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDef *src,
                            src->target.virtio_mem.address);
             return false;
         }
+
+        if (src->target.virtio_mem.dynamicMemslots != dst->target.virtio_mem.dynamicMemslots) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Target memory device 'dynamicMemslots' property doesn't match source memory device"));
+            return false;
+        }
         break;
 
     case VIR_DOMAIN_MEMORY_MODEL_DIMM:
@@ -25374,6 +25384,7 @@ virDomainMemoryTargetDefFormat(virBuffer *buf,
                                unsigned int flags)
 {
     g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+    g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
 
     virBufferAsprintf(&childBuf, "<size unit='KiB'>%llu</size>\n", def->size);
     if (def->targetNode >= 0)
@@ -25413,6 +25424,11 @@ virDomainMemoryTargetDefFormat(virBuffer *buf,
         if (def->target.virtio_mem.address)
             virBufferAsprintf(&childBuf, "<address base='0x%llx'/>\n",
                               def->target.virtio_mem.address);
+
+        if (def->target.virtio_mem.dynamicMemslots) {
+            virBufferAsprintf(&attrBuf, " dynamicMemslots='%s'",
+                              virTristateBoolTypeToString(def->target.virtio_mem.dynamicMemslots));
+        }
         break;
 
     case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
@@ -25422,7 +25438,7 @@ virDomainMemoryTargetDefFormat(virBuffer *buf,
         break;
     }
 
-    virXMLFormatElement(buf, "target", NULL, &childBuf);
+    virXMLFormatElement(buf, "target", &attrBuf, &childBuf);
 }
 
 static int
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 58b2c92f4c..88ff3e1d84 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2674,6 +2674,7 @@ struct _virDomainMemoryDef {
             unsigned long long currentsize; /* kibibytes, valid for an active
                                                domain only and parsed */
             unsigned long long address; /* address where memory is mapped */
+            virTristateBool dynamicMemslots;
         } virtio_mem;
         struct {
         } sgx_epc;
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index f318c06797..08c408e138 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -7260,6 +7260,11 @@
 
   <define name="memorydev-target">
     <element name="target">
+      <optional>
+        <attribute name="dynamicMemslots">
+          <ref name="virYesNo"/>
+        </attribute>
+      </optional>
       <interleave>
         <element name="size">
           <ref name="scaledInteger"/>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml
index c578209d8a..359ece7a77 100644
--- a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml
+++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml
@@ -60,7 +60,7 @@
         <nodemask>1-3</nodemask>
         <pagesize unit='KiB'>2048</pagesize>
       </source>
-      <target>
+      <target dynamicMemslots='yes'>
         <size unit='KiB'>2097152</size>
         <node>0</node>
         <block unit='KiB'>2048</block>
-- 
2.41.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
> .dynamic-memslots attribute for virtio-mem-pci devices. When
> turned on, it allows memory exposed to guest to be split into
> multiple memslots and thus smaller memory footprint (see the
> original commit for detailed explanation).
> 
> Therefore, introduce new <target/> attribute which will control
> that QEMU knob.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/formatdomain.rst                          |  6 ++++++
>  src/conf/domain_conf.c                         | 18 +++++++++++++++++-
>  src/conf/domain_conf.h                         |  1 +
>  src/conf/schemas/domaincommon.rng              |  5 +++++
>  .../memory-hotplug-virtio-mem.xml              |  2 +-
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 96e03a3807..57974de9f4 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
>     The ``node`` subelement configures the guest NUMA node to attach the memory
>     to. The element shall be used only if the guest has NUMA nodes configured.
>  
> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
> +   multiple memory slots (allocate them dynamically based on the amount of
> +   memory exposed to the guest), resulting in smaller memory footprint.
> +   :since:`Since 10.0.0 and QEMU 8.2.0`
> +

Except for the docs here, which are insufficient as outlined in the
other subthread, the code looks good, so for the code:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 1/10/24 12:59, Peter Krempa wrote:
> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
>> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
>> .dynamic-memslots attribute for virtio-mem-pci devices. When
>> turned on, it allows memory exposed to guest to be split into
>> multiple memslots and thus smaller memory footprint (see the
>> original commit for detailed explanation).
>>
>> Therefore, introduce new <target/> attribute which will control
>> that QEMU knob.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  docs/formatdomain.rst                          |  6 ++++++
>>  src/conf/domain_conf.c                         | 18 +++++++++++++++++-
>>  src/conf/domain_conf.h                         |  1 +
>>  src/conf/schemas/domaincommon.rng              |  5 +++++
>>  .../memory-hotplug-virtio-mem.xml              |  2 +-
>>  5 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 96e03a3807..57974de9f4 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
>>     The ``node`` subelement configures the guest NUMA node to attach the memory
>>     to. The element shall be used only if the guest has NUMA nodes configured.
>>  
>> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
>> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
>> +   multiple memory slots (allocate them dynamically based on the amount of
>> +   memory exposed to the guest), resulting in smaller memory footprint.
>> +   :since:`Since 10.0.0 and QEMU 8.2.0`
>> +
> 
> Except for the docs here, which are insufficient as outlined in the
> other subthread, the code looks good, so for the code:

How about this:

   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
   (accepted values "yes"/"no") which allows hypervisor to spread memory into
   multiple memory slots (allocate them dynamically based on the amount of
   memory exposed to the guest), resulting in smaller memory footprint. But be
   aware this may affect vhost-user devices. When enabled, older vhost-user
   devices may refuse to initialize resulting in failed domain startup or
   device hotplug (set this to "no" then). On the other hand, modern vhost-user
   devices, or when no vhost-user devices are present (nor they will be
   hotplugged), this leads to smaller memory footprint (set this to "yes"
   then). The current default is hypervisor dependant (for QEMU is "no"). If
   the default changes and you are having difficulties with vhost-user devices,
   try toggling this to "no". :since:`Since 10.1.0 and QEMU 8.2.0`


> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

Thank you!

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Jan 10, 2024 at 15:54:18 +0100, Michal Prívozník wrote:
> On 1/10/24 12:59, Peter Krempa wrote:
> > On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> >> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
> >> .dynamic-memslots attribute for virtio-mem-pci devices. When
> >> turned on, it allows memory exposed to guest to be split into
> >> multiple memslots and thus smaller memory footprint (see the
> >> original commit for detailed explanation).
> >>
> >> Therefore, introduce new <target/> attribute which will control
> >> that QEMU knob.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  docs/formatdomain.rst                          |  6 ++++++
> >>  src/conf/domain_conf.c                         | 18 +++++++++++++++++-
> >>  src/conf/domain_conf.h                         |  1 +
> >>  src/conf/schemas/domaincommon.rng              |  5 +++++
> >>  .../memory-hotplug-virtio-mem.xml              |  2 +-
> >>  5 files changed, 30 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> >> index 96e03a3807..57974de9f4 100644
> >> --- a/docs/formatdomain.rst
> >> +++ b/docs/formatdomain.rst
> >> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
> >>     The ``node`` subelement configures the guest NUMA node to attach the memory
> >>     to. The element shall be used only if the guest has NUMA nodes configured.
> >>  
> >> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
> >> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
> >> +   multiple memory slots (allocate them dynamically based on the amount of
> >> +   memory exposed to the guest), resulting in smaller memory footprint.
> >> +   :since:`Since 10.0.0 and QEMU 8.2.0`
> >> +
> > 
> > Except for the docs here, which are insufficient as outlined in the
> > other subthread, the code looks good, so for the code:
> 
> How about this:
> 
>    For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
>    (accepted values "yes"/"no") which allows hypervisor to spread memory into
>    multiple memory slots (allocate them dynamically based on the amount of
>    memory exposed to the guest), resulting in smaller memory footprint. But be
>    aware this may affect vhost-user devices. When enabled, older vhost-user

I'd add: 'vhost-user devices (such as virtiofsd)'.

Optionally you can add more, but virtiofsd is the one users can
configure without being aware of it.a


also perhaps 'vhost-user device implementations' for the next sentence.

>    devices may refuse to initialize resulting in failed domain startup or
>    device hotplug (set this to "no" then). On the other hand, modern vhost-user

IMO the stuff in the parentheses can be dropped.


>    devices, or when no vhost-user devices are present (nor they will be
>    hotplugged), this leads to smaller memory footprint (set this to "yes"

the memory footprint note is redundant from the first sentence.

How about:

When only modern vhost-user based devices will be used or when no
vhost-user devices are expected to be used it's beneficial to enable
this feature.

>    then). The current default is hypervisor dependant (for QEMU is "no"). If
>    the default changes and you are having difficulties with vhost-user devices,
>    try toggling this to "no". :since:`Since 10.1.0 and QEMU 8.2.0`
> 
> 
> > 
> > Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> > 
> 
> Thank you!
> 
> Michal
> 
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
> .dynamic-memslots attribute for virtio-mem-pci devices. When
> turned on, it allows memory exposed to guest to be split into
> multiple memslots and thus smaller memory footprint (see the
> original commit for detailed explanation).
> 
> Therefore, introduce new <target/> attribute which will control
> that QEMU knob.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/formatdomain.rst                          |  6 ++++++
>  src/conf/domain_conf.c                         | 18 +++++++++++++++++-
>  src/conf/domain_conf.h                         |  1 +
>  src/conf/schemas/domaincommon.rng              |  5 +++++
>  .../memory-hotplug-virtio-mem.xml              |  2 +-
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 96e03a3807..57974de9f4 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
>     The ``node`` subelement configures the guest NUMA node to attach the memory
>     to. The element shall be used only if the guest has NUMA nodes configured.
>  
> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
> +   multiple memory slots (allocate them dynamically based on the amount of
> +   memory exposed to the guest), resulting in smaller memory footprint.
> +   :since:`Since 10.0.0 and QEMU 8.2.0`

Is there any drawback in always setting 'yes'? (modulo cases when it
would not work, obviously)

Disclaimer, I didn't read the qemu commit, but IMO this is an important
information to see how we should approach this feature and libvirt users
won't have the pointer to the qemu commit here in the docs.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 05.01.24 13:40, Peter Krempa wrote:
> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
>> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
>> .dynamic-memslots attribute for virtio-mem-pci devices. When
>> turned on, it allows memory exposed to guest to be split into
>> multiple memslots and thus smaller memory footprint (see the
>> original commit for detailed explanation).
>>
>> Therefore, introduce new <target/> attribute which will control
>> that QEMU knob.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   docs/formatdomain.rst                          |  6 ++++++
>>   src/conf/domain_conf.c                         | 18 +++++++++++++++++-
>>   src/conf/domain_conf.h                         |  1 +
>>   src/conf/schemas/domaincommon.rng              |  5 +++++
>>   .../memory-hotplug-virtio-mem.xml              |  2 +-
>>   5 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 96e03a3807..57974de9f4 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
>>      The ``node`` subelement configures the guest NUMA node to attach the memory
>>      to. The element shall be used only if the guest has NUMA nodes configured.
>>   
>> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
>> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
>> +   multiple memory slots (allocate them dynamically based on the amount of
>> +   memory exposed to the guest), resulting in smaller memory footprint.
>> +   :since:`Since 10.0.0 and QEMU 8.2.0`
> 
> Is there any drawback in always setting 'yes'? (modulo cases when it
> would not work, obviously)

In combination with some vhost devices (especially vhost-user), starting 
QEMU might fail until these devices support more memslots. There is 
ongoing work to improve these vhost-user devices.

To make these setups temporarily work, one would have to manually set 
"dynamic-memslots=off" here, which is not too bad I guess.

Besides that, I don't think there is a real drawback -- there are mostly 
only benefits of having it :)

I'm actually hoping that we can default-enable it, at least in RHEL soon.

Maybe we can default-enable it right away and document that vhost-user 
devices will require it to be manually disabled until they support more 
memslots?

-- 
Cheers,

David / dhildenb
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
> On 05.01.24 13:40, Peter Krempa wrote:
> > On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> > > Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
> > > .dynamic-memslots attribute for virtio-mem-pci devices. When
> > > turned on, it allows memory exposed to guest to be split into
> > > multiple memslots and thus smaller memory footprint (see the
> > > original commit for detailed explanation).
> > > 
> > > Therefore, introduce new <target/> attribute which will control
> > > that QEMU knob.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   docs/formatdomain.rst                          |  6 ++++++
> > >   src/conf/domain_conf.c                         | 18 +++++++++++++++++-
> > >   src/conf/domain_conf.h                         |  1 +
> > >   src/conf/schemas/domaincommon.rng              |  5 +++++
> > >   .../memory-hotplug-virtio-mem.xml              |  2 +-
> > >   5 files changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > index 96e03a3807..57974de9f4 100644
> > > --- a/docs/formatdomain.rst
> > > +++ b/docs/formatdomain.rst
> > > @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
> > >      The ``node`` subelement configures the guest NUMA node to attach the memory
> > >      to. The element shall be used only if the guest has NUMA nodes configured.
> > > +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
> > > +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
> > > +   multiple memory slots (allocate them dynamically based on the amount of
> > > +   memory exposed to the guest), resulting in smaller memory footprint.
> > > +   :since:`Since 10.0.0 and QEMU 8.2.0`
> > 
> > Is there any drawback in always setting 'yes'? (modulo cases when it
> > would not work, obviously)
> 
> In combination with some vhost devices (especially vhost-user), starting
> QEMU might fail until these devices support more memslots. There is ongoing
> work to improve these vhost-user devices.
> 
> To make these setups temporarily work, one would have to manually set
> "dynamic-memslots=off" here, which is not too bad I guess.
> 
> Besides that, I don't think there is a real drawback -- there are mostly
> only benefits of having it :)
> 
> I'm actually hoping that we can default-enable it, at least in RHEL soon.
> 
> Maybe we can default-enable it right away and document that vhost-user
> devices will require it to be manually disabled until they support more
> memslots?

This series is very specifically adding it only for virtio-mem, which
AFAIK doesn't have any vhost-user variant, thus having this as a
configurable knob is almost pointless.

The only real reason to have it is to ensure ABI compatibility between
configs and when migrating, but ideally should be completely hidden if
there's no need for users to disable it. (libvirt can do that but it's a
bit less straightforward)

Ideally for any kind of options which are ABI incompatible but there's
no real reason for users to disable them, qemu should add them as a
machine type property (to ensure ABI stability) and just enable them by
default.

QEMU would need to bind them to a machine type anyways once changing the
default.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 05.01.24 14:58, Peter Krempa wrote:
> On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
>> On 05.01.24 13:40, Peter Krempa wrote:
>>> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
>>>> Introduced in v8.2.0-rc0~74^2~2, QEMU now allows setting
>>>> .dynamic-memslots attribute for virtio-mem-pci devices. When
>>>> turned on, it allows memory exposed to guest to be split into
>>>> multiple memslots and thus smaller memory footprint (see the
>>>> original commit for detailed explanation).
>>>>
>>>> Therefore, introduce new <target/> attribute which will control
>>>> that QEMU knob.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>    docs/formatdomain.rst                          |  6 ++++++
>>>>    src/conf/domain_conf.c                         | 18 +++++++++++++++++-
>>>>    src/conf/domain_conf.h                         |  1 +
>>>>    src/conf/schemas/domaincommon.rng              |  5 +++++
>>>>    .../memory-hotplug-virtio-mem.xml              |  2 +-
>>>>    5 files changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>> index 96e03a3807..57974de9f4 100644
>>>> --- a/docs/formatdomain.rst
>>>> +++ b/docs/formatdomain.rst
>>>> @@ -8395,6 +8395,12 @@ Example: usage of the memory devices
>>>>       The ``node`` subelement configures the guest NUMA node to attach the memory
>>>>       to. The element shall be used only if the guest has NUMA nodes configured.
>>>> +   For ``virtio-mem`` optional attribute ``dynamicMemslots`` can be specified
>>>> +   (accepted values "yes"/"no") which allows hypervisor to spread memory into
>>>> +   multiple memory slots (allocate them dynamically based on the amount of
>>>> +   memory exposed to the guest), resulting in smaller memory footprint.
>>>> +   :since:`Since 10.0.0 and QEMU 8.2.0`
>>>
>>> Is there any drawback in always setting 'yes'? (modulo cases when it
>>> would not work, obviously)
>>
>> In combination with some vhost devices (especially vhost-user), starting
>> QEMU might fail until these devices support more memslots. There is ongoing
>> work to improve these vhost-user devices.
>>
>> To make these setups temporarily work, one would have to manually set
>> "dynamic-memslots=off" here, which is not too bad I guess.
>>
>> Besides that, I don't think there is a real drawback -- there are mostly
>> only benefits of having it :)
>>
>> I'm actually hoping that we can default-enable it, at least in RHEL soon.
>>
>> Maybe we can default-enable it right away and document that vhost-user
>> devices will require it to be manually disabled until they support more
>> memslots?
> 
> This series is very specifically adding it only for virtio-mem, which
> AFAIK doesn't have any vhost-user variant, thus having this as a
> configurable knob is almost pointless.

No, that problem is the combination. Assume you have VM with both 
virtio-mem to hotplug memory and virtio-user-fs+virtiofsd.

The memory that virtio-mem provides has to be usable by 
virtio-user-fs+virtiofsd. Therefore, we have to map that shared memory 
(provided by virtio-mem) into the virtiofsd process.

However, virtiofsd only supports 32 memslots and virito-mem wants to use 
up to 256 (like having 256 DIMMs). virtiofsd support is in the works but 
not upstream yet.

So if you start a vm with

  -device virtio-mem-pci,dynamic-memslots=on,...
  -device virtio-user-fs-pci, ...

QEMU might reject to start if the virtio-user-fs-pci backend has 
insufficient memory slots.

In that case (and only then), we want to disable dynamic memslots. 
Without the virtio-user complication (that I hate), this wouldn't even 
be a configurable toggle.

> 
> The only real reason to have it is to ensure ABI compatibility between
> configs and when migrating, but ideally should be completely hidden if
> there's no need for users to disable it. (libvirt can do that but it's a
> bit less straightforward)
> 
> Ideally for any kind of options which are ABI incompatible but there's
> no real reason for users to disable them, qemu should add them as a
> machine type property (to ensure ABI stability) and just enable them by
> default.
> 
> QEMU would need to bind them to a machine type anyways once changing the
> default.

Yes.

-- 
Cheers,

David / dhildenb
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 18:30:56 +0100, David Hildenbrand wrote:
> On 05.01.24 14:58, Peter Krempa wrote:
> > On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
> > > On 05.01.24 13:40, Peter Krempa wrote:
> > > > On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:

[...]

> > This series is very specifically adding it only for virtio-mem, which
> > AFAIK doesn't have any vhost-user variant, thus having this as a
> > configurable knob is almost pointless.
> 
> No, that problem is the combination. Assume you have VM with both virtio-mem
> to hotplug memory and virtio-user-fs+virtiofsd.
> 
> The memory that virtio-mem provides has to be usable by
> virtio-user-fs+virtiofsd. Therefore, we have to map that shared memory
> (provided by virtio-mem) into the virtiofsd process.
> 
> However, virtiofsd only supports 32 memslots and virito-mem wants to use up
> to 256 (like having 256 DIMMs). virtiofsd support is in the works but not
> upstream yet.
> 
> So if you start a vm with
> 
>  -device virtio-mem-pci,dynamic-memslots=on,...
>  -device virtio-user-fs-pci, ...
> 
> QEMU might reject to start if the virtio-user-fs-pci backend has
> insufficient memory slots.
> 
> In that case (and only then), we want to disable dynamic memslots. Without
> the virtio-user complication (that I hate), this wouldn't even be a
> configurable toggle.

Ah I see. So we can't simply enable it by default because AND not even
automatically:

1) It would break existing configs (but solvable before startup)
2) it would break hotplug into VMs (when hotplugging e.g. virtiofs)

This means that if we'd want to implement it now it MUST be a
configurable toggle.

Note that libvirt will have to carry this code forever even once qemu
enables this by default and thus the toggle in libvirt becomes
effectively pointless. Thus if this will be a short-term workaround I'd
really welcome if we'd re-consider doing this.

If the benefits from enabling the feature, and the timeline of
auto-enabling it in qemu are such that we want to implement the toggle
in libvirt for the time being, this patch needs to document (in
formatdomain.rst, not the commit message the following):

1) the fact that it's beneficial to enable it
2) which setup it could block (mentioning also hotplug)
3) that qemu will auto-enable the feature once it's fully available

Having this toggle without any of the above will result in basically
nobody enabling it.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 08.01.24 09:38, Peter Krempa wrote:
> On Fri, Jan 05, 2024 at 18:30:56 +0100, David Hildenbrand wrote:
>> On 05.01.24 14:58, Peter Krempa wrote:
>>> On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
>>>> On 05.01.24 13:40, Peter Krempa wrote:
>>>>> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> 
> [...]
> 
>>> This series is very specifically adding it only for virtio-mem, which
>>> AFAIK doesn't have any vhost-user variant, thus having this as a
>>> configurable knob is almost pointless.
>>
>> No, that problem is the combination. Assume you have VM with both virtio-mem
>> to hotplug memory and virtio-user-fs+virtiofsd.
>>
>> The memory that virtio-mem provides has to be usable by
>> virtio-user-fs+virtiofsd. Therefore, we have to map that shared memory
>> (provided by virtio-mem) into the virtiofsd process.
>>
>> However, virtiofsd only supports 32 memslots and virito-mem wants to use up
>> to 256 (like having 256 DIMMs). virtiofsd support is in the works but not
>> upstream yet.
>>
>> So if you start a vm with
>>
>>   -device virtio-mem-pci,dynamic-memslots=on,...
>>   -device virtio-user-fs-pci, ...
>>
>> QEMU might reject to start if the virtio-user-fs-pci backend has
>> insufficient memory slots.
>>
>> In that case (and only then), we want to disable dynamic memslots. Without
>> the virtio-user complication (that I hate), this wouldn't even be a
>> configurable toggle.
> 
> Ah I see. So we can't simply enable it by default because AND not even
> automatically:
> 
> 1) It would break existing configs (but solvable before startup)

Right. One could keep the toggle disabled for old QEMU machines, or 
disable it if any vhost-user device is configured as well. yes, that is 
more complicated and better avoided.

> 2) it would break hotplug into VMs (when hotplugging e.g. virtiofs)

Right, although not a common setup, especially not with virtio-mem.

> 
> This means that if we'd want to implement it now it MUST be a
> configurable toggle.
> 
> Note that libvirt will have to carry this code forever even once qemu
> enables this by default and thus the toggle in libvirt becomes
> effectively pointless. Thus if this will be a short-term workaround I'd
> really welcome if we'd re-consider doing this.

I assume that in the long-term everything based on the vhost-user rust 
crate will become compatible. And at this point, we primarily only care 
about virtiofsd with virito-mem.


> If the benefits from enabling the feature, and the timeline of
> auto-enabling it in qemu are such that we want to implement the toggle
> in libvirt for the time being, this patch needs to document (in
> formatdomain.rst, not the commit message the following):
> 
> 1) the fact that it's beneficial to enable it
> 2) which setup it could block (mentioning also hotplug)
> 3) that qemu will auto-enable the feature once it's fully available
> 
> Having this toggle without any of the above will result in basically
> nobody enabling it.

I'm hoping that we can enable it always in RHEL where we can control 
which version of vortiofsd we ship, and can better control what we 
actually support.

-- 
Cheers,

David / dhildenb
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 1/8/24 10:00, David Hildenbrand wrote:
> On 08.01.24 09:38, Peter Krempa wrote:
>> On Fri, Jan 05, 2024 at 18:30:56 +0100, David Hildenbrand wrote:
>>> On 05.01.24 14:58, Peter Krempa wrote:
>>>> On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
>>>>> On 05.01.24 13:40, Peter Krempa wrote:
>>>>>> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
>>
>> [...]
>>

So what's the resolution here? IIUC, QEMU will enable this by default
and keep it off for older machine types. If that's the case, we don't
need these after all. Or am I misunderstanding something?

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Jan 10, 2024 at 12:36:58 +0100, Michal Prívozník wrote:
> On 1/8/24 10:00, David Hildenbrand wrote:
> > On 08.01.24 09:38, Peter Krempa wrote:
> >> On Fri, Jan 05, 2024 at 18:30:56 +0100, David Hildenbrand wrote:
> >>> On 05.01.24 14:58, Peter Krempa wrote:
> >>>> On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
> >>>>> On 05.01.24 13:40, Peter Krempa wrote:
> >>>>>> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
> >>
> >> [...]
> >>
> 
> So what's the resolution here? IIUC, QEMU will enable this by default
> and keep it off for older machine types. If that's the case, we don't
> need these after all. Or am I misunderstanding something?

So, at this point automatically enabling that feature is not possible as
it makes the VM incompatible with 'vhost-user' devices, including ones
hotplugged in the future.

Based on the fact that it's about 'vhost-user' qemu won't be ever 100%
sure that the possibly hotplugged device is modern enough to be
compatible, as it depends on other software such as virtiofsd.

As of such we will most likely need the flag as a workaround for when
qemu decides to enable it, in case users want to use older software.

Regarding this patchset, the documentation needs to be improved to
clearly state the specific details about what users are supposed to,
including outlining the workaround it provides:

 - if users don't want vhost-user devices (now, or hotplug) -> enable it
 - if users have modern vhost-user devices -> enable it
 - for now the default value is: disabled, but qemu might want to
   auto-enable it in the future
 - if there's a problem in the future once it's auto-enabled, disable it
   explicitly if you have problems with vhost-user (e.g. virtiofs)
   devices.

With the clear guidance on when to set it the flag is acceptable as it
sometimes requires policy decision (such as user wants to hotplug
virtiofs, which is not yet fixed) which we can't make automatically
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 10.01.24 12:50, Peter Krempa wrote:
> On Wed, Jan 10, 2024 at 12:36:58 +0100, Michal Prívozník wrote:
>> On 1/8/24 10:00, David Hildenbrand wrote:
>>> On 08.01.24 09:38, Peter Krempa wrote:
>>>> On Fri, Jan 05, 2024 at 18:30:56 +0100, David Hildenbrand wrote:
>>>>> On 05.01.24 14:58, Peter Krempa wrote:
>>>>>> On Fri, Jan 05, 2024 at 14:34:53 +0100, David Hildenbrand wrote:
>>>>>>> On 05.01.24 13:40, Peter Krempa wrote:
>>>>>>>> On Fri, Jan 05, 2024 at 13:33:32 +0100, Michal Privoznik wrote:
>>>>
>>>> [...]
>>>>
>>
>> So what's the resolution here? IIUC, QEMU will enable this by default
>> and keep it off for older machine types. If that's the case, we don't
>> need these after all. Or am I misunderstanding something?
> 
> So, at this point automatically enabling that feature is not possible as
> it makes the VM incompatible with 'vhost-user' devices, including ones
> hotplugged in the future.
> 
> Based on the fact that it's about 'vhost-user' qemu won't be ever 100%
> sure that the possibly hotplugged device is modern enough to be
> compatible, as it depends on other software such as virtiofsd.
> 
> As of such we will most likely need the flag as a workaround for when
> qemu decides to enable it, in case users want to use older software.

Agreed with all.

-- 
Cheers,

David / dhildenb
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org