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

Along with vircaps2xmltest case updated.

Signed-off-by: Eli Qiao <liyong.qiao@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;
     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;
+}
+
 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;
-
     /* 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'/>
     </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",
+                        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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> 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'/>

Either the XML is malformed, or the indentation is wrong. The indentation
suggests you want nested XML elements, but the parent element is an empty
tag, so you've actually got a flat namespace here.

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

What are CLoS IDs and what are they used for ?



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote:
>On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
>> 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'/>
>
>Either the XML is malformed, or the indentation is wrong. The indentation
>suggests you want nested XML elements, but the parent element is an empty
>tag, so you've actually got a flat namespace here.
>
>> >
>>
>> 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.
>
>What are CLoS IDs and what are they used for ?
>

Effectively an ID for the allocation.  The hardware has a limited number
of them, in this case 4.  I can't remember whether that number is
per-bank, but it would not make much sense otherwise.

>
>
>Regards,
>Daniel
>--
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>--
>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

On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote:
> On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote:
> > On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> > > 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'/>
> > 
> > Either the XML is malformed, or the indentation is wrong. The indentation
> > suggests you want nested XML elements, but the parent element is an empty
> > tag, so you've actually got a flat namespace here.
> > 
> > > >
> > > 
> > > 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.
> > 
> > What are CLoS IDs and what are they used for ?
> > 
> 
> Effectively an ID for the allocation.  The hardware has a limited number
> of them, in this case 4.  I can't remember whether that number is
> per-bank, but it would not make much sense otherwise.

So, if guests are requesting a private cache allocation, and cos id == 4,
then we can only run 4 guests ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

On Thursday, 6 April 2017 at 6:10 PM, Daniel P. Berrange wrote:

> On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote:
> > On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote:
> > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> > > > 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'/>
> > > > >  
> > > >  
> > > >  
> > >  
> > >  
> > > Either the XML is malformed, or the indentation is wrong. The indentation
> > > suggests you want nested XML elements, but the parent element is an empty
> > > tag, so you've actually got a flat namespace here.
> > >  
> > > >  
> > > > 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.
> > > >  
> > >  
> > >  
> > > What are CLoS IDs and what are they used for ?
> >  
> > Effectively an ID for the allocation. The hardware has a limited number
> > of them, in this case 4. I can't remember whether that number is
> > per-bank, but it would not make much sense otherwise.
> >  
>  
>  
> So, if guests are requesting a private cache allocation, and cos id == 4,
> then we can only run 4 guests ?
>  
>  


I think yes, but it’s per bank resource (the bank here is equal to cache id), if you have 2 banks , you can create 8 guests (each has 1 bank allocation)

As far as I know, the number of clos id is 16 on most of Intel xeon CPUs
>  
> Regards,
> Daniel
> --  
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
>  
> --
> 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, Apr 06, 2017 at 06:29:47PM +0800, Eli Qiao wrote:
> 
> 
> On Thursday, 6 April 2017 at 6:10 PM, Daniel P. Berrange wrote:
> 
> > On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote:
> > > On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote:
> > > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> > > > > 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'/>
> > > > > >  
> > > > >  
> > > > >  
> > > >  
> > > >  
> > > > Either the XML is malformed, or the indentation is wrong. The indentation
> > > > suggests you want nested XML elements, but the parent element is an empty
> > > > tag, so you've actually got a flat namespace here.
> > > >  
> > > > >  
> > > > > 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.
> > > > >  
> > > >  
> > > >  
> > > > What are CLoS IDs and what are they used for ?
> > >  
> > > Effectively an ID for the allocation. The hardware has a limited number
> > > of them, in this case 4. I can't remember whether that number is
> > > per-bank, but it would not make much sense otherwise.
> > >  
> >  
> >  
> > So, if guests are requesting a private cache allocation, and cos id == 4,
> > then we can only run 4 guests ?
> >  
> >  
> 
> 
> I think yes, but it’s per bank resource (the bank here is equal to cache
> id), if you have 2 banks , you can create 8 guests (each has 1 bank
> allocation)

Ok, well if it is resource limiting in this way, then I think it is important
to expose in the capabilities. This information would be needed by openstack
schedular in order to avoid placing too guests on the same host when they have
requested cache allocation.

Picking a more obvious attr name would be nice though eg  'nallocations'

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

On Thursday, 6 April 2017 at 5:51 PM, Daniel P. Berrange wrote:

> On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> > 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'/>
> > > 
> > 
> > 
> 
> 
> Either the XML is malformed, or the indentation is wrong. The indentation
> suggests you want nested XML elements, but the parent element is an empty
> tag, so you've actually got a flat namespace here.
> 

<cache>
  <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
    <control min='768' unit='KiB' type='unified' nallocations='4'/>
  <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
    <control min='768' unit='KiB' type='unified' nallocations='4'/>
</cache>


I validate it online, no errors, the result is:
 
http://www.xmlvalidation.com/index.php?id=1&L=0&target=/xmlvalidation/start.jsp;jsessionid=5B8746DEE269CE0ADFB1B8F88CF5E94F
 
> 
> > > 
> > 
> > 
> > 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.
> > 
> 
> 
> What are CLoS IDs and what are they used for ?
> 
> 
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
> 
> --
> 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, Apr 06, 2017 at 07:31:00PM +0800, Eli Qiao wrote:
> 
> 
> On Thursday, 6 April 2017 at 5:51 PM, Daniel P. Berrange wrote:
> 
> > On Thu, Apr 06, 2017 at 11:49:14AM +0200, Martin Kletzander wrote:
> > > 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'/>
> > > > 
> > > 
> > > 
> > 
> > 
> > Either the XML is malformed, or the indentation is wrong. The indentation
> > suggests you want nested XML elements, but the parent element is an empty
> > tag, so you've actually got a flat namespace here.
> > 
> 
> <cache>
>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
>     <control min='768' unit='KiB' type='unified' nallocations='4'/>
>   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
>     <control min='768' unit='KiB' type='unified' nallocations='4'/>
> </cache>
> 
> 
> I validate it online, no errors, the result is:

You've checked the XML syntax, but *not* the semantics.

You've implemented this:

 <cache>
   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
   <control min='768' unit='KiB' type='unified' nallocations='4'/>
   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
   <control min='768' unit='KiB' type='unified' nallocations='4'/>
 </cache>

but indentation suggests you meant to do

 <cache>
   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
     <control min='768' unit='KiB' type='unified' nallocations='4'/>
   </bank>
   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
     <control min='768' unit='KiB' type='unified' nallocations='4'/>
   </bank>
 </cache>

both are valid XML, but they have completely different semantics

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

On Thursday, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:

> 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.
>  
> >  
We don’t expose it in previous discussion as in my old design, it is saved in a global internal struct, and we will use
it to limit the cache allocation number.

For now as you expose all these to capabilities XML, I want expose it out which can be reference by resctrl utils later.

Thought?

> > --
> > 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, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:

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

Just saw Daniel’s comments, we may need it, with ‘nallocations’, is that okay?

Do you want me to do a reversion now?

  
>  
> > Along with vircaps2xmltest case updated.
> >  
> > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@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.
>  
Okay, I can more it before return.  
>  
> > 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.
yes, I think we can ignore this value or not? if we enabled resctrl (we can always read correct thing from /sys/fs/resctrl/info/…)  
>  
> > +}
> > +
> > 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.
>  
Okay, I thought it was a temporary work around before we get resctrl information.

I can add it back if you think this is useful.
>  
> > /* 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.
>  
Yes, I know the reason now,
On my local test host, I have enabled resctrl with CDP, but in the test case, we don’t mock /sys/fs/resctrl,
so it pick the real one.

I will remove it.

But the solution maybe we alway mock /sys/fs/resctrl to avoid use host’s default one?

is that okay?
>  
> > </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

Ah ha, stupid me, I get it now.  
>  
> > + 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@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, Apr 06, 2017 at 06:56:09PM +0800, Eli Qiao wrote:
>
>
>On Thursday, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:
>
>> 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.
>>
>
>Just saw Daniel’s comments, we may need it, with ‘nallocations’, is that okay?
>
>Do you want me to do a reversion now?
>
>
>>
>> > Along with vircaps2xmltest case updated.
>> >
>> > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@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.
>>
>Okay, I can more it before return.
>>
>> > 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.
>yes, I think we can ignore this value or not? if we enabled resctrl (we can always read correct thing from /sys/fs/resctrl/info/…)
>>

We need to make sure we don't continue and report wrong information.
Like when VIR_APPEND_ELEMENT fails, you must report an error and fail
loading.

>> > +}
>> > +
>> > 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.
>>
>Okay, I thought it was a temporary work around before we get resctrl information.
>
>I can add it back if you think this is useful.
>>

Maybe if level >= cache_min_level || bank->control (or something like that)

>> > /* 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.
>>
>Yes, I know the reason now,
>On my local test host, I have enabled resctrl with CDP, but in the test case, we don’t mock /sys/fs/resctrl,
>so it pick the real one.
>
>I will remove it.
>
>But the solution maybe we alway mock /sys/fs/resctrl to avoid use host’s default one?
>
>is that okay?
>>

sure, we need to mock it always.  Tests should not touch any part of the system.

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

As Dan said as well, this must be:
<bank>
  <cache/>
</control>

>> > </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
>
>Ah ha, stupid me, I get it now.
>>
>> > + 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@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