[libvirt] [RFC PATCH 2/2] conf: Extend cputune/cachetune to support memory bandwidth allocation

bing.niu@intel.com posted 2 patches 6 years, 11 months ago
There is a newer version of this series
[libvirt] [RFC PATCH 2/2] conf: Extend cputune/cachetune to support memory bandwidth allocation
Posted by bing.niu@intel.com 6 years, 11 months ago
From: Bing Niu <bing.niu@intel.com>

Extend current cachetune section to support memory bandwidth allocation.
Add a new cachetune element llc for memory allocation. As the example
below:

        <cachetune vcpus='0'>
                <llc id='0' bandwidth='30'/>
        </cachetune>

id        --- on which last level cache memory bandwidth to be set
bandwidth --- the memory bandwidth percent to set.

Signed-off-by: Bing Niu <bing.niu@intel.com>
---
 src/conf/domain_conf.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----
 src/util/virresctrl.c  |  7 ++++
 2 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6ac47c..aba998d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18578,6 +18578,55 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
     return ret;
 }
 
+static int
+virDomainCachetuneDefParseMemoryBandwidth(xmlXPathContextPtr ctxt,
+                                          xmlNodePtr node,
+                                          virResctrlAllocPtr alloc)
+{
+    xmlNodePtr oldnode = ctxt->node;
+    unsigned int id;
+    unsigned int bandwidth;
+    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, &id) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'id' value '%s'"),
+                       tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    tmp = virXMLPropString(node, "bandwidth");
+    if (!tmp) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Missing cachetune attribute 'bandwidth'"));
+        goto cleanup;
+    }
+    if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cachetune attribute 'bandwidth' value '%s'"),
+                       tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+    if (virResctrlSetMemoryBandwidth(alloc, id, bandwidth) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    ctxt->node = oldnode;
+    VIR_FREE(tmp);
+    return ret;
+}
 
 static int
 virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
@@ -18670,6 +18719,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
     ssize_t i = 0;
     int n;
     int ret = -1;
+    bool mba_available = false;
 
     ctxt->node = node;
 
@@ -18701,12 +18751,26 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
         goto cleanup;
     }
 
+    if ((n = virXPathNodeSet("./llc", ctxt, &nodes)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Cannot extract Memory Bandwidth  nodes under "
+                         "cachetune. try cache allocation"));
+    }
+
+    for (i = 0; i < n; i++) {
+        if (virDomainCachetuneDefParseMemoryBandwidth(ctxt, nodes[i], alloc) < 0)
+            goto cleanup;
+    }
+
+    if (n)
+        mba_available = true;
+
     if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Cannot extract cache nodes under cachetune"));
-        goto cleanup;
+        if (!mba_available)
+            goto cleanup;
     }
-
     for (i = 0; i < n; i++) {
         if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
             goto cleanup;
@@ -26394,11 +26458,19 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
     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);
+    /* If type is VIR_CACHE_TYPE_LAST, this means it's a memory
+     * bandwidth allocation formatting request */
+    if (type == VIR_CACHE_TYPE_LAST)
+        virBufferAsprintf(buf,
+                          "<llc id='%u' bandwidth='%llu'/>\n",
+                          cache, size);
+
+    else
+        virBufferAsprintf(buf,
+                          "<cache id='%u' level='%u' type='%s' "
+                          "size='%llu' unit='%s'/>\n",
+                          cache, level, virCacheTypeToString(type),
+                          short_size, unit);
 
     return 0;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index ad050c7..c720074 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -954,6 +954,13 @@ virResctrlAllocForeachSize(virResctrlAllocPtr resctrl,
         }
     }
 
+    if (resctrl->mba) {
+        virResctrlAllocMBPtr mba = resctrl->mba;
+        for (cache = 0; cache < mba->nsizes; cache++)
+            if (mba->bandwidth[cache])
+                cb(0, VIR_CACHE_TYPE_LAST, cache, *mba->bandwidth[cache], opaque);
+    }
+
     return 0;
 }
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 2/2] conf: Extend cputune/cachetune to support memory bandwidth allocation
Posted by Ján Tomko 6 years, 11 months ago
On Tue, May 29, 2018 at 06:58:03PM +0800, bing.niu@intel.com wrote:
>From: Bing Niu <bing.niu@intel.com>
>
>Extend current cachetune section to support memory bandwidth allocation.
>Add a new cachetune element llc for memory allocation. As the example
>below:
>
>        <cachetune vcpus='0'>
>                <llc id='0' bandwidth='30'/>
>        </cachetune>
>
>id        --- on which last level cache memory bandwidth to be set
>bandwidth --- the memory bandwidth percent to set.
>
>Signed-off-by: Bing Niu <bing.niu@intel.com>
>---
> src/conf/domain_conf.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----
> src/util/virresctrl.c  |  7 ++++
> 2 files changed, 86 insertions(+), 7 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index d6ac47c..aba998d 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -18578,6 +18578,55 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>     return ret;
> }
>
> static int
> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>@@ -18670,6 +18719,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>     ssize_t i = 0;
>     int n;
>     int ret = -1;
>+    bool mba_available = false;
>
>     ctxt->node = node;
>
>@@ -18701,12 +18751,26 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>         goto cleanup;
>     }
>
>+    if ((n = virXPathNodeSet("./llc", ctxt, &nodes)) < 0) {
>+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                       _("Cannot extract Memory Bandwidth  nodes under "
>+                         "cachetune. try cache allocation"));
>+    }
>+
>+    for (i = 0; i < n; i++) {
>+        if (virDomainCachetuneDefParseMemoryBandwidth(ctxt, nodes[i], alloc) < 0)
>+            goto cleanup;
>+    }
>+
>+    if (n)
>+        mba_available = true;
>+
>     if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                        _("Cannot extract cache nodes under cachetune"));
>-        goto cleanup;
>+        if (!mba_available)
>+            goto cleanup;

Is it okay to skip cache parsing in that case?

>     }
>-
>     for (i = 0; i < n; i++) {
>         if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
>             goto cleanup;
>@@ -26394,11 +26458,19 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
>     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);
>+    /* If type is VIR_CACHE_TYPE_LAST, this means it's a memory
>+     * bandwidth allocation formatting request */

_TYPE_LAST values are sentinels used to iterate over all enum values
and should never be given any other meaning.

Jano

>+    if (type == VIR_CACHE_TYPE_LAST)
>+        virBufferAsprintf(buf,
>+                          "<llc id='%u' bandwidth='%llu'/>\n",
>+                          cache, size);
>+

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 2/2] conf: Extend cputune/cachetune to support memory bandwidth allocation
Posted by bing.niu 6 years, 11 months ago
Hi Jano,
Thanks for review, please see my response.

On 2018年06月02日 22:54, Ján Tomko wrote:
> On Tue, May 29, 2018 at 06:58:03PM +0800, bing.niu@intel.com wrote:
>> From: Bing Niu <bing.niu@intel.com>
>>
>> Extend current cachetune section to support memory bandwidth allocation.
>> Add a new cachetune element llc for memory allocation. As the example
>> below:
>>
>>        <cachetune vcpus='0'>
>>                <llc id='0' bandwidth='30'/>
>>        </cachetune>
>>
>> id        --- on which last level cache memory bandwidth to be set
>> bandwidth --- the memory bandwidth percent to set.
>>
>> Signed-off-by: Bing Niu <bing.niu@intel.com>
>> ---
>> src/conf/domain_conf.c | 86 
>> ++++++++++++++++++++++++++++++++++++++++++++++----
>> src/util/virresctrl.c  |  7 ++++
>> 2 files changed, 86 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d6ac47c..aba998d 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18578,6 +18578,55 @@ virDomainDefParseBootOptions(virDomainDefPtr 
>> def,
>>     return ret;
>> }
>>
>> static int
>> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>> @@ -18670,6 +18719,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>>     ssize_t i = 0;
>>     int n;
>>     int ret = -1;
>> +    bool mba_available = false;
>>
>>     ctxt->node = node;
>>
>> @@ -18701,12 +18751,26 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>>         goto cleanup;
>>     }
>>
>> +    if ((n = virXPathNodeSet("./llc", ctxt, &nodes)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Cannot extract Memory Bandwidth  nodes under "
>> +                         "cachetune. try cache allocation"));
>> +    }
>> +
>> +    for (i = 0; i < n; i++) {
>> +        if (virDomainCachetuneDefParseMemoryBandwidth(ctxt, nodes[i], 
>> alloc) < 0)
>> +            goto cleanup;
>> +    }
>> +
>> +    if (n)
>> +        mba_available = true;
>> +
>>     if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
>>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                        _("Cannot extract cache nodes under cachetune"));
>> -        goto cleanup;
>> +        if (!mba_available)
>> +            goto cleanup;
> 
> Is it okay to skip cache parsing in that case?

Yes, but we need vcpu_str part below the cache parsing part. I saw the 
vcpu_str is used to create folder in virresctrl for a rdt group.
Maybe we can add inline wrapper functions to skip cache parsing part.
How do you think?
> 
>>     }
>> -
>>     for (i = 0; i < n; i++) {
>>         if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
>>             goto cleanup;
>> @@ -26394,11 +26458,19 @@ virDomainCachetuneDefFormatHelper(unsigned 
>> int level,
>>     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);
>> +    /* If type is VIR_CACHE_TYPE_LAST, this means it's a memory
>> +     * bandwidth allocation formatting request */
> 
> _TYPE_LAST values are sentinels used to iterate over all enum values
> and should never be given any other meaning.

hmm. Let me adjust callback functions to handle this explicitly.
> 
> Jano
> 
>> +    if (type == VIR_CACHE_TYPE_LAST)
>> +        virBufferAsprintf(buf,
>> +                          "<llc id='%u' bandwidth='%llu'/>\n",
>> +                          cache, size);
>> +
> 

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