[libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune

[John Ferlan]

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> More info in the documentation, this is basically the XML parsing/formatting
> support, schemas, tests and documentation for the new cputune/cachetune element
> that will get used by following patches.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  docs/formatdomain.html.in                          |  54 +++++
>  docs/schemas/domaincommon.rng                      |  32 +++
>  src/conf/domain_conf.c                             | 251 +++++++++++++++++++++
>  src/conf/domain_conf.h                             |  13 ++
>  tests/genericxml2xmlindata/cachetune-cdp.xml       |  36 +++
>  .../cachetune-colliding-allocs.xml                 |  30 +++
>  .../cachetune-colliding-tunes.xml                  |  32 +++
>  .../cachetune-colliding-types.xml                  |  30 +++
>  tests/genericxml2xmlindata/cachetune-small.xml     |  29 +++
>  tests/genericxml2xmlindata/cachetune.xml           |  33 +++
>  tests/genericxml2xmltest.c                         |  10 +
>  11 files changed, 550 insertions(+)
>  create mode 100644 tests/genericxml2xmlindata/cachetune-cdp.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-types.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-small.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 01db83e60820..623860cc0e95 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -689,6 +689,10 @@
>      <iothread_quota>-1</iothread_quota>
>      <vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/>
>      <iothreadsched iothreads='2' scheduler='batch'/>
> +    <cachetune vcpus='0-3'>
> +      <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> +      <cache id='1' level='3' type='both' size='3' unit='MiB'/>
> +    </cachetune>
>    </cputune>
>    ...
>  </domain>
> @@ -834,6 +838,56 @@
>          <span class="since">Since 1.2.13</span>
>        </dd>
>  
> +      <dt><code>cachetune</code><span class="since">Since 3.10.0</span></dt>

At least 4.0.0 if not 4.1.0

> +      <dd>
> +        Optional <code>cachetune</code> element can control allocations for CPU
> +        caches using the resctrlfs on the host. Whether or not is this supported

s/resctrlfs/Resource Control File System/

Although this defaults to /sys/fs/resctrl, I'm not sure if we want to
state the default or not.  It *is* the path we will use though and I
don't believe we have a way to alter that default (yet).


> +        can be gathered from capabilities where some limitations like minimum
> +        size and required granularity are reported as well.  The required
> +        attribute <code>vcpus</code> specifies to which vCPUs this allocation
> +        applies. A vCPU can only be member of one <code>cachetune</code> element
> +        allocations. Supported subelements are:
> +        <dl>
> +          <dt><code>cache</code></dt>
> +          <dd>
> +            This element controls the allocation of CPU cache and has the
> +            following attributes:
> +            <dl>
> +              <dt><code>level</code></dt>
> +              <dd>
> +                Host cache level from which to allocate.
> +              </dd>
> +              <dt><code>id</code></dt>
> +              <dd>
> +                Host cache id from which to allocate.

Oh - should have read this first ;-)... Maybe rename @id internally to
hostCacheID... Shall I assume someone setting this up would know how to
determine how to get these values from the host?

> +              </dd>
> +              <dt><code>type</code></dt>
> +              <dd>
> +                Type of allocation.  Can be <code>code</code> for code
> +                (instructions), <code>data</code> for data or <code>both</code>
> +                for both code and data (unified).  Currently the allocation can
> +                be done only with the same type as the host supports, meaning
> +                you cannot request <code>both</code> for host with CDP
> +                (code/data prioritization) enabled.
> +              </dd>
> +              <dt><code>size</code></dt>
> +              <dd>
> +                The size of the region to allocate. The value by default is in
> +                bytes, but the <code>unit</code> attribute can be used to scale
> +                the value.
> +              </dd>
> +              <dt><code>unit</code> (optional)</dt>
> +              <dd>
> +                If specified it is the unit such as KiB, MiB, GiB, or TiB
> +                (described in the <code>memory</code> element
> +                for <a href="#elementsMemoryAllocation">Memory Allocation</a>)
> +                in which <code>size</code> is specified, defaults to bytes.
> +              </dd>
> +            </dl>
> +          </dd>
> +        </dl>
> +
> +      </dd>
>      </dl>
>  
>  

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bdbee..ec8d760c7971 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2883,6 +2883,17 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>      VIR_FREE(loader);
>  }
>  
> +static void
> +virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
> +{
> +    if (!cachetune)
> +        return;
> +
> +    virObjectUnref(cachetune->alloc);
> +    virBitmapFree(cachetune->vcpus);
> +    VIR_FREE(cachetune);
> +}
> +

Two empty lines before and after

>  void virDomainDefFree(virDomainDefPtr def)
>  {
>      size_t i;

[...]

>  
> +struct virCachetuneHelperData {
> +    virBufferPtr buf;
> +    size_t vcpu_id;
> +};
> +
> +static int
> +virDomainCachetuneDefFormatHelper(unsigned int level,
> +                                  virCacheType type,
> +                                  unsigned int cache,
> +                                  unsigned long long size,
> +                                  void *opaque)
> +{
> +    const char *unit;
> +    virBufferPtr buf = opaque;
> +    unsigned long long short_size = virFormatIntPretty(size, &unit);
> +
> +    virBufferAsprintf(buf,
> +                      "<cache id='%u' level='%u' type='%s' "
> +                      "size='%llu' unit='%s'/>\n",
> +                      cache, level, virCacheTypeToString(type),
> +                      short_size, unit);

A nit, but if the default is "B" and we don't require for parse, then do
we want to Format the output?  IOW: why print unit='B'

> +
> +    return 0;
> +}
> +
> +

[...]

> diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml b/tests/genericxml2xmlindata/cachetune-cdp.xml
> new file mode 100644
> index 000000000000..1331aad06e54
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml
> @@ -0,0 +1,36 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>2</vcpu>
> +  <cputune>
> +    <cachetune vcpus='0-1'>
> +      <cache id='0' level='3' type='code' size='7680' unit='KiB'/>
> +      <cache id='1' level='3' type='data' size='3840' unit='KiB'/>
> +    </cachetune>
> +    <cachetune vcpus='2'>
> +      <cache id='1' level='3' type='code' size='6' unit='MiB'/>
> +    </cachetune>
> +    <cachetune vcpus='3'>
> +      <cache id='1' level='3' type='data' size='6912' unit='KiB'/>
> +    </cachetune>

But there's only 2 vcpu's on this guest?

> +  </cputune>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>

[...]

> diff --git a/tests/genericxml2xmlindata/cachetune.xml b/tests/genericxml2xmlindata/cachetune.xml
> new file mode 100644
> index 000000000000..0051410aec32
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune.xml
> @@ -0,0 +1,33 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>2</vcpu>
> +  <cputune>
> +    <cachetune vcpus='0-1'>
> +      <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> +      <cache id='1' level='3' type='both' size='3' unit='MiB'/>
> +    </cachetune>
> +    <cachetune vcpus='3'>
> +      <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> +    </cachetune>

Another one with vcpusid > nvcpus defined above....

I'll give you my:

Reviewed-by: John Ferlan <jferlan at redhat.com>

Since adjustments are simple going forward. I'm not sure how you should
handle the vcpusid value > nvcpus if that's an issue or not...

John

> +  </cputune>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>