1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/9] Yet another version of CAT stuff (no idea about the version number)
  4. View patch

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

Martin Kletzander posted 9 patches 7 years, 5 months ago
[libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune
Posted by Martin Kletzander 7 years, 5 months ago 
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@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 @@
     &lt;iothread_quota&gt;-1&lt;/iothread_quota&gt;
     &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt;
     &lt;iothreadsched iothreads='2' scheduler='batch'/&gt;
+    &lt;cachetune vcpus='0-3'&gt;
+      &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt;
+      &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt;
+    &lt;/cachetune&gt;
   &lt;/cputune&gt;
   ...
 &lt;/domain&gt;
@@ -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>
+      <dd>
+        Optional <code>cachetune</code> element can control allocations for CPU
+        caches using the resctrlfs on the host. Whether or not is this supported
+        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.
+              </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/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f22c932f6c09..252f58f4379c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -900,6 +900,38 @@
             <ref name="schedparam"/>
           </element>
         </zeroOrMore>
+        <zeroOrMore>
+          <element name="cachetune">
+            <attribute name="vcpus">
+              <ref name='cpuset'/>
+            </attribute>
+            <oneOrMore>
+              <element name="cache">
+                <attribute name="id">
+                  <ref name='unsignedInt'/>
+                </attribute>
+                <attribute name="level">
+                  <ref name='unsignedInt'/>
+                </attribute>
+                <attribute name="type">
+                  <choice>
+                    <value>both</value>
+                    <value>code</value>
+                    <value>data</value>
+                  </choice>
+                </attribute>
+                <attribute name="size">
+                  <ref name='unsignedLong'/>
+                </attribute>
+                <optional>
+                  <attribute name='unit'>
+                    <ref name='unit'/>
+                  </attribute>
+                </optional>
+              </element>
+            </oneOrMore>
+          </element>
+        </zeroOrMore>
       </interleave>
     </element>
   </define>
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);
+}
+
 void virDomainDefFree(virDomainDefPtr def)
 {
     size_t i;
@@ -3055,6 +3066,10 @@ void virDomainDefFree(virDomainDefPtr def)
         virDomainShmemDefFree(def->shmems[i]);
     VIR_FREE(def->shmems);
 
+    for (i = 0; i < def->ncachetunes; i++)
+        virDomainCachetuneDefFree(def->cachetunes[i]);
+    VIR_FREE(def->cachetunes);
+
     VIR_FREE(def->keywrap);
 
     if (def->namespaceData && def->ns.free)
@@ -18202,6 +18217,165 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
 }
 
 
+static int
+virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
+                                xmlNodePtr node,
+                                virResctrlAllocPtr alloc)
+{
+    xmlNodePtr oldnode = ctxt->node;
+    unsigned int level;
+    unsigned int cache;
+    int type;
+    unsigned long long size;
+    char *tmp = NULL;
+    int ret = -1;
+
+    ctxt->node = node;
+
+    tmp = virXMLPropString(node, "id");
+    if (!tmp) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Missing cachetune attribute 'id'"));
+        goto cleanup;
+    }
+    if (virStrToLong_uip(tmp, NULL, 10, &cache) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'id' value '%s'"),
+                       tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    tmp = virXMLPropString(node, "level");
+    if (!tmp) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Missing cachetune attribute 'level'"));
+        goto cleanup;
+    }
+    if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'level' value '%s'"),
+                       tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    tmp = virXMLPropString(node, "type");
+    if (!tmp) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Missing cachetune attribute 'type'"));
+        goto cleanup;
+    }
+    type = virCacheTypeFromString(tmp);
+    if (type < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'type' value '%s'"),
+                       tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    if (virDomainParseScaledValue("./@size", "./@unit",
+                                  ctxt, &size, 1024,
+                                  ULLONG_MAX, true) < 0)
+        goto cleanup;
+
+    if (virResctrlAllocSetSize(alloc, level, type, cache, size) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    ctxt->node = oldnode;
+    VIR_FREE(tmp);
+    return ret;
+}
+
+
+static int
+virDomainCachetuneDefParse(virDomainDefPtr def,
+                           xmlXPathContextPtr ctxt,
+                           xmlNodePtr node)
+{
+    xmlNodePtr oldnode = ctxt->node;
+    xmlNodePtr *nodes = NULL;
+    virBitmapPtr vcpus = NULL;
+    virResctrlAllocPtr alloc = virResctrlAllocNew();
+    virDomainCachetuneDefPtr tmp_cachetune = NULL;
+    char *tmp = NULL;
+    char *vcpus_str = NULL;
+    ssize_t i = 0;
+    int n;
+    int ret = -1;
+
+    ctxt->node = node;
+
+    if (!alloc)
+        goto cleanup;
+
+    if (VIR_ALLOC(tmp_cachetune) < 0)
+        goto cleanup;
+
+    vcpus_str = virXMLPropString(node, "vcpus");
+    if (!vcpus_str) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Missing cachetune attribute 'vcpus'"));
+        goto cleanup;
+    }
+    if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'vcpus' value '%s'"),
+                       vcpus_str);
+        goto cleanup;
+    }
+
+    if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Cannot extract cache nodes under cachetune"));
+        goto cleanup;
+    }
+
+    for (i = 0; i < n; i++) {
+        if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
+            goto cleanup;
+    }
+
+    if (virResctrlAllocIsEmpty(alloc)) {
+        ret = 0;
+        goto cleanup;
+    }
+
+    virBitmapShrink(vcpus, def->maxvcpus);
+
+    for (i = 0; i < def->ncachetunes; i++) {
+        if (virBitmapOverlaps(def->cachetunes[i]->vcpus, vcpus)) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Overlapping vcpus in cachetunes"));
+            goto cleanup;
+        }
+    }
+
+    if (virResctrlAllocSetID(alloc, vcpus_str) < 0)
+        goto cleanup;
+
+    VIR_STEAL_PTR(tmp_cachetune->vcpus, vcpus);
+    VIR_STEAL_PTR(tmp_cachetune->alloc, alloc);
+
+    if (VIR_APPEND_ELEMENT(def->cachetunes, def->ncachetunes, tmp_cachetune) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    ctxt->node = oldnode;
+    virDomainCachetuneDefFree(tmp_cachetune);
+    virObjectUnref(alloc);
+    virBitmapFree(vcpus);
+    VIR_FREE(vcpus_str);
+    VIR_FREE(nodes);
+    VIR_FREE(tmp);
+    return ret;
+}
+
+
 static virDomainDefPtr
 virDomainDefParseXML(xmlDocPtr xml,
                      xmlNodePtr root,
@@ -18754,6 +18928,18 @@ virDomainDefParseXML(xmlDocPtr xml,
     }
     VIR_FREE(nodes);
 
+    if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("cannot extract cachetune nodes"));
+        goto error;
+    }
+
+    for (i = 0; i < n; i++) {
+        if (virDomainCachetuneDefParse(def, ctxt, nodes[i]) < 0)
+            goto error;
+    }
+    VIR_FREE(nodes);
+
     if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu) < 0)
         goto error;
 
@@ -25666,6 +25852,68 @@ virDomainSchedulerFormat(virBufferPtr buf,
 }
 
 
+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);
+
+    return 0;
+}
+
+
+static int
+virDomainCachetuneDefFormat(virBufferPtr buf,
+                            virDomainCachetuneDefPtr cachetune)
+{
+    virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
+    char *vcpus = NULL;
+    int ret = -1;
+
+    virBufferSetChildIndent(&childrenBuf, buf);
+    virResctrlAllocForeachSize(cachetune->alloc,
+                               virDomainCachetuneDefFormatHelper,
+                               &childrenBuf);
+
+
+    if (virBufferCheckError(&childrenBuf) < 0)
+        goto cleanup;
+
+    if (!virBufferUse(&childrenBuf)) {
+        ret = 0;
+        goto cleanup;
+    }
+
+    vcpus = virBitmapFormat(cachetune->vcpus);
+
+    virBufferAsprintf(buf, "<cachetune vcpus='%s'>\n", vcpus);
+    virBufferAddBuffer(buf, &childrenBuf);
+    virBufferAddLit(buf, "</cachetune>\n");
+
+    ret = 0;
+ cleanup:
+    virBufferFreeAndReset(&childrenBuf);
+    VIR_FREE(vcpus);
+    return ret;
+}
+
+
 static int
 virDomainCputuneDefFormat(virBufferPtr buf,
                           virDomainDefPtr def)
@@ -25767,6 +26015,9 @@ virDomainCputuneDefFormat(virBufferPtr buf,
                                  def->iothreadids[i]->iothread_id);
     }
 
+    for (i = 0; i < def->ncachetunes; i++)
+        virDomainCachetuneDefFormat(&childrenBuf, def->cachetunes[i]);
+
     if (virBufferCheckError(&childrenBuf) < 0)
         return -1;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 59f250ac96ce..f5f6aed82e54 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -56,6 +56,7 @@
 # include "virperf.h"
 # include "virtypedparam.h"
 # include "virsavecookie.h"
+# include "virresctrl.h"
 
 /* forward declarations of all device types, required by
  * virDomainDeviceDef
@@ -2194,6 +2195,15 @@ struct _virDomainCputune {
 };
 
 
+typedef struct _virDomainCachetuneDef virDomainCachetuneDef;
+typedef virDomainCachetuneDef *virDomainCachetuneDefPtr;
+
+struct _virDomainCachetuneDef {
+    virBitmapPtr vcpus;
+    virResctrlAllocPtr alloc;
+};
+
+
 typedef struct _virDomainVcpuDef virDomainVcpuDef;
 typedef virDomainVcpuDef *virDomainVcpuDefPtr;
 
@@ -2322,6 +2332,9 @@ struct _virDomainDef {
 
     virDomainCputune cputune;
 
+    virDomainCachetuneDefPtr *cachetunes;
+    size_t ncachetunes;
+
     virDomainNumaPtr numa;
     virDomainResourceDefPtr resource;
     virDomainIdMapDef idmap;
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>
+  </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-colliding-allocs.xml b/tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
new file mode 100644
index 000000000000..994f8fcf2a4e
--- /dev/null
+++ b/tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
@@ -0,0 +1,30 @@
+<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'>
+      <cache id='0' level='3' type='code' size='12' unit='KiB'/>
+      <cache id='0' level='3' type='code' size='18' unit='KiB'/>
+    </cachetune>
+  </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-colliding-tunes.xml b/tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
new file mode 100644
index 000000000000..3d9db85b12d4
--- /dev/null
+++ b/tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
@@ -0,0 +1,32 @@
+<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'>
+      <cache id='0' level='3' type='code' size='12' unit='KiB'/>
+    </cachetune>
+    <cachetune vcpus='0'>
+      <cache id='0' level='3' type='data' size='18' unit='KiB'/>
+    </cachetune>
+  </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-colliding-types.xml b/tests/genericxml2xmlindata/cachetune-colliding-types.xml
new file mode 100644
index 000000000000..1a4e62cd17d3
--- /dev/null
+++ b/tests/genericxml2xmlindata/cachetune-colliding-types.xml
@@ -0,0 +1,30 @@
+<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='12' unit='KiB'/>
+      <cache id='0' level='3' type='code' size='6' unit='KiB'/>
+    </cachetune>
+  </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-small.xml b/tests/genericxml2xmlindata/cachetune-small.xml
new file mode 100644
index 000000000000..4b25490b3abb
--- /dev/null
+++ b/tests/genericxml2xmlindata/cachetune-small.xml
@@ -0,0 +1,29 @@
+<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='768' unit='KiB'/>
+    </cachetune>
+  </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>
+  </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/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index deaf5bd9b823..496e9260fa86 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -130,6 +130,16 @@ mymain(void)
     DO_TEST_FULL("chardev-reconnect-invalid-mode", 0, false,
                  TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
 
+    DO_TEST("cachetune");
+    DO_TEST("cachetune-small");
+    DO_TEST("cachetune-cdp");
+    DO_TEST_FULL("cachetune-colliding-allocs", false, true,
+                 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
+    DO_TEST_FULL("cachetune-colliding-tunes", false, true,
+                 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
+    DO_TEST_FULL("cachetune-colliding-types", false, true,
+                 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
+
     virObjectUnref(caps);
     virObjectUnref(xmlopt);
 
-- 
2.15.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune
Posted by John Ferlan 7 years, 4 months ago 

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@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 @@
>      &lt;iothread_quota&gt;-1&lt;/iothread_quota&gt;
>      &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt;
>      &lt;iothreadsched iothreads='2' scheduler='batch'/&gt;
> +    &lt;cachetune vcpus='0-3'&gt;
> +      &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt;
> +      &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt;
> +    &lt;/cachetune&gt;
>    &lt;/cputune&gt;
>    ...
>  &lt;/domain&gt;
> @@ -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@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>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune
Posted by Martin Kletzander 7 years, 3 months ago 
On Wed, Jan 03, 2018 at 06:50:22PM -0500, John Ferlan wrote:
>
>
>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@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 @@
>>      &lt;iothread_quota&gt;-1&lt;/iothread_quota&gt;
>>      &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt;
>>      &lt;iothreadsched iothreads='2' scheduler='batch'/&gt;
>> +    &lt;cachetune vcpus='0-3'&gt;
>> +      &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt;
>> +      &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt;
>> +    &lt;/cachetune&gt;
>>    &lt;/cputune&gt;
>>    ...
>>  &lt;/domain&gt;
>> @@ -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/
>

If you search for Resource Control File System you will find lot of unrelevant
stuff.  For resctrlfs (or rather resctrl), however, you will get the precise
info you are looking for.  It's also called resctrl in the kernel.  You mount it
by doing `mount -t resctrl resctrl [-o cdp] /sys/fs/resctrl`, so I think it's
clearer this way (or without the fs at the end).

>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).
>

We don't.  We don't mention any other sysfs path in the docs.  Why would we?
It's defined elsewhere, not in libvirt.  And since it's the only place to be
mounted for now (according to the docs) we can have it hardcoded.  Just like the
other paths.

>
>> +        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?
>

I don't see the point in that.  Especially since it's called `cache` in the code.

>> +              </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
>

This will look awkward when other functions around have one only, but I've
already given up this fight.

>>  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'
>

1) Because that way we can default to something else in the future.  We
   want to specify everything possible, otherwise we'll get stuck with
   something we can't change.

2) All other code does that, it's consistent.

3) It's more readable for users even without reading the docs.

4) It would be more code with no useful output.

>> +
>> +    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?
>

Sure, this will be shrunk and forgotten.

>> +  </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@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...
>

I have to send another version anyway.  Not only because
virCachetuneHelperData is not used at all, but other things need
adjustments.

>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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune
Posted by John Ferlan 7 years, 3 months ago 

On 01/23/2018 05:07 AM, Martin Kletzander wrote:
> On Wed, Jan 03, 2018 at 06:50:22PM -0500, John Ferlan wrote:
>>
>>
>> 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@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 @@
>>>      &lt;iothread_quota&gt;-1&lt;/iothread_quota&gt;
>>>      &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt;
>>>      &lt;iothreadsched iothreads='2' scheduler='batch'/&gt;
>>> +    &lt;cachetune vcpus='0-3'&gt;
>>> +      &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt;
>>> +      &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt;
>>> +    &lt;/cachetune&gt;
>>>    &lt;/cputune&gt;
>>>    ...
>>>  &lt;/domain&gt;
>>> @@ -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/
>>
> 
> If you search for Resource Control File System you will find lot of
> unrelevant
> stuff.  For resctrlfs (or rather resctrl), however, you will get the
> precise
> info you are looking for.  It's also called resctrl in the kernel.  You
> mount it
> by doing `mount -t resctrl resctrl [-o cdp] /sys/fs/resctrl`, so I think
> it's
> clearer this way (or without the fs at the end).
> 

OK that's fine not that big a deal.

>> 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).
>>
> 
> We don't.  We don't mention any other sysfs path in the docs.  Why would
> we?
> It's defined elsewhere, not in libvirt.  And since it's the only place
> to be
> mounted for now (according to the docs) we can have it hardcoded.  Just
> like the
> other paths.
> 
>>
>>> +        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?
>>
> 
> I don't see the point in that.  Especially since it's called `cache` in
> the code.
> 

Tough to recall - review done a month ago, but I have a recollection of
confusion while reading the code.  Of course you've been working with it
longer so it's second nature. At this point I was perhaps reflecting a
bit too much.

>>> +              </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
>>
> 
> This will look awkward when other functions around have one only, but I've
> already given up this fight.
> 

I cannot disagree and understand the desire to follow existing style,
but IIRC there's been another agreement at one point in time to go with
the double blank lines between functions.  I'm so used to mentioning it
now that it's just habit. I'm sure I don't always follow it 100% in my
patches either.  I mention it - if it's done it's done, if it's not,
well I noted it. It's not like there's a syntax-check rule or some
hacking.html requirement.

>>>  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'
>>
> 
> 1) Because that way we can default to something else in the future.  We
>   want to specify everything possible, otherwise we'll get stuck with
>   something we can't change.
> 
> 2) All other code does that, it's consistent.
> 
> 3) It's more readable for users even without reading the docs.
> 
> 4) It would be more code with no useful output.
> 

AOK... I guess it was more of a "reaction" to seeing 'B' (or 'bytes')
rather than 'KiB' or 'MiB' - as in "yeah so".


John


>>> +
>>> +    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?
>>
> 
> Sure, this will be shrunk and forgotten.
> 
>>> +  </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@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...
>>
> 
> I have to send another version anyway.  Not only because
> virCachetuneHelperData is not used at all, but other things need
> adjustments.
> 
>> 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>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.