[PATCH 1/4] conf: Introduce memory allocation threads

[Michal Prívozník]

On 3/24/22 10:26, Martin Kletzander wrote:
> On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote:
>> Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of
> 
> Anything wrong with the old "commit ffac16fab33b" ?

Not specifically no. But I do it this ways so that it's visible at the
first sight what QEMU version was this feature introduced in. And it
works with 'git show' too (for those who are interested in the
particular commit). IOW:

  "Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable .."

contains more information than:

  "Since its commit ffac16fab33b QEMU is capable .."

But I don't care that much. I agree that in this particular case it's a
bit messy, because QEMU doesn't have linear history. It's way nicer with
libvirt commits.

> 
>> specifying number of threads used to allocate memory. While it
>> defaults to the number of vCPUs, users might want to use a
>> different value (especially for humongous guests with gigantic
>> pages).
>>
>> In general, on QEMU cmd line level it is possible to use
>> different number of threads per each memory-backend-* object, in
>> practical terms it's not useful. Therefore, use <memoryBacking/>
>> to set guest wide value and let all memory devices 'inherit' it,
>> silently. IOW, don't introduce per device knob because that would
>> only complicate things for a little or no benefit.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> docs/formatdomain.rst                        |  6 ++++--
>> docs/schemas/domaincommon.rng                | 19 +++++++++++++------
>> src/conf/domain_conf.c                       | 15 ++++++++++++++-
>> src/conf/domain_conf.h                       |  1 +
>> tests/qemuxml2argvdata/memfd-memory-numa.xml |  2 +-
>> 5 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 9b1b69bb4d..8e25474db0 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -1004,7 +1004,7 @@ Memory Backing
>>        <locked/>
>>        <source type="file|anonymous|memfd"/>
>>        <access mode="shared|private"/>
>> -       <allocation mode="immediate|ondemand"/>
>> +       <allocation mode="immediate|ondemand" threads='8'/>
>>        <discard/>
>>      </memoryBacking>
>>      ...
>> @@ -1054,7 +1054,9 @@ influence how virtual memory pages are backed by
>> host pages.
>>    "private". This can be overridden per numa node by ``memAccess``.
>> ``allocation``
>>    Using the ``mode`` attribute, specify when to allocate the memory by
>> -   supplying either "immediate" or "ondemand".
>> +   supplying either "immediate" or "ondemand". :since:`Since 8.2.0` this
>> +   attribute is optional among with ``threads`` attribute, that sets
>> the number
>> +   of threads that hypervisor uses to allocate memory.
> 
> This is weird to read.  Just say "Using the optional mode attribute" and
> refer to threads as optional too.  If anyone wants to use just the
> allocation threads and leave out the mode they have to be on 8.2.0
> anyway, so no need to complicate things.

Alright, fair enough.

> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e0dfc9e45f..2414a806d0 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18914,6 +18914,13 @@ virDomainDefParseMemory(virDomainDef *def,
>>         VIR_FREE(tmp);
>>     }
>>
>> +    if (virXPathUInt("string(./memoryBacking/allocation/@threads)",
>> +                     ctxt, &def->mem.allocation_threads) == -2) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("Failed to parse memory allocation threads"));
>> +        return -1;
>> +    }
>> +
>>     if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
>>         /* hugepages will be used */
>>         if ((n = virXPathNodeSet("./memoryBacking/hugepages/page",
>> ctxt, &nodes)) < 0) {
>> @@ -27464,6 +27471,7 @@ virDomainMemorybackingFormat(virBuffer *buf,
>>                              const virDomainMemtune *mem)
>> {
>>     g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>> +    g_auto(virBuffer) allocAttrBuf = VIR_BUFFER_INITIALIZER;
>>
>>     if (mem->nhugepages)
>>         virDomainHugepagesFormat(&childBuf, mem->hugepages,
>> mem->nhugepages);
>> @@ -27478,8 +27486,13 @@ virDomainMemorybackingFormat(virBuffer *buf,
>>         virBufferAsprintf(&childBuf, "<access mode='%s'/>\n",
>>                          
>> virDomainMemoryAccessTypeToString(mem->access));
>>     if (mem->allocation)
>> -        virBufferAsprintf(&childBuf, "<allocation mode='%s'/>\n",
>> +        virBufferAsprintf(&allocAttrBuf, " mode='%s'",
>>                          
>> virDomainMemoryAllocationTypeToString(mem->allocation));
>> +    if (mem->allocation_threads)
> 
> Here you check if (mem->allocation_threads), but in 3/4 you check if
> (allocation_threads > 0), which is a bit inconsistent.  I prefer the
> former although I know we had some disputes with this, so pick whatever
> one and make it consistent.

D'oh! I remember I wanted to fix this. > 0 it is.

Michal