This patch adds new xml element to support cache tune as:

<cputune>
  ...
  <cachetune id='0' cache_id='0' level='3' type='both' size='2816' unit='KiB'
  vcpus='1,2'/>
  ...
</cputune>

id: any non-minus number
cache_id: reference of the host's cache banks id, it's from capabilities
level: cache level
type: cache bank type
size: should be multiples of the min_size of the bank on host.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus

Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
---
 docs/schemas/domaincommon.rng |  54 +++++++++++++++++
 src/conf/domain_conf.c        | 131 ++++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h        |  21 +++++++
 3 files changed, 206 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4950ddc..11dbb55 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -834,6 +834,35 @@
             </attribute>
           </element>
         </zeroOrMore>
+        <zeroOrMore>
+          <element name="cachetune">
+            <attribute name="id">
+              <ref name="cachetuneid"/>
+            </attribute>
+            <attribute name="cache_id">
+              <ref name="cacheid"/>
+            </attribute>
+            <attribute name="level">
+              <ref name="cachelevel"/>
+            </attribute>
+            <attribute name="type">
+              <ref name="cachetype"/>
+            </attribute>
+            <attribute name="size">
+              <ref name="unsignedInt"/>
+            </attribute>
+            <optional>
+              <attribute name="unit">
+                <ref name="cacheunit"/>
+              </attribute>
+            </optional>
+            <optional>
+              <attribute name="vcpus">
+                <ref name="cpuset"/>
+              </attribute>
+            </optional>
+          </element>
+        </zeroOrMore>
         <optional>
           <element name="emulatorpin">
             <attribute name="cpuset">
@@ -5686,6 +5715,31 @@
       <param name="minInclusive">-1</param>
     </data>
   </define>
+  <define name="cachetuneid">
+    <data type="unsignedShort">
+      <param name="pattern">[0-9]+</param>
+    </data>
+  </define>
+  <define name="cacheid">
+    <data type="unsignedShort">
+      <param name="pattern">[0-9]+</param>
+    </data>
+  </define>
+  <define name="cachelevel">
+    <data type="unsignedShort">
+      <param name="pattern">(3)</param>
+    </data>
+  </define>
+  <define name="cachetype">
+    <data type="string">
+      <param name="pattern">(both|code|data)</param>
+    </data>
+  </define>
+  <define name="cacheunit">
+    <data type="string">
+      <param name="pattern">KiB</param>
+    </data>
+  </define>
   <!-- weight currently is in range [100, 1000] -->
   <define name="weight">
     <data type="unsignedInt">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c32f93..93984bc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16250,6 +16250,104 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
     return ret;
 }
 
+/* Parse the XML definition for cachetune
+ * and a cachetune has the form
+ * <cachetune id='0' cache_id='0' level='3' type='both' size='1024' unit='KiB'/>
+ */
+static int
+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
+                              int n,
+                              xmlNodePtr* nodes)
+{
+    char* tmp = NULL;
+    size_t i;
+    int type = -1;
+    virDomainCacheBankPtr bank = NULL;
+
+    if (VIR_ALLOC_N(bank, n) < 0)
+        goto cleanup;
+
+    for (i = 0; i < n; i++) {
+        if (!(tmp = virXMLPropString(nodes[i], "id"))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache tune"));
+            goto cleanup;
+        }
+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid setting for cache id '%s'"), tmp);
+            goto cleanup;
+        }
+        VIR_FREE(tmp);
+
+        if (!(tmp = virXMLPropString(nodes[i], "cache_id"))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache id in cache tune"));
+            goto cleanup;
+        }
+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].cache_id)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid setting for cache host id '%s'"), tmp);
+            goto cleanup;
+        }
+        VIR_FREE(tmp);
+
+        if (!(tmp = virXMLPropString(nodes[i], "level"))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache level in cache tune"));
+            goto cleanup;
+        }
+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].level)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid setting for cache level '%s'"), tmp);
+            goto cleanup;
+        }
+        VIR_FREE(tmp);
+
+
+        if (!(tmp = virXMLPropString(nodes[i], "size"))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache tune"));
+            goto cleanup;
+        }
+        if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid setting for cache size '%s'"), tmp);
+            goto cleanup;
+        }
+        /* convert to B */
+        bank[i].size *= 1024;
+        VIR_FREE(tmp);
+
+        if (!(tmp = virXMLPropString(nodes[i], "type"))) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("missing cache type"));
+            goto cleanup;
+        }
+        if ((type = virCacheTypeFromString(tmp)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                    _("'unsupported cache type '%s'"), tmp);
+            goto cleanup;
+        }
+
+        if ((tmp = virXMLPropString(nodes[i], "vcpus"))) {
+            if (virBitmapParse(tmp, &bank[i].vcpus,
+                        VIR_DOMAIN_CPUMASK_LEN) < 0)
+                goto cleanup;
+
+            if (virBitmapIsAllClear(bank[i].vcpus)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                        _("Invalid value of 'vcpus': %s"), tmp);
+                goto cleanup;
+            }
+        }
+    }
+
+    def->cachetune.cache_banks = bank;
+    def->cachetune.n_banks = n;
+    return 0;
+
+ cleanup:
+    VIR_FREE(bank);
+    VIR_FREE(tmp);
+    return -1;
+}
 
 /* Parse the XML definition for a iothreadpin
  * and an iothreadspin has the form
@@ -17538,6 +17636,14 @@ virDomainDefParseXML(xmlDocPtr xml,
     }
     VIR_FREE(nodes);
 
+    if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0)
+        goto error;
+
+    if (virDomainCacheTuneDefParseXML(def, n, nodes) < 0)
+        goto error;
+
+    VIR_FREE(nodes);
+
     if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("cannot extract emulatorpin nodes"));
@@ -24328,6 +24434,29 @@ virDomainSchedulerFormat(virBufferPtr buf,
 }
 
 
+static void
+virDomainCacheTuneDefFormat(virBufferPtr buf,
+                            virDomainCachetunePtr cache)
+{
+    size_t i;
+    for (i = 0; i < cache->n_banks; i ++) {
+        virBufferAsprintf(buf, "<cachetune id='%u' cache_id='%u' "
+                               "level='%u' type='%s' size='%llu' unit='KiB'",
+                               cache->cache_banks[i].id,
+                               cache->cache_banks[i].cache_id,
+                               cache->cache_banks[i].level,
+                               virCacheTypeToString(cache->cache_banks[i].type),
+                               cache->cache_banks[i].size / 1024);
+
+        if (cache->cache_banks[i].vcpus)
+            virBufferAsprintf(buf, " vcpus='%s'/>\n",
+                    virBitmapFormat(cache->cache_banks[i].vcpus));
+        else
+            virBufferAddLit(buf, "/>\n");
+    }
+}
+
+
 static int
 virDomainCputuneDefFormat(virBufferPtr buf,
                           virDomainDefPtr def)
@@ -24374,6 +24503,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
                           "</iothread_quota>\n",
                           def->cputune.iothread_quota);
 
+    virDomainCacheTuneDefFormat(&childrenBuf, &def->cachetune);
+
     for (i = 0; i < def->maxvcpus; i++) {
         char *cpumask;
         virDomainVcpuDefPtr vcpu = def->vcpus[i];
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e67b6fd..0a3566a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2190,6 +2190,25 @@ struct _virDomainMemtune {
     int allocation; /* enum virDomainMemoryAllocation */
 };
 
+typedef struct _virDomainCacheBank virDomainCacheBank;
+typedef virDomainCacheBank *virDomainCacheBankPtr;
+
+struct _virDomainCacheBank {
+    unsigned int id;
+    unsigned int cache_id; /* cache id to be allocated on */
+    unsigned int level; /* cache level */
+    int type; /* enum virCacheType */
+    unsigned long long size; /* in kibibytes */
+    virBitmapPtr vcpus;
+};
+
+typedef struct _virDomainCachetune virDomainCachetune;
+typedef virDomainCachetune *virDomainCachetunePtr;
+struct _virDomainCachetune {
+    size_t n_banks;
+    virDomainCacheBankPtr cache_banks;
+};
+
 typedef struct _virDomainPowerManagement virDomainPowerManagement;
 typedef virDomainPowerManagement *virDomainPowerManagementPtr;
 
@@ -2263,6 +2282,8 @@ struct _virDomainDef {
 
     virDomainCputune cputune;
 
+    virDomainCachetune cachetune;
+
     virDomainNumaPtr numa;
     virDomainResourceDefPtr resource;
     virDomainIdMapDef idmap;
-- 
1.9.1

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

On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote:
>This patch adds new xml element to support cache tune as:
>
><cputune>
>  ...
>  <cachetune id='0' cache_id='0' level='3' type='both' size='2816' unit='KiB'
>  vcpus='1,2'/>

The cache_id automatically implies level and type.  Either have one or
the other.  I know we talked about this already (maybe multiple times),
but without any clear outcome.  For me the sensible thing is to have
level and type as that doesn't need to be changed when moving between
hosts, and if it cannot be migrated, then it's properly checked.

>  ...
></cputune>
>
>id: any non-minus number
>cache_id: reference of the host's cache banks id, it's from capabilities
>level: cache level
>type: cache bank type
>size: should be multiples of the min_size of the bank on host.

must be multiple of granularity, must be greater than or equal to minimum.

>vcpus: cache allocation on vcpu set, if empty, will apply the allocation
>on all vcpus
>
>Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
>---
> docs/schemas/domaincommon.rng |  54 +++++++++++++++++
> src/conf/domain_conf.c        | 131 ++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h        |  21 +++++++
> 3 files changed, 206 insertions(+)
>
>diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>index 4950ddc..11dbb55 100644
>--- a/docs/schemas/domaincommon.rng
>+++ b/docs/schemas/domaincommon.rng
>@@ -834,6 +834,35 @@
>             </attribute>
>           </element>
>         </zeroOrMore>
>+        <zeroOrMore>
>+          <element name="cachetune">
>+            <attribute name="id">
>+              <ref name="cachetuneid"/>
>+            </attribute>
>+            <attribute name="cache_id">
>+              <ref name="cacheid"/>
>+            </attribute>
>+            <attribute name="level">
>+              <ref name="cachelevel"/>
>+            </attribute>
>+            <attribute name="type">
>+              <ref name="cachetype"/>
>+            </attribute>
>+            <attribute name="size">
>+              <ref name="unsignedInt"/>
>+            </attribute>
>+            <optional>
>+              <attribute name="unit">
>+                <ref name="cacheunit"/>
>+              </attribute>
>+            </optional>
>+            <optional>
>+              <attribute name="vcpus">
>+                <ref name="cpuset"/>
>+              </attribute>
>+            </optional>
>+          </element>
>+        </zeroOrMore>
>         <optional>
>           <element name="emulatorpin">
>             <attribute name="cpuset">
>@@ -5686,6 +5715,31 @@
>       <param name="minInclusive">-1</param>
>     </data>
>   </define>
>+  <define name="cachetuneid">
>+    <data type="unsignedShort">
>+      <param name="pattern">[0-9]+</param>
>+    </data>
>+  </define>
>+  <define name="cacheid">
>+    <data type="unsignedShort">
>+      <param name="pattern">[0-9]+</param>
>+    </data>
>+  </define>

These are useless ^^

>+  <define name="cachelevel">
>+    <data type="unsignedShort">
>+      <param name="pattern">(3)</param>

Either don't restrict it at all or do the following:

<choice>
  <!-- For now we only support allocation of L3 caches -->
  <value>3</value>
<choice>

>+    </data>
>+  </define>
>+  <define name="cachetype">
>+    <data type="string">
>+      <param name="pattern">(both|code|data)</param>
>+    </data>
>+  </define>

<choice>
  <value>both</value>
  <value>code</value>
  <value>data</value>
</choice>

>+  <define name="cacheunit">
>+    <data type="string">
>+      <param name="pattern">KiB</param>
>+    </data>
>+  </define>

This doesn't have to be in KiB, otherwise the unit doesn't make sense.
For all the units I'm sure there already is a define in the sources
somewhere.

>   <!-- weight currently is in range [100, 1000] -->
>   <define name="weight">
>     <data type="unsignedInt">
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 5c32f93..93984bc 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -16250,6 +16250,104 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
>     return ret;
> }
>
>+/* Parse the XML definition for cachetune
>+ * and a cachetune has the form
>+ * <cachetune id='0' cache_id='0' level='3' type='both' size='1024' unit='KiB'/>
>+ */
>+static int
>+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
>+                              int n,
>+                              xmlNodePtr* nodes)
>+{
>+    char* tmp = NULL;
>+    size_t i;
>+    int type = -1;
>+    virDomainCacheBankPtr bank = NULL;
>+
>+    if (VIR_ALLOC_N(bank, n) < 0)
>+        goto cleanup;
>+
>+    for (i = 0; i < n; i++) {
>+        if (!(tmp = virXMLPropString(nodes[i], "id"))) {
>+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache tune"));
>+            goto cleanup;
>+        }
>+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                           _("invalid setting for cache id '%s'"), tmp);
>+            goto cleanup;
>+        }
>+        VIR_FREE(tmp);
>+
>+        if (!(tmp = virXMLPropString(nodes[i], "cache_id"))) {
>+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache id in cache tune"));
>+            goto cleanup;
>+        }
>+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].cache_id)) < 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                           _("invalid setting for cache host id '%s'"), tmp);
>+            goto cleanup;
>+        }
>+        VIR_FREE(tmp);
>+
>+        if (!(tmp = virXMLPropString(nodes[i], "level"))) {
>+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache level in cache tune"));
>+            goto cleanup;
>+        }
>+        if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].level)) < 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                           _("invalid setting for cache level '%s'"), tmp);
>+            goto cleanup;
>+        }
>+        VIR_FREE(tmp);
>+
>+
>+        if (!(tmp = virXMLPropString(nodes[i], "size"))) {
>+            virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache tune"));
>+            goto cleanup;
>+        }
>+        if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                           _("invalid setting for cache size '%s'"), tmp);

Your error messages until now were almost consistent

>+            goto cleanup;
>+        }
>+        /* convert to B */
>+        bank[i].size *= 1024;
>+        VIR_FREE(tmp);
>+
>+        if (!(tmp = virXMLPropString(nodes[i], "type"))) {
>+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                           _("missing cache type"));
>+            goto cleanup;
>+        }
>+        if ((type = virCacheTypeFromString(tmp)) < 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                    _("'unsupported cache type '%s'"), tmp);

But now it's different, plus s/'unsupp/unsupp/

>+            goto cleanup;
>+        }
>+
>+        if ((tmp = virXMLPropString(nodes[i], "vcpus"))) {
>+            if (virBitmapParse(tmp, &bank[i].vcpus,
>+                        VIR_DOMAIN_CPUMASK_LEN) < 0)

This can be one line

>+                goto cleanup;
>+
>+            if (virBitmapIsAllClear(bank[i].vcpus)) {
>+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                        _("Invalid value of 'vcpus': %s"), tmp);

And now there's a colon, apostrophes around 'vcpus', but tmp can be
empty string and that will not be very visible...

>+                goto cleanup;
>+            }
>+        }
>+    }
>+
>+    def->cachetune.cache_banks = bank;
>+    def->cachetune.n_banks = n;
>+    return 0;
>+
>+ cleanup:
>+    VIR_FREE(bank);
>+    VIR_FREE(tmp);
>+    return -1;
>+}
>
> /* Parse the XML definition for a iothreadpin
>  * and an iothreadspin has the form
>@@ -17538,6 +17636,14 @@ virDomainDefParseXML(xmlDocPtr xml,
>     }
>     VIR_FREE(nodes);
>
>+    if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0)
>+        goto error;
>+
>+    if (virDomainCacheTuneDefParseXML(def, n, nodes) < 0)

You are calling it even when n == 0

>+        goto error;
>+
>+    VIR_FREE(nodes);
>+
>     if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) {
>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                        _("cannot extract emulatorpin nodes"));
>@@ -24328,6 +24434,29 @@ virDomainSchedulerFormat(virBufferPtr buf,
> }
>
>
>+static void
>+virDomainCacheTuneDefFormat(virBufferPtr buf,
>+                            virDomainCachetunePtr cache)
>+{
>+    size_t i;

Missing empty line

>+    for (i = 0; i < cache->n_banks; i ++) {
>+        virBufferAsprintf(buf, "<cachetune id='%u' cache_id='%u' "
>+                               "level='%u' type='%s' size='%llu' unit='KiB'",
>+                               cache->cache_banks[i].id,
>+                               cache->cache_banks[i].cache_id,
>+                               cache->cache_banks[i].level,
>+                               virCacheTypeToString(cache->cache_banks[i].type),
>+                               cache->cache_banks[i].size / 1024);
>+
>+        if (cache->cache_banks[i].vcpus)
>+            virBufferAsprintf(buf, " vcpus='%s'/>\n",
>+                    virBitmapFormat(cache->cache_banks[i].vcpus));
>+        else
>+            virBufferAddLit(buf, "/>\n");
>+    }
>+}
>+
>+
> static int
> virDomainCputuneDefFormat(virBufferPtr buf,
>                           virDomainDefPtr def)
>@@ -24374,6 +24503,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
>                           "</iothread_quota>\n",
>                           def->cputune.iothread_quota);
>
>+    virDomainCacheTuneDefFormat(&childrenBuf, &def->cachetune);
>+
>     for (i = 0; i < def->maxvcpus; i++) {
>         char *cpumask;
>         virDomainVcpuDefPtr vcpu = def->vcpus[i];
>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index e67b6fd..0a3566a 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -2190,6 +2190,25 @@ struct _virDomainMemtune {
>     int allocation; /* enum virDomainMemoryAllocation */
> };
>
>+typedef struct _virDomainCacheBank virDomainCacheBank;
>+typedef virDomainCacheBank *virDomainCacheBankPtr;
>+
>+struct _virDomainCacheBank {
>+    unsigned int id;
>+    unsigned int cache_id; /* cache id to be allocated on */
>+    unsigned int level; /* cache level */
>+    int type; /* enum virCacheType */
>+    unsigned long long size; /* in kibibytes */
>+    virBitmapPtr vcpus;
>+};
>+
>+typedef struct _virDomainCachetune virDomainCachetune;
>+typedef virDomainCachetune *virDomainCachetunePtr;
>+struct _virDomainCachetune {
>+    size_t n_banks;
>+    virDomainCacheBankPtr cache_banks;
>+};
>+

I don't think there's a real need for this intermediate structure, but
since you want to be passing it around everywhere...

> typedef struct _virDomainPowerManagement virDomainPowerManagement;
> typedef virDomainPowerManagement *virDomainPowerManagementPtr;
>
>@@ -2263,6 +2282,8 @@ struct _virDomainDef {
>
>     virDomainCputune cputune;
>
>+    virDomainCachetune cachetune;
>+

It is part of cputune in the XML, why not here?

>     virDomainNumaPtr numa;
>     virDomainResourceDefPtr resource;
>     virDomainIdMapDef idmap;
>--
>1.9.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

hi Martin  

It’s really nice of you to help reviewing the mass code. Thanks.

I don’t find a better way to split patch.  


On Wednesday, 21 June 2017 at 9:53 PM, Martin Kletzander wrote:

> On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote:
> > This patch adds new xml element to support cache tune as:
> >  
> > <cputune>
> > ...
> > <cachetune id='0' cache_id='0' level='3' type='both' size='2816' unit='KiB'
> > vcpus='1,2'/>
> >  
>  
>  
> The cache_id automatically implies level and type. Either have one or
> the other. I know we talked about this already (maybe multiple times),
> but without any clear outcome. For me the sensible thing is to have
> level and type as that doesn't need to be changed when moving between
> hosts, and if it cannot be migrated, then it's properly checked.
>  
Think about this case, if the VM has numa setting, the VM has multiple vcpu
running across sockets, if we don’t specify cache_id (cache id stand for
on which Socket/Cache), how can we know on which Socket we allocation for the VM?

I can image there’s 2 cases:

1. if we don’t specify vcpus, and our host have 2 or more Socket, we have this xml define

 <cachetune id='0' level='3' type='both' size='2816' unit=‘KiB’>

We allocate 2816 KiB cache on all of the Socket/Cache.

2. if we specify vcpus
 <cachetune id='0' level='3' type='both' size='2816' unit=‘KiB’, vcpus=‘1,2'>

 <cachetune id=‘1' level='3' type='both' size=‘5632' unit=‘KiB’, vcpus=‘3,4’>

We need to make sure we vcpu 1, 2 are mapped to Socket/Cache 0 and 3,4 on Socket/Cache 1.
So that vcpus running on Socket/Cache 0 has 2816 KiB cache allocated and vcpus running on
Socket/Cache 1 has 5632 KiB cache allocated.

Does it make sense?
  
…


> >  
> > virDomainCputune cputune;
> >  
> > + virDomainCachetune cachetune;
> > +
> >  
>  
>  
> It is part of cputune in the XML, why not here?

Oh yes, I will rethink how to simple the domain cache tune.  
>  
> > virDomainNumaPtr numa;
> > virDomainResourceDefPtr resource;
> > virDomainIdMapDef idmap;
> > --
> > 1.9.1
> >  
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >  
>  
>  
> --
> libvir-list mailing list
> libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> https://www.redhat.com/mailman/listinfo/libvir-list
>  
>  


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

On Thu, Jun 22, 2017 at 01:24:29PM +0800, Eli Qiao wrote:
>hi Martin
>
>It’s really nice of you to help reviewing the mass code. Thanks.
>
>I don’t find a better way to split patch.
>
>
>On Wednesday, 21 June 2017 at 9:53 PM, Martin Kletzander wrote:
>
>> On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote:
>> > This patch adds new xml element to support cache tune as:
>> >
>> > <cputune>
>> > ...
>> > <cachetune id='0' cache_id='0' level='3' type='both' size='2816' unit='KiB'
>> > vcpus='1,2'/>
>> >
>>
>>
>> The cache_id automatically implies level and type. Either have one or
>> the other. I know we talked about this already (maybe multiple times),
>> but without any clear outcome. For me the sensible thing is to have
>> level and type as that doesn't need to be changed when moving between
>> hosts, and if it cannot be migrated, then it's properly checked.
>>
>Think about this case, if the VM has numa setting, the VM has multiple vcpu
>running across sockets, if we don’t specify cache_id (cache id stand for
>on which Socket/Cache), how can we know on which Socket we allocation for the VM?
>

I missed this.  Ye, you're right, thanks for showing that with a use
case.  We could then require only the cache_id and automatically fill
in level and type.  Migration (or rather any start) should then fail if
that cache_id doesn't correspond to the level and type requested.  I
still can't find a reason for 'id', though.

>I can image there’s 2 cases:
>
>1. if we don’t specify vcpus, and our host have 2 or more Socket, we have this xml define
>
> <cachetune id='0' level='3' type='both' size='2816' unit=‘KiB’>
>
>We allocate 2816 KiB cache on all of the Socket/Cache.
>
>2. if we specify vcpus
> <cachetune id='0' level='3' type='both' size='2816' unit=‘KiB’, vcpus=‘1,2'>
>
> <cachetune id=‘1' level='3' type='both' size=‘5632' unit=‘KiB’, vcpus=‘3,4’>
>
>We need to make sure we vcpu 1, 2 are mapped to Socket/Cache 0 and 3,4 on Socket/Cache 1.
>So that vcpus running on Socket/Cache 0 has 2816 KiB cache allocated and vcpus running on
>Socket/Cache 1 has 5632 KiB cache allocated.
>
>Does it make sense?
>
>…
>
>
>> >
>> > virDomainCputune cputune;
>> >
>> > + virDomainCachetune cachetune;
>> > +
>> >
>>
>>
>> It is part of cputune in the XML, why not here?
>
>Oh yes, I will rethink how to simple the domain cache tune.
>>
>> > virDomainNumaPtr numa;
>> > virDomainResourceDefPtr resource;
>> > virDomainIdMapDef idmap;
>> > --
>> > 1.9.1
>> >
>> > --
>> > libvir-list mailing list
>> > libvir-list@redhat.com (mailto:libvir-list@redhat.com)
>> > https://www.redhat.com/mailman/listinfo/libvir-list
>> >
>>
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com (mailto:libvir-list@redhat.com)
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thursday, 22 June 2017 at 8:41 PM, Martin Kletzander wrote:

> I missed this. Ye, you're right, thanks for showing that with a use
> case. We could then require only the cache_id and automatically fill
> in level and type. Migration (or rather any start) should then fail if
> that cache_id doesn't correspond to the level and type requested. I
> still can't find a reason for 'id', though.

I agree that id, level is useless, but also type is required, the case is when
host’s enabled CDP like this:

      <bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'>
        <control granularity='768' unit='KiB' type='code' maxAllocs='8'/>
        <control granularity='768' unit='KiB' type='data' maxAllocs='8'/>
      </bank>


the cachetune need to be defined as:

<cachetune cache_id='0' type=‘l3data' size='2816' unit='KiB' vcpus=‘0,1’/>
<cachetune cache_id='0' type=‘l3code' size='2816' unit='KiB' vcpus=‘0,1’/>
<cachetune cache_id=‘1' type=‘l3data' size='2816' unit='KiB' vcpus=‘2,3’/>
<cachetune cache_id=‘1' type=‘l3code' size='2816' unit='KiB' vcpus=’2,3’/>


Thought?

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