[libvirt] [PATCH] Expose resource control capabilites on cache bank

Martin Kletzander mkletzan at redhat.com
Thu Apr 6 09:49:14 UTC 2017


On Thu, Apr 06, 2017 at 04:20:06PM +0800, Eli Qiao wrote:
>This patch is based on Martin's cache branch.
>
>This patch amends the cache bank capability as follow:
>
><bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
>  <control min='768' unit='KiB' type='unified' nclos='4'/>
><bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
>  <control min='768' unit='KiB' type='unified' nclos='4'/>
>

Were we exposing the number of CLoS IDs before?  Was there a discussion
about it?  Do we want to expose them?  Probably yes, I'm just wondering.

>Along with vircaps2xmltest case updated.
>
>Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
>---
> src/conf/capabilities.c                          | 112 +++++++++++++++++++++--
> src/conf/capabilities.h                          |  13 ++-
> tests/vircaps2xmldata/vircaps-x86_64-caches.xml  |   2 +
> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml |   2 +
> tests/vircaps2xmltest.c                          |  11 +++
> 5 files changed, 132 insertions(+), 8 deletions(-)
>
>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>index 416dd1a..75c0bec 100644
>--- a/src/conf/capabilities.c
>+++ b/src/conf/capabilities.c
>@@ -52,6 +52,7 @@
> #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
>
> #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
>+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
>
> VIR_LOG_INIT("conf.capabilities")
>
>@@ -873,6 +874,7 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>                             virCapsHostCacheBankPtr *caches)
> {
>     size_t i = 0;
>+    size_t j = 0;
>
>     if (!ncaches)
>         return 0;
>@@ -900,6 +902,18 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>                           bank->size >> (kilos * 10),
>                           kilos ? "KiB" : "B",
>                           cpus_str);
>+        virBufferAdjustIndent(buf, 2);
>+        for (j = 0; j < bank->ncontrol; j++) {
>+            bool min_kilos = !(bank->controls[j]->min % 1024);
>+            virBufferAsprintf(buf,
>+                              "<control min='%llu' unit='%s' "
>+                              "type='%s' nclos='%u'/>\n",
>+                              bank->controls[j]->min >> (min_kilos * 10),
>+                              min_kilos ? "KiB" : "B",
>+                              virCacheTypeToString(bank->controls[j]->type),
>+                              bank->controls[j]->nclos);
>+        }
>+        virBufferAdjustIndent(buf, -2);
>
>         VIR_FREE(cpus_str);
>     }
>@@ -1516,10 +1530,93 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
>     if (!ptr)
>         return;
>
>+    size_t i;

Upstream frowns upon C99 initialization in the middle of the code.

>     virBitmapFree(ptr->cpus);
>+    for (i = 0; i < ptr->ncontrol; i++)
>+        VIR_FREE(ptr->controls[i]);
>+    VIR_FREE(ptr->controls);
>     VIR_FREE(ptr);
> }
>
>+/* test which kinds of cache control supported
>+ * -1: don't support
>+ *  0: cat
>+ *  1: cdp
>+ */
>+static int
>+virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
>+{
>+    int ret = -1;
>+    char *path = NULL;
>+    if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%u", bank->level) < 0)
>+        return -1;
>+
>+    if (virFileExists(path)) {
>+        ret = 0;
>+    } else {
>+        VIR_FREE(path);
>+        if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%uCODE", bank->level) < 0)
>+            return -1;
>+        if (virFileExists(path))
>+            ret = 1;
>+    }
>+
>+    VIR_FREE(path);
>+    return ret;
>+}
>+
>+static int
>+virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank, const char* type)
>+{
>+    int ret = -1;
>+    char *path = NULL;
>+    char *cbm_mask = NULL;
>+    virCapsHostCacheControlPtr control;
>+
>+    if (VIR_ALLOC(control) < 0)
>+        goto cleanup;
>+
>+    if (virFileReadValueUint(&control->nclos,
>+                             SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
>+                             bank->level,
>+                             type) < 0)
>+        goto cleanup;
>+
>+    if (virFileReadValueString(&cbm_mask,
>+                               SYSFS_RESCTRL_PATH
>+                               "info/L%u%s/cbm_mask",
>+                               bank->level,
>+                               type) < 0)
>+        goto cleanup;
>+
>+    virStringTrimOptionalNewline(cbm_mask);
>+
>+    control->min = bank->size / (strlen(cbm_mask) * 4);
>+
>+    if (STREQ("", type))
>+        control->type = VIR_CACHE_TYPE_UNIFIED;
>+    else if (STREQ("CODE", type))
>+        control->type = VIR_CACHE_TYPE_INSTRUCTION;
>+    else if (STREQ("DATA", type))
>+        control->type = VIR_CACHE_TYPE_DATA;
>+    else
>+        goto cleanup;
>+
>+    if (VIR_APPEND_ELEMENT(bank->controls,
>+                           bank->ncontrol,
>+                           control) < 0)
>+        goto error;
>+
>+    ret = 0;
>+
>+ cleanup:
>+    VIR_FREE(path);
>+    return ret;
>+ error:
>+    VIR_FREE(control);
>+    return -1;

The return value is not used anywhere.

>+}
>+
> int
> virCapabilitiesInitCaches(virCapsPtr caps)
> {
>@@ -1533,11 +1630,6 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>     struct dirent *ent = NULL;
>     virCapsHostCacheBankPtr bank = NULL;
>
>-    /* Minimum level to expose in capabilities.  Can be lowered or removed (with
>-     * the appropriate code below), but should not be increased, because we'd
>-     * lose information. */
>-    const int cache_min_level = 3;
>-

You are removing this and only reporting it for those we can control.
But I believe the cache information can be valuable by itself, even
for those who can't change it.  If this was intentional, it should be
mentioned in the commit message.

>     /* offline CPUs don't provide cache info */
>     if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
>         return -1;
>@@ -1595,12 +1687,19 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>                                        pos, ent->d_name) < 0)
>                 goto cleanup;
>
>-            if (bank->level < cache_min_level) {
>+            if ((ret = virCapabilitiesGetCacheControlType(bank)) < 0) {
>                 virCapsHostCacheBankFree(bank);
>                 bank = NULL;
>                 continue;
>             }
>
>+            if (ret == 0) {
>+                virCapabilitiesGetCacheControl(bank, "");
>+            } else if (ret == 1) {
>+                virCapabilitiesGetCacheControl(bank, "CODE");
>+                virCapabilitiesGetCacheControl(bank, "DATA");
>+            }
>+
>             for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
>                 *tmp_c = c_tolower(*tmp_c);
>
>@@ -1617,6 +1716,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>                 if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
>                     break;
>             }
>+
>             if (i == caps->host.ncaches) {
>                 if (VIR_APPEND_ELEMENT(caps->host.caches,
>                                        caps->host.ncaches,
>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
>index e099ccc..13effdd 100644
>--- a/src/conf/capabilities.h
>+++ b/src/conf/capabilities.h
>@@ -139,15 +139,22 @@ struct _virCapsHostSecModel {
> };
>
> typedef enum {
>-    VIR_CACHE_TYPE_DATA,
>-    VIR_CACHE_TYPE_INSTRUCTION,
>     VIR_CACHE_TYPE_UNIFIED,
>+    VIR_CACHE_TYPE_INSTRUCTION,
>+    VIR_CACHE_TYPE_DATA,
>
>     VIR_CACHE_TYPE_LAST
> } virCacheType;
>
> VIR_ENUM_DECL(virCache);
>
>+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
>+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
>+struct _virCapsHostCacheControl {
>+    unsigned long long min; /* B */
>+    virCacheType type;  /* Data, Instruction or Unified */
>+    unsigned int nclos; /* number class of id */
>+};
> typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
> typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;
> struct _virCapsHostCacheBank {
>@@ -156,6 +163,8 @@ struct _virCapsHostCacheBank {
>     unsigned long long size; /* B */
>     virCacheType type;  /* Data, Instruction or Unified */
>     virBitmapPtr cpus;  /* All CPUs that share this bank */
>+    size_t ncontrol;
>+    virCapsHostCacheControlPtr *controls;
> };
>
> typedef struct _virCapsHost virCapsHost;
>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
>index f2da28e..61269ea 100644
>--- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
>+++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
>@@ -30,6 +30,8 @@
>     </topology>
>     <cache>
>       <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/>
>+        <control min='419430' unit='B' type='instruction' nclos='8'/>
>+        <control min='419430' unit='B' type='data' nclos='8'/>

This file should have no <control/> in it.  There is no resctrl for
these.  I don't see the bug immeadiatelly, though.

>     </cache>
>   </host>
>
>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>index c30ea87..df27b94 100644
>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>@@ -42,7 +42,9 @@
>     </topology>
>     <cache>
>       <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
>+        <control min='768' unit='KiB' type='unified' nclos='4'/>
>       <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
>+        <control min='768' unit='KiB' type='unified' nclos='4'/>
>     </cache>
>   </host>
>
>diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
>index f590249..93f776a 100644
>--- a/tests/vircaps2xmltest.c
>+++ b/tests/vircaps2xmltest.c
>@@ -47,6 +47,7 @@ test_virCapabilities(const void *opaque)
>     char *capsXML = NULL;
>     char *path = NULL;
>     char *dir = NULL;
>+    char *resctrl_dir = NULL;
>     int ret = -1;
>
>     /*
>@@ -58,6 +59,15 @@ test_virCapabilities(const void *opaque)
>                     data->resctrl ? "/system" : "") < 0)
>         goto cleanup;
>
>+    if (data->resctrl) {
>+        if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s%s",

just /resctrl instead of the last %s

>+                        abs_srcdir, data->filename,
>+                        "/resctrl") < 0)
>+        goto cleanup;
>+        virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);
>+    }
>+
>+
>     virFileMockAddPrefix("/sys/devices/system", dir);
>     caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>
>@@ -84,6 +94,7 @@ test_virCapabilities(const void *opaque)
>
>  cleanup:
>     VIR_FREE(dir);
>+    VIR_FREE(resctrl_dir);
>     VIR_FREE(path);
>     VIR_FREE(capsXML);
>     virObjectUnref(caps);
>--
>1.9.1
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170406/23c2f4c3/attachment-0001.sig>


More information about the libvir-list mailing list