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

Eli Qiao posted 1 patch 6 years, 11 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
docs/schemas/capability.rng                        |  20 +++
src/conf/capabilities.c                            | 135 ++++++++++++++++++++-
src/conf/capabilities.h                            |  26 +++-
.../vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus |   1 +
.../linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask |   1 +
.../resctrl/info/L3CODE/min_cbm_bits               |   1 +
.../resctrl/info/L3CODE/num_closids                |   1 +
.../linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask |   1 +
.../resctrl/info/L3DATA/min_cbm_bits               |   1 +
.../resctrl/info/L3DATA/num_closids                |   1 +
.../linux-resctrl-cdp/resctrl/manualres/cpus       |   1 +
.../linux-resctrl-cdp/resctrl/manualres/schemata   |   2 +
.../linux-resctrl-cdp/resctrl/manualres/tasks      |   0
.../linux-resctrl-cdp/resctrl/schemata             |   2 +
.../linux-resctrl-cdp/resctrl/tasks                |   0
tests/vircaps2xmldata/linux-resctrl-cdp/system     |   1 +
.../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml |  55 +++++++++
tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml   |   8 +-
tests/vircaps2xmltest.c                            |  10 ++
19 files changed, 261 insertions(+), 6 deletions(-)
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
create mode 120000 tests/vircaps2xmldata/linux-resctrl-cdp/system
create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
[libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Posted by Eli Qiao 6 years, 11 months ago
This patch is based on Martin's cache branch.

* This patch amends the cache bank capability as follow:

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

For CDP enabled on x86 arch, we will have:
<cache>
  <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
    <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
    <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
  </bank>
...

* Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
case.

* Along with vircaps2xmltest case updated.

P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
keep it capital upper as I need to use a macro to convert from enum to
string easily.

Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
---
 docs/schemas/capability.rng                        |  20 +++
 src/conf/capabilities.c                            | 135 ++++++++++++++++++++-
 src/conf/capabilities.h                            |  26 +++-
 .../vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus |   1 +
 .../linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask |   1 +
 .../resctrl/info/L3CODE/min_cbm_bits               |   1 +
 .../resctrl/info/L3CODE/num_closids                |   1 +
 .../linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask |   1 +
 .../resctrl/info/L3DATA/min_cbm_bits               |   1 +
 .../resctrl/info/L3DATA/num_closids                |   1 +
 .../linux-resctrl-cdp/resctrl/manualres/cpus       |   1 +
 .../linux-resctrl-cdp/resctrl/manualres/schemata   |   2 +
 .../linux-resctrl-cdp/resctrl/manualres/tasks      |   0
 .../linux-resctrl-cdp/resctrl/schemata             |   2 +
 .../linux-resctrl-cdp/resctrl/tasks                |   0
 tests/vircaps2xmldata/linux-resctrl-cdp/system     |   1 +
 .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml |  55 +++++++++
 tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml   |   8 +-
 tests/vircaps2xmltest.c                            |  10 ++
 19 files changed, 261 insertions(+), 6 deletions(-)
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
 create mode 100755 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
 create mode 120000 tests/vircaps2xmldata/linux-resctrl-cdp/system
 create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 2080953..bfed4f8 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -277,6 +277,26 @@
           <attribute name='cpus'>
             <ref name='cpuset'/>
           </attribute>
+          <zeroOrMore>
+            <element name='control'>
+              <attribute name='min'>
+                <ref name='unsignedInt'/>
+              </attribute>
+              <attribute name='unit'>
+                <ref name='unit'/>
+              </attribute>
+              <attribute name='scope'>
+                <choice>
+                  <value>BOTH</value>
+                  <value>CODE</value>
+                  <value>DATA</value>
+                </choice>
+              </attribute>
+              <attribute name='max_allocation'>
+                <ref name='unsignedInt'/>
+              </attribute>
+            </element>
+          </zeroOrMore>
         </element>
       </oneOrMore>
     </element>
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 416dd1a..c6641d1 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,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
                             virCapsHostCacheBankPtr *caches)
 {
     size_t i = 0;
+    size_t j = 0;
+    int indent = virBufferGetIndent(buf, false);
+    virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
 
     if (!ncaches)
         return 0;
@@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
          */
         virBufferAsprintf(buf,
                           "<bank id='%u' level='%u' type='%s' "
-                          "size='%llu' unit='%s' cpus='%s'/>\n",
+                          "size='%llu' unit='%s' cpus='%s'",
                           bank->id, bank->level,
                           virCacheTypeToString(bank->type),
                           bank->size >> (kilos * 10),
                           kilos ? "KiB" : "B",
                           cpus_str);
 
+        /* Format control */
+        virBufferAdjustIndent(&controlBuf, indent + 4);
+        for (j = 0; j < bank->ncontrols; j++) {
+            bool min_kilos = !(bank->controls[j]->min % 1024);
+            virBufferAsprintf(&controlBuf,
+                              "<control min='%llu' unit='%s' "
+                              "scope='%s' max_allocation='%u'/>\n",
+                              bank->controls[j]->min >> (min_kilos * 10),
+                              min_kilos ? "KiB" : "B",
+                              virResctrlCacheTypeToString(bank->controls[j]->scope),
+                              bank->controls[j]->max_allocation);
+        }
+
+        /* Put it all together */
+        if (virBufferUse(&controlBuf)) {
+            virBufferAddLit(buf, ">\n");
+            virBufferAddBuffer(buf, &controlBuf);
+            virBufferAddLit(buf, "</bank>\n");
+
+        } else {
+            virBufferAddLit(buf, "/>\n");
+        }
+
+
+        virBufferFreeAndReset(&controlBuf);
         VIR_FREE(cpus_str);
     }
 
@@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
 void
 virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
 {
+    size_t i;
+
     if (!ptr)
         return;
 
     virBitmapFree(ptr->cpus);
+    for (i = 0; i < ptr->ncontrols; i++)
+        VIR_FREE(ptr->controls[i]);
+    VIR_FREE(ptr->controls);
     VIR_FREE(ptr);
 }
 
+VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
+              "BOTH",
+              "CODE",
+              "DATA")
+
+/* test which TYPE 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);
+        /* for CDP enabled case, CODE and DATA will show together */
+        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,
+                               virResctrlCacheType scope)
+{
+    int ret = -1;
+    char *path = NULL;
+    char *cbm_mask = NULL;
+    virCapsHostCacheControlPtr control;
+
+    if (VIR_ALLOC(control) < 0)
+        goto cleanup;
+
+    if (virFileReadValueUint(&control->max_allocation,
+                             SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
+                             bank->level,
+                             scope ? virResctrlCacheTypeToString(scope) : "") < 0)
+        goto cleanup;
+
+    if (virFileReadValueString(&cbm_mask,
+                               SYSFS_RESCTRL_PATH
+                               "info/L%u%s/cbm_mask",
+                               bank->level,
+                               scope ? virResctrlCacheTypeToString(scope) : "") < 0)
+        goto cleanup;
+
+    virStringTrimOptionalNewline(cbm_mask);
+
+    control->min = bank->size / (strlen(cbm_mask) * 4);
+
+    control->scope = scope;
+
+    if (VIR_APPEND_ELEMENT(bank->controls,
+                           bank->ncontrols,
+                           control) < 0)
+        goto error;
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(path);
+    return ret;
+ error:
+    VIR_FREE(control);
+    return -1;
+}
+
 int
 virCapabilitiesInitCaches(virCapsPtr caps)
 {
@@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)
                                        pos, ent->d_name) < 0)
                 goto cleanup;
 
-            if (bank->level < cache_min_level) {
+            ret = virCapabilitiesGetCacheControlType(bank);
+
+            if ((bank->level >= cache_min_level) || (ret >= 0)) {
+                if (ret == 0) {
+                    if (virCapabilitiesGetCacheControl(bank,
+                                                       VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)
+                        goto cleanup;
+                } else if (ret == 1) {
+                    if ((virCapabilitiesGetCacheControl(bank,
+                                                        VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||
+                            (virCapabilitiesGetCacheControl(bank,
+                                                            VIR_RESCTRL_CACHE_TYPE_DATA) < 0))
+                        goto cleanup;
+                }
+            } else {
                 virCapsHostCacheBankFree(bank);
                 bank = NULL;
                 continue;
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index e099ccc..5fd3e57 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -138,10 +138,30 @@ struct _virCapsHostSecModel {
     virCapsHostSecModelLabelPtr labels;
 };
 
+/* the enum value is equal to virCacheType, but we define a new enum
+   as for resctrl it has different statement */
+typedef enum {
+    VIR_RESCTRL_CACHE_TYPE_BOTH,
+    VIR_RESCTRL_CACHE_TYPE_CODE,
+    VIR_RESCTRL_CACHE_TYPE_DATA,
+
+    VIR_RESCTRL_CACHE_TYPE_LAST
+} virResctrlCacheType;
+
+VIR_ENUM_DECL(virResctrlCache);
+
+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
+struct _virCapsHostCacheControl {
+    unsigned long long min; /* B */
+    virResctrlCacheType scope;  /* Data, Code or Both */
+    unsigned int max_allocation; /* max number of supported allocations */
+};
+
 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;
@@ -156,6 +176,8 @@ struct _virCapsHostCacheBank {
     unsigned long long size; /* B */
     virCacheType type;  /* Data, Instruction or Unified */
     virBitmapPtr cpus;  /* All CPUs that share this bank */
+    size_t ncontrols;
+    virCapsHostCacheControlPtr *controls;
 };
 
 typedef struct _virCapsHost virCapsHost;
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
new file mode 100644
index 0000000..b3a79aa
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
@@ -0,0 +1 @@
+ffffff,ffffffff,ffffffff
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
new file mode 100755
index 0000000..78031da
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
@@ -0,0 +1 @@
+fffff
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
new file mode 100755
index 0000000..d00491f
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
@@ -0,0 +1 @@
+1
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
new file mode 100755
index 0000000..45a4fb7
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
@@ -0,0 +1 @@
+8
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
new file mode 100755
index 0000000..78031da
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
@@ -0,0 +1 @@
+fffff
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
new file mode 100755
index 0000000..d00491f
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
@@ -0,0 +1 @@
+1
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
new file mode 100755
index 0000000..45a4fb7
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
@@ -0,0 +1 @@
+8
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
new file mode 100644
index 0000000..ede4cc2
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
@@ -0,0 +1 @@
+000000,00000000,00000000
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
new file mode 100644
index 0000000..a0ef381
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
@@ -0,0 +1,2 @@
+L3DATA:0=c0000;1=c0000
+L3CODE:0=30000;1=30000
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
new file mode 100644
index 0000000..e69de29
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
new file mode 100644
index 0000000..89dc76b
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
@@ -0,0 +1,2 @@
+L3DATA:0=fffff;1=fffff
+L3CODE:0=fffff;1=fffff
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks b/tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
new file mode 100644
index 0000000..e69de29
diff --git a/tests/vircaps2xmldata/linux-resctrl-cdp/system b/tests/vircaps2xmldata/linux-resctrl-cdp/system
new file mode 120000
index 0000000..2f3a1d9
--- /dev/null
+++ b/tests/vircaps2xmldata/linux-resctrl-cdp/system
@@ -0,0 +1 @@
+../linux-resctrl/system/
\ No newline at end of file
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
new file mode 100644
index 0000000..1242d2e
--- /dev/null
+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
@@ -0,0 +1,55 @@
+<capabilities>
+
+  <host>
+    <cpu>
+      <arch>x86_64</arch>
+    </cpu>
+    <power_management/>
+    <migration_features>
+      <live/>
+    </migration_features>
+    <topology>
+      <cells num='2'>
+        <cell id='0'>
+          <memory unit='KiB'>1048576</memory>
+          <pages unit='KiB' size='4'>2048</pages>
+          <pages unit='KiB' size='2048'>4096</pages>
+          <pages unit='KiB' size='1048576'>6144</pages>
+          <cpus num='6'>
+            <cpu id='0' socket_id='0' core_id='0' siblings='0'/>
+            <cpu id='1' socket_id='0' core_id='1' siblings='1'/>
+            <cpu id='2' socket_id='0' core_id='2' siblings='2'/>
+            <cpu id='3' socket_id='0' core_id='3' siblings='3'/>
+            <cpu id='4' socket_id='0' core_id='4' siblings='4'/>
+            <cpu id='5' socket_id='0' core_id='5' siblings='5'/>
+          </cpus>
+        </cell>
+        <cell id='1'>
+          <memory unit='KiB'>2097152</memory>
+          <pages unit='KiB' size='4'>4096</pages>
+          <pages unit='KiB' size='2048'>6144</pages>
+          <pages unit='KiB' size='1048576'>8192</pages>
+          <cpus num='6'>
+            <cpu id='6' socket_id='1' core_id='0' siblings='6'/>
+            <cpu id='7' socket_id='1' core_id='1' siblings='7'/>
+            <cpu id='8' socket_id='1' core_id='2' siblings='8'/>
+            <cpu id='9' socket_id='1' core_id='3' siblings='9'/>
+            <cpu id='10' socket_id='1' core_id='4' siblings='10'/>
+            <cpu id='11' socket_id='1' core_id='5' siblings='11'/>
+          </cpus>
+        </cell>
+      </cells>
+    </topology>
+    <cache>
+      <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
+        <control min='768' unit='KiB' scope='CODE' max_allocation='8'/>
+        <control min='768' unit='KiB' scope='DATA' max_allocation='8'/>
+      </bank>
+      <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
+        <control min='768' unit='KiB' scope='CODE' max_allocation='8'/>
+        <control min='768' unit='KiB' scope='DATA' max_allocation='8'/>
+      </bank>
+    </cache>
+  </host>
+
+</capabilities>
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
index c30ea87..ad2ca62 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
@@ -41,8 +41,12 @@
       </cells>
     </topology>
     <cache>
-      <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
-      <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
+      <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
+        <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
+      </bank>
+      <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
+        <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
+      </bank>
     </cache>
   </host>
 
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index f590249..db7de4c 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 = NULL;
     int ret = -1;
 
     /*
@@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
                     data->resctrl ? "/system" : "") < 0)
         goto cleanup;
 
+    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
+                    abs_srcdir,
+                    data->resctrl ? data->filename : "foo") < 0)
+        goto cleanup;
+
+
+    virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
     virFileMockAddPrefix("/sys/devices/system", dir);
     caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
 
@@ -84,6 +92,7 @@ test_virCapabilities(const void *opaque)
 
  cleanup:
     VIR_FREE(dir);
+    VIR_FREE(resctrl);
     VIR_FREE(path);
     VIR_FREE(capsXML);
     virObjectUnref(caps);
@@ -112,6 +121,7 @@ mymain(void)
     DO_TEST("caches", VIR_ARCH_X86_64);
 
     DO_TEST_FULL("resctrl", VIR_ARCH_X86_64, true, true, true);
+    DO_TEST_FULL("resctrl-cdp", VIR_ARCH_X86_64, true, true, true);
 
     return ret;
 }
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Posted by Peter Krempa 6 years, 11 months ago
On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> This patch is based on Martin's cache branch.
> 
> * This patch amends the cache bank capability as follow:
> 
> <cache>
>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>   </bank>
>   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>   </bank>
> </cache>
> 
> For CDP enabled on x86 arch, we will have:
> <cache>
>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>     <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
>     <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
>   </bank>
> ...
> 
> * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
> case.
> 
> * Along with vircaps2xmltest case updated.
> 
> P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
> keep it capital upper as I need to use a macro to convert from enum to
> string easily.

We dont need to do that. The attributes should be lower case. The code
can convert it to anything it needs.

> 
> Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
> ---

[...]

> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index 2080953..bfed4f8 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -277,6 +277,26 @@
>            <attribute name='cpus'>
>              <ref name='cpuset'/>
>            </attribute>
> +          <zeroOrMore>
> +            <element name='control'>
> +              <attribute name='min'>
> +                <ref name='unsignedInt'/>
> +              </attribute>
> +              <attribute name='unit'>
> +                <ref name='unit'/>
> +              </attribute>
> +              <attribute name='scope'>
> +                <choice>
> +                  <value>BOTH</value>
> +                  <value>CODE</value>
> +                  <value>DATA</value>

Why are the values all caps? We prefer lowercase attributes in the XML.

> +                </choice>
> +              </attribute>
> +              <attribute name='max_allocation'>
> +                <ref name='unsignedInt'/>
> +              </attribute>
> +            </element>
> +          </zeroOrMore>
>          </element>
>        </oneOrMore>
>      </element>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 416dd1a..c6641d1 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,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>                              virCapsHostCacheBankPtr *caches)
>  {
>      size_t i = 0;
> +    size_t j = 0;
> +    int indent = virBufferGetIndent(buf, false);
> +    virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
>  
>      if (!ncaches)
>          return 0;
> @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>           */
>          virBufferAsprintf(buf,
>                            "<bank id='%u' level='%u' type='%s' "
> -                          "size='%llu' unit='%s' cpus='%s'/>\n",
> +                          "size='%llu' unit='%s' cpus='%s'",
>                            bank->id, bank->level,
>                            virCacheTypeToString(bank->type),
>                            bank->size >> (kilos * 10),
>                            kilos ? "KiB" : "B",
>                            cpus_str);
>  
> +        /* Format control */
> +        virBufferAdjustIndent(&controlBuf, indent + 4);

This looks wrong. You should increase/decrease the indent by some
number. The old value should not be used.

> +        for (j = 0; j < bank->ncontrols; j++) {
> +            bool min_kilos = !(bank->controls[j]->min % 1024);
> +            virBufferAsprintf(&controlBuf,
> +                              "<control min='%llu' unit='%s' "
> +                              "scope='%s' max_allocation='%u'/>\n",
> +                              bank->controls[j]->min >> (min_kilos * 10),
> +                              min_kilos ? "KiB" : "B",
> +                              virResctrlCacheTypeToString(bank->controls[j]->scope),
> +                              bank->controls[j]->max_allocation);
> +        }
> +
> +        /* Put it all together */

You don't need to point out obvious things.

> +        if (virBufferUse(&controlBuf)) {

This logic looks weird and opaque. Can you decide by any other means
than the filling of the buffer?

> +            virBufferAddLit(buf, ">\n");
> +            virBufferAddBuffer(buf, &controlBuf);
> +            virBufferAddLit(buf, "</bank>\n");
> +
> +        } else {
> +            virBufferAddLit(buf, "/>\n");
> +        }
> +
> +
> +        virBufferFreeAndReset(&controlBuf);
>          VIR_FREE(cpus_str);
>      }
>  
> @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
>  void
>  virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
>  {
> +    size_t i;
> +
>      if (!ptr)
>          return;
>  
>      virBitmapFree(ptr->cpus);
> +    for (i = 0; i < ptr->ncontrols; i++)
> +        VIR_FREE(ptr->controls[i]);
> +    VIR_FREE(ptr->controls);
>      VIR_FREE(ptr);
>  }
>  
> +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> +              "BOTH",
> +              "CODE",
> +              "DATA")
> +
> +/* test which TYPE 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);
> +        /* for CDP enabled case, CODE and DATA will show together */
> +        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,
> +                               virResctrlCacheType scope)
> +{
> +    int ret = -1;
> +    char *path = NULL;
> +    char *cbm_mask = NULL;
> +    virCapsHostCacheControlPtr control;
> +
> +    if (VIR_ALLOC(control) < 0)
> +        goto cleanup;
> +
> +    if (virFileReadValueUint(&control->max_allocation,
> +                             SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
> +                             bank->level,
> +                             scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> +        goto cleanup;
> +
> +    if (virFileReadValueString(&cbm_mask,
> +                               SYSFS_RESCTRL_PATH
> +                               "info/L%u%s/cbm_mask",
> +                               bank->level,
> +                               scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> +        goto cleanup;
> +
> +    virStringTrimOptionalNewline(cbm_mask);
> +
> +    control->min = bank->size / (strlen(cbm_mask) * 4);

What is this calculating? Using length of a string looks weird. Please
add a comment explaining this.

> +
> +    control->scope = scope;
> +
> +    if (VIR_APPEND_ELEMENT(bank->controls,
> +                           bank->ncontrols,
> +                           control) < 0)
> +        goto error;
> +
> +    ret = 0;

cbm_mask is leaked.

> +
> + cleanup:
> +    VIR_FREE(path);
> +    return ret;
> + error:
> +    VIR_FREE(control);
> +    return -1;

Why do you need an error label?

> +}
> +
>  int
>  virCapabilitiesInitCaches(virCapsPtr caps)
>  {
> @@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>                                         pos, ent->d_name) < 0)
>                  goto cleanup;
>  
> -            if (bank->level < cache_min_level) {
> +            ret = virCapabilitiesGetCacheControlType(bank);

This overwrites ret. The function uses ret for the final return. This
might break it. I'd suggest you use a different variable.

> +
> +            if ((bank->level >= cache_min_level) || (ret >= 0)) {
> +                if (ret == 0) {

Here ret is 0. (success)

> +                    if (virCapabilitiesGetCacheControl(bank,
> +                                                       VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)
> +                        goto cleanup;

And if this fails you return success.

> +                } else if (ret == 1) {
> +                    if ((virCapabilitiesGetCacheControl(bank,
> +                                                        VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||
> +                            (virCapabilitiesGetCacheControl(bank,
> +                                                            VIR_RESCTRL_CACHE_TYPE_DATA) < 0))
> +                        goto cleanup;
> +                }
> +            } else {
>                  virCapsHostCacheBankFree(bank);
>                  bank = NULL;
>                  continue;
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index e099ccc..5fd3e57 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -138,10 +138,30 @@ struct _virCapsHostSecModel {
>      virCapsHostSecModelLabelPtr labels;
>  };
>  
> +/* the enum value is equal to virCacheType, but we define a new enum
> +   as for resctrl it has different statement */

I don't know what you wanted to say with this comment.

> +typedef enum {
> +    VIR_RESCTRL_CACHE_TYPE_BOTH,
> +    VIR_RESCTRL_CACHE_TYPE_CODE,
> +    VIR_RESCTRL_CACHE_TYPE_DATA,
> +
> +    VIR_RESCTRL_CACHE_TYPE_LAST
> +} virResctrlCacheType;
> +
> +VIR_ENUM_DECL(virResctrlCache);
> +
> +typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> +struct _virCapsHostCacheControl {
> +    unsigned long long min; /* B */

B ?

> +    virResctrlCacheType scope;  /* Data, Code or Both */
> +    unsigned int max_allocation; /* max number of supported allocations */
> +};
> +
>  typedef enum {
> -    VIR_CACHE_TYPE_DATA,
> -    VIR_CACHE_TYPE_INSTRUCTION,
>      VIR_CACHE_TYPE_UNIFIED,
> +    VIR_CACHE_TYPE_INSTRUCTION,
> +    VIR_CACHE_TYPE_DATA,

This is suspicious. Any explanation for moving them around?

>  
>      VIR_CACHE_TYPE_LAST
>  } virCacheType;
> @@ -156,6 +176,8 @@ struct _virCapsHostCacheBank {
>      unsigned long long size; /* B */
>      virCacheType type;  /* Data, Instruction or Unified */
>      virBitmapPtr cpus;  /* All CPUs that share this bank */
> +    size_t ncontrols;
> +    virCapsHostCacheControlPtr *controls;
>  };
>  
>  typedef struct _virCapsHost virCapsHost;

> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> index f590249..db7de4c 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 = NULL;
>      int ret = -1;
>  
>      /*
> @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
>                      data->resctrl ? "/system" : "") < 0)
>          goto cleanup;
>  
> +    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
> +                    abs_srcdir,
> +                    data->resctrl ? data->filename : "foo") < 0)

"foo"? Some testing code leftover?

> +        goto cleanup;
> +
> +

Too many empty lines.

> +    virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
>      virFileMockAddPrefix("/sys/devices/system", dir);
>      caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>  
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Posted by Martin Kletzander 6 years, 11 months ago
On Tue, Apr 11, 2017 at 09:48:54AM +0200, Peter Krempa wrote:
>On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
>> This patch is based on Martin's cache branch.
>>
>> * This patch amends the cache bank capability as follow:
>>
>> <cache>
>>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>>   </bank>
>>   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
>>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>>   </bank>
>> </cache>
>>
>> For CDP enabled on x86 arch, we will have:
>> <cache>
>>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>>     <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
>>     <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
>>   </bank>
>> ...
>>
>> * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
>> case.
>>
>> * Along with vircaps2xmltest case updated.
>>
>> P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
>> keep it capital upper as I need to use a macro to convert from enum to
>> string easily.
>
>We dont need to do that. The attributes should be lower case. The code
>can convert it to anything it needs.
>
>>
>> Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
>> ---

[...]

>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 416dd1a..c6641d1 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c

[...]

>> @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>>           */
>>          virBufferAsprintf(buf,
>>                            "<bank id='%u' level='%u' type='%s' "
>> -                          "size='%llu' unit='%s' cpus='%s'/>\n",
>> +                          "size='%llu' unit='%s' cpus='%s'",
>>                            bank->id, bank->level,
>>                            virCacheTypeToString(bank->type),
>>                            bank->size >> (kilos * 10),
>>                            kilos ? "KiB" : "B",
>>                            cpus_str);
>>
>> +        /* Format control */
>> +        virBufferAdjustIndent(&controlBuf, indent + 4);
>
>This looks wrong. You should increase/decrease the indent by some
>number. The old value should not be used.
>

This is used everywhere in the code with childrenBuf, it's pretty widely
used and I think readable as well.  I agree with the rest of the review,
though.

>> +        for (j = 0; j < bank->ncontrols; j++) {
>> +            bool min_kilos = !(bank->controls[j]->min % 1024);
>> +            virBufferAsprintf(&controlBuf,
>> +                              "<control min='%llu' unit='%s' "
>> +                              "scope='%s' max_allocation='%u'/>\n",
>> +                              bank->controls[j]->min >> (min_kilos * 10),
>> +                              min_kilos ? "KiB" : "B",
>> +                              virResctrlCacheTypeToString(bank->controls[j]->scope),
>> +                              bank->controls[j]->max_allocation);
>> +        }
>> +
>> +        /* Put it all together */
>
>You don't need to point out obvious things.
>
>> +        if (virBufferUse(&controlBuf)) {
>
>This logic looks weird and opaque. Can you decide by any other means
>than the filling of the buffer?
>

Same here.

>> +            virBufferAddLit(buf, ">\n");
>> +            virBufferAddBuffer(buf, &controlBuf);
>> +            virBufferAddLit(buf, "</bank>\n");
>> +
>> +        } else {
>> +            virBufferAddLit(buf, "/>\n");
>> +        }
>> +
>> +
>> +        virBufferFreeAndReset(&controlBuf);
>>          VIR_FREE(cpus_str);
>>      }
>>

[...]

>> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
>> index f590249..db7de4c 100644
>> --- a/tests/vircaps2xmltest.c
>> +++ b/tests/vircaps2xmltest.c
>> @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
>>                      data->resctrl ? "/system" : "") < 0)
>>          goto cleanup;
>>
>> +    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
>> +                    abs_srcdir,
>> +                    data->resctrl ? data->filename : "foo") < 0)
>
>"foo"? Some testing code leftover?
>

This should just be:

+    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
+                    abs_srcdir, data->filename) < 0)

I think I said that in v3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Posted by Eli Qiao 6 years, 11 months ago

On Tuesday, 11 April 2017 at 4:10 PM, Martin Kletzander wrote:

> This should just be:
> 
> + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
> + abs_srcdir, data->filename) < 0)
> 
yep, stupid me. I get it now. --
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Posted by Eli Qiao 6 years, 11 months ago

On Tuesday, 11 April 2017 at 3:48 PM, Peter Krempa wrote:

> On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> > This patch is based on Martin's cache branch.
> >  
> > * This patch amends the cache bank capability as follow:
> >  
> > <cache>
> > <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
> > <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
> > </bank>
> > <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
> > <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
> > </bank>
> > </cache>
> >  
> > For CDP enabled on x86 arch, we will have:
> > <cache>
> > <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
> > <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
> > <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
> > </bank>
> > ...
> >  
> > * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
> > case.
> >  
> > * Along with vircaps2xmltest case updated.
> >  
> > P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
> > keep it capital upper as I need to use a macro to convert from enum to
> > string easily.
> >  
>  
>  
> We dont need to do that. The attributes should be lower case. The code
> can convert it to anything it needs.
>  
>  

what I did is that expose what the host’s sys/fs/resctrl/info looks like, if people
think we should use lower case, I am okay to change.  
>  
> >  
> > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@intel.com)>
> > ---
> >  
>  
>  
> [...]
>  
> > diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> > index 2080953..bfed4f8 100644
> > --- a/docs/schemas/capability.rng
> > +++ b/docs/schemas/capability.rng
> > @@ -277,6 +277,26 @@
> > <attribute name='cpus'>
> > <ref name='cpuset'/>
> > </attribute>
> > + <zeroOrMore>
> > + <element name='control'>
> > + <attribute name='min'>
> > + <ref name='unsignedInt'/>
> > + </attribute>
> > + <attribute name='unit'>
> > + <ref name='unit'/>
> > + </attribute>
> > + <attribute name='scope'>
> > + <choice>
> > + <value>BOTH</value>
> > + <value>CODE</value>
> > + <value>DATA</value>
> >  
>  
>  
> Why are the values all caps? We prefer lowercase attributes in the XML.
>  
Answer in previous reply.  
>  
> > + </choice>
> > + </attribute>
> > + <attribute name='max_allocation'>
> > + <ref name='unsignedInt'/>
> > + </attribute>
> > + </element>
> > + </zeroOrMore>
> > </element>
> > </oneOrMore>
> > </element>
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index 416dd1a..c6641d1 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,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > virCapsHostCacheBankPtr *caches)
> > {
> > size_t i = 0;
> > + size_t j = 0;
> > + int indent = virBufferGetIndent(buf, false);
> > + virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
> >  
> > if (!ncaches)
> > return 0;
> > @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > */
> > virBufferAsprintf(buf,
> > "<bank id='%u' level='%u' type='%s' "
> > - "size='%llu' unit='%s' cpus='%s'/>\n",
> > + "size='%llu' unit='%s' cpus='%s'",
> > bank->id, bank->level,
> > virCacheTypeToString(bank->type),
> > bank->size >> (kilos * 10),
> > kilos ? "KiB" : "B",
> > cpus_str);
> >  
> > + /* Format control */
> > + virBufferAdjustIndent(&controlBuf, indent + 4);
> >  
>  
>  
> This looks wrong. You should increase/decrease the indent by some
> number. The old value should not be used.
>  
I have test case covered, please check tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml  
>  
> > + for (j = 0; j < bank->ncontrols; j++) {
> > + bool min_kilos = !(bank->controls[j]->min % 1024);
> > + virBufferAsprintf(&controlBuf,
> > + "<control min='%llu' unit='%s' "
> > + "scope='%s' max_allocation='%u'/>\n",
> > + bank->controls[j]->min >> (min_kilos * 10),
> > + min_kilos ? "KiB" : "B",
> > + virResctrlCacheTypeToString(bank->controls[j]->scope),
> > + bank->controls[j]->max_allocation);
> > + }
> > +
> > + /* Put it all together */
> >  
>  
>  
> You don't need to point out obvious things.

Just copy from other place, I am okay to remove it.  
> > + if (virBufferUse(&controlBuf)) {
>  
>  
> This logic looks weird and opaque. Can you decide by any other means
> than the filling of the buffer?
>  
>  

It’s same meaning with  bank->ncontrols > 0
this testfile will help you to easy understand what’s doing here.
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml  

I am okay to change if you feel it’s hard to get understand.
>  
> > + virBufferAddLit(buf, ">\n");
> > + virBufferAddBuffer(buf, &controlBuf);
> > + virBufferAddLit(buf, "</bank>\n");
> > +
> > + } else {
> > + virBufferAddLit(buf, "/>\n");
> > + }
> > +
> > +
> > + virBufferFreeAndReset(&controlBuf);
> > VIR_FREE(cpus_str);
> > }
> >  
> > @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
> > void
> > virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> > {
> > + size_t i;
> > +
> > if (!ptr)
> > return;
> >  
> > virBitmapFree(ptr->cpus);
> > + for (i = 0; i < ptr->ncontrols; i++)
> > + VIR_FREE(ptr->controls[i]);
> > + VIR_FREE(ptr->controls);
> > VIR_FREE(ptr);
> > }
> >  
> > +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> > + "BOTH",
> > + "CODE",
> > + "DATA")
> > +
> > +/* test which TYPE 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);
> > + /* for CDP enabled case, CODE and DATA will show together */
> > + 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,
> > + virResctrlCacheType scope)
> > +{
> > + int ret = -1;
> > + char *path = NULL;
> > + char *cbm_mask = NULL;
> > + virCapsHostCacheControlPtr control;
> > +
> > + if (VIR_ALLOC(control) < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueUint(&control->max_allocation,
> > + SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
> > + bank->level,
> > + scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueString(&cbm_mask,
> > + SYSFS_RESCTRL_PATH
> > + "info/L%u%s/cbm_mask",
> > + bank->level,
> > + scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> > + goto cleanup;
> > +
> > + virStringTrimOptionalNewline(cbm_mask);
> > +
> > + control->min = bank->size / (strlen(cbm_mask) * 4);
> >  
>  
>  
> What is this calculating? Using length of a string looks weird. Please
> add a comment explaining this.
>  
>  


Sure,  here cbm_mask is a hex, I will amend comments.
#cat resctrl/info/L3CODE/cbm_mask
fffff
>  
> > +
> > + control->scope = scope;
> > +
> > + if (VIR_APPEND_ELEMENT(bank->controls,
> > + bank->ncontrols,
> > + control) < 0)
> > + goto error;
> > +
> > + ret = 0;
> >  
>  
>  
> cbm_mask is leaked.
ops, thx for pointing it out.  
>  
> > +
> > + cleanup:
> > + VIR_FREE(path);
> > + return ret;
> > + error:
> > + VIR_FREE(control);
> > + return -1;
> >  
>  
>  
> Why do you need an error label?

What should I do if VIR_APPEND_ELEMENT fail, I need to free that piece of memory.
Any suggestions?  
>  
> > +}
> > +
> > int
> > virCapabilitiesInitCaches(virCapsPtr caps)
> > {
> > @@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> > pos, ent->d_name) < 0)
> > goto cleanup;
> >  
> > - if (bank->level < cache_min_level) {
> > + ret = virCapabilitiesGetCacheControlType(bank);
> >  
>  
>  
> This overwrites ret. The function uses ret for the final return. This
> might break it. I'd suggest you use a different variable.
>  
>  

Okay, good catch.  
>  
> > +
> > + if ((bank->level >= cache_min_level) || (ret >= 0)) {
> > + if (ret == 0) {
> >  
>  
>  
> Here ret is 0. (success)
>  
> > + if (virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)
> > + goto cleanup;
> >  
>  
>  
> And if this fails you return success.
I will add another tmp variable instead of using ret.  
>  
> > + } else if (ret == 1) {
> > + if ((virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||
> > + (virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_DATA) < 0))
> > + goto cleanup;
> > + }
> > + } else {
> > virCapsHostCacheBankFree(bank);
> > bank = NULL;
> > continue;
> > diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> > index e099ccc..5fd3e57 100644
> > --- a/src/conf/capabilities.h
> > +++ b/src/conf/capabilities.h
> > @@ -138,10 +138,30 @@ struct _virCapsHostSecModel {
> > virCapsHostSecModelLabelPtr labels;
> > };
> >  
> > +/* the enum value is equal to virCacheType, but we define a new enum
> > + as for resctrl it has different statement */
> >  
>  
>  
> I don't know what you wanted to say with this comment.
>  
My bad.

We read the cache type /sys/devices/system

cat /sys/devices/system/cpu/cpu0/cache/index3/type
Unified


but, if the host enabled CAT,  the l3 cache can be divided to L3CODE L3DATA

$ ls /sys/fs/resctrl/info/
L3CODE  L3DATA


Sorry for poor English, some previous discussion can be found from https://www.redhat.com/archives/libvir-list/2017-April/msg00333.html

> > +typedef enum {
> > + VIR_RESCTRL_CACHE_TYPE_BOTH,
> > + VIR_RESCTRL_CACHE_TYPE_CODE,
> > + VIR_RESCTRL_CACHE_TYPE_DATA,
> > +
> > + VIR_RESCTRL_CACHE_TYPE_LAST
> > +} virResctrlCacheType;
> > +
> > +VIR_ENUM_DECL(virResctrlCache);
> > +
> > +typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> > +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> > +struct _virCapsHostCacheControl {
> > + unsigned long long min; /* B */
> >  
>  
>  
> B ?
unit is B.  
>  
> > + virResctrlCacheType scope; /* Data, Code or Both */
> > + unsigned int max_allocation; /* max number of supported allocations */
> > +};
> > +
> > typedef enum {
> > - VIR_CACHE_TYPE_DATA,
> > - VIR_CACHE_TYPE_INSTRUCTION,
> > VIR_CACHE_TYPE_UNIFIED,
> > + VIR_CACHE_TYPE_INSTRUCTION,
> > + VIR_CACHE_TYPE_DATA,
> >  
>  
>  
> This is suspicious. Any explanation for moving them around?
Yes, this is a mistake, I’v pointed it out to martin.
Please note that this patch is based on Martin’s `cache` branch not against master, I’v wrote down
it in the commit message.
>  
> > VIR_CACHE_TYPE_LAST
> > } virCacheType;
> > @@ -156,6 +176,8 @@ struct _virCapsHostCacheBank {
> > unsigned long long size; /* B */
> > virCacheType type; /* Data, Instruction or Unified */
> > virBitmapPtr cpus; /* All CPUs that share this bank */
> > + size_t ncontrols;
> > + virCapsHostCacheControlPtr *controls;
> > };
> >  
> > typedef struct _virCapsHost virCapsHost;
> >  
>  
>  
> > diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> > index f590249..db7de4c 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 = NULL;
> > int ret = -1;
> >  
> > /*
> > @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
> > data->resctrl ? "/system" : "") < 0)
> > goto cleanup;
> >  
> > + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
> > + abs_srcdir,
> > + data->resctrl ? data->filename : "foo") < 0)
> >  
>  
>  
> "foo"? Some testing code leftover?
No, foo is correct here.
This is for the test case don’t pick the system’s /sys/fs/resctrl to avoid wrong test result.  
>  
> > + goto cleanup;
> > +
> > +
> >  
>  
>  
> Too many empty lines.
Sure, will remove.  
>  
> > + virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
> > virFileMockAddPrefix("/sys/devices/system", dir);
> > caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
> >  
> >  
>  
>  
> --
> 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