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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.