Re: [V4 RESEND PATCH] Expose resource control capabilites on cache bank

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

hi Martin
Thanks for you reviewing and I am okay with the diff as  you suggested.

Please help to push this patch.

Eli.

On Sunday, 4 June 2017 at 5:39 PM, Martin Kletzander wrote:

On Wed, May 17, 2017 at 05:08:33PM +0800, taget wrote:
From: Eli Qiao <liyong.qiao@xxxxxxxxx>

* 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@xxxxxxxxx>
---
docs/schemas/capability.rng | 20 ++++
src/conf/capabilities.c | 133 ++++++++++++++++++++-
src/conf/capabilities.h | 10 ++
.../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 | 8 ++
19 files changed, 244 insertions(+), 3 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 26f0aa2..927fc18 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'>

This should be 'type', as discussed before.

+ <choice>
+ <value>both</value>
+ <value>code</value>
+ <value>data</value>
+ </choice>
+ </attribute>

And hence this could be its own new definition, since it is already used
in the bank definition.

+ <attribute name='max_allocation'>

Since we are trying to prefer camelCase, even in the XML, and this
should be plural, I would suggest 'maxAllocs'.

+ <ref name='unsignedInt'/>
+ </attribute>
+ </element>
+ </zeroOrMore>
</element>
</oneOrMore>
</element>
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index d699b08..c4a1fdf 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -51,6 +51,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")

@@ -872,6 +873,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;
@@ -893,13 +897,35 @@ 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);

+ 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",
+ virCacheTypeToString(bank->controls[j]->scope),
+ bank->controls[j]->max_allocation);
+ }
+
+ if (virBufferUse(&controlBuf)) {
+ virBufferAddLit(buf, ">\n");
+ virBufferAddBuffer(buf, &controlBuf);
+ virBufferAddLit(buf, "</bank>\n");
+

Spurious line.

+ } else {
+ virBufferAddLit(buf, "/>\n");
+ }
+
+ virBufferFreeAndReset(&controlBuf);

No need for this, it is already done by virBufferAddBuffer.

VIR_FREE(cpus_str);
}

@@ -1519,13 +1545,102 @@ 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);
}

+/* test which TYPE of cache control supported
+ * -1: don't support
+ * 0: cat
+ * 1: cdp
+ */

I would slightly reword this comment.

+static int
+virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
+{
+ int ret = -1;
+ char *path = NULL;

Empty line here

+ 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 CDP is enabled, there will be both CODE and DATA, but it's enough to
check one of those only."

+ 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,
+ virCacheType scope)
+{
+ int ret = -1;
+ char *path = NULL;
+ char *cbm_mask = NULL;
+ char *type_upper = NULL;
+ virCapsHostCacheControlPtr control;
+
+ if (VIR_ALLOC(control) < 0)
+ goto cleanup;
+
+ if ((scope > VIR_CACHE_TYPE_BOTH)
+ && (virStringToUpper(&type_upper, virCacheTypeToString(scope)) < 0))

Order comparison on enum values should be done only in rare cases,
and there should then be some explanation why the order must be kept and
where to add what new values. I would just do != here instead.

Also the indentation is wrong, the && belongs to the previous line and
there's too much parentheses.

+ goto cleanup;
+
+ if (virFileReadValueUint(&control->max_allocation,
+ SYSFS_RESCTRL_PATH "/info/L%u%s/num_closids",
+ bank->level,
+ type_upper ? type_upper : "") < 0)
+ goto cleanup;
+
+ if (virFileReadValueString(&cbm_mask,
+ SYSFS_RESCTRL_PATH
+ "/info/L%u%s/cbm_mask",
+ bank->level,
+ type_upper ? type_upper: "") < 0)
+ goto cleanup;
+
+ virStringTrimOptionalNewline(cbm_mask);
+
+ /* cbm_mask: cache bit mask, it's in hex, eg: fffff */
+ control->min = bank->size / (strlen(cbm_mask) * 4);
+

I believe this should be multiplied by min_cbm_bits.

+ control->scope = scope;
+
+ if (VIR_APPEND_ELEMENT(bank->controls,
+ bank->ncontrols,
+ control) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(path);
+ VIR_FREE(cbm_mask);
+ VIR_FREE(type_upper);
+ VIR_FREE(control);
+ return ret;
+}
+
int
virCapabilitiesInitCaches(virCapsPtr caps)
{
@@ -1534,6 +1649,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)
ssize_t pos = -1;
DIR *dirp = NULL;
int ret = -1;
+ int typeret;
char *path = NULL;
char *type = NULL;
struct dirent *ent = NULL;
@@ -1607,12 +1723,27 @@ virCapabilitiesInitCaches(virCapsPtr caps)
SYSFS_SYSTEM_PATH, pos, ent->d_name) < 0)
goto cleanup;

+ typeret = virCapabilitiesGetCacheControlType(bank);
+
+ if (typeret == 0) {
+ if (virCapabilitiesGetCacheControl(bank,
+ VIR_CACHE_TYPE_BOTH) < 0)
+ goto cleanup;
+ } else if (typeret == 1) {
+ if ((virCapabilitiesGetCacheControl(bank,
+ VIR_CACHE_TYPE_CODE) < 0) ||
+ (virCapabilitiesGetCacheControl(bank,
+ VIR_CACHE_TYPE_DATA) < 0))

Again, wrong indentation and unnecessary parentheses.

Otherwise it looks good, so ACK with those differences and reworded
commit message. Let me know if you agree and I can push it immediately.
The suggested diff follows:

diff --git i/docs/schemas/capability.rng w/docs/schemas/capability.rng
index 927fc184de41..e5cbfa362ec0 100644
--- i/docs/schemas/capability.rng
+++ w/docs/schemas/capability.rng
@@ -261,13 +261,7 @@
<attribute name='level'>
<ref name='unsignedInt'/>
</attribute>
- <attribute name='type'>
- <choice>
- <value>both</value>
- <value>code</value>
- <value>data</value>
- </choice>
- </attribute>
+ <ref name='cacheType'/>
<attribute name='size'>
<ref name='unsignedInt'/>
</attribute>
@@ -285,14 +279,8 @@
<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='cacheType'/>
+ <attribute name='maxAllocs'>
<ref name='unsignedInt'/>
</attribute>
</element>
@@ -302,6 +290,16 @@
</element>
</define>

+ <define name='cacheType'>
+ <attribute name='type'>
+ <choice>
+ <value>both</value>
+ <value>code</value>
+ <value>data</value>
+ </choice>
+ </attribute>
+ </define>
+
<define name='guestcaps'>
<element name='guest'>
<ref name='ostype'/>
diff --git i/src/conf/capabilities.c w/src/conf/capabilities.c
index 8cd2957e9c88..3becc7e18c62 100644
--- i/src/conf/capabilities.c
+++ w/src/conf/capabilities.c
@@ -909,7 +909,7 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
bool min_kilos = !(bank->controls[j]->min % 1024);
virBufferAsprintf(&controlBuf,
"<control min='%llu' unit='%s' "
- "scope='%s' max_allocation='%u'/>\n",
+ "type='%s' maxAllocs='%u'/>\n",
bank->controls[j]->min >> (min_kilos * 10),
min_kilos ? "KiB" : "B",
virCacheTypeToString(bank->controls[j]->scope),
@@ -920,12 +920,10 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &controlBuf);
virBufferAddLit(buf, "</bank>\n");
-
} else {
virBufferAddLit(buf, "/>\n");
}

- virBufferFreeAndReset(&controlBuf);
VIR_FREE(cpus_str);
}

@@ -1557,16 +1555,19 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
VIR_FREE(ptr);
}

-/* test which TYPE of cache control supported
- * -1: don't support
- * 0: cat
- * 1: cdp
+/*
+ * This function tests which TYPE of cache control is supported
+ * Return values are:
+ * -1: not supported
+ * 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)
@@ -1576,7 +1577,10 @@ virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
ret = 0;
} else {
VIR_FREE(path);
- /* for CDP enabled case, CODE and DATA will show together */
+ /*
+ * If CDP is enabled, there will be both CODE and DATA, but it's enough
+ * to check one of those only.
+ */
if (virAsprintf(&path,
SYSFS_RESCTRL_PATH "/info/L%uCODE",
bank->level) < 0)
@@ -1597,13 +1601,14 @@ virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank,
char *path = NULL;
char *cbm_mask = NULL;
char *type_upper = NULL;
+ unsigned int min_cbm_bits = 0;
virCapsHostCacheControlPtr control;

if (VIR_ALLOC(control) < 0)
goto cleanup;

- if ((scope > VIR_CACHE_TYPE_BOTH)
- && (virStringToUpper(&type_upper, virCacheTypeToString(scope)) < 0))
+ if (scope != VIR_CACHE_TYPE_BOTH &&
+ virStringToUpper(&type_upper, virCacheTypeToString(scope)) < 0)
goto cleanup;

if (virFileReadValueUint(&control->max_allocation,
@@ -1619,10 +1624,16 @@ virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank,
type_upper ? type_upper: "") < 0)
goto cleanup;

+ if (virFileReadValueUint(&min_cbm_bits,
+ SYSFS_RESCTRL_PATH "/info/L%u%s/min_cbm_bits",
+ bank->level,
+ type_upper ? type_upper : "") < 0)
+ goto cleanup;
+
virStringTrimOptionalNewline(cbm_mask);

/* cbm_mask: cache bit mask, it's in hex, eg: fffff */
- control->min = bank->size / (strlen(cbm_mask) * 4);
+ control->min = min_cbm_bits * bank->size / (strlen(cbm_mask) * 4);

control->scope = scope;

@@ -1732,10 +1743,10 @@ virCapabilitiesInitCaches(virCapsPtr caps)
VIR_CACHE_TYPE_BOTH) < 0)
goto cleanup;
} else if (typeret == 1) {
- if ((virCapabilitiesGetCacheControl(bank,
- VIR_CACHE_TYPE_CODE) < 0) ||
- (virCapabilitiesGetCacheControl(bank,
- VIR_CACHE_TYPE_DATA) < 0))
+ if (virCapabilitiesGetCacheControl(bank,
+ VIR_CACHE_TYPE_CODE) < 0 ||
+ virCapabilitiesGetCacheControl(bank,
+ VIR_CACHE_TYPE_DATA) < 0)
goto cleanup;
}

diff --git i/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml w/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
index c9f460d8a227..49aa0b98ca88 100644
--- i/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
+++ w/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
@@ -42,12 +42,12 @@
</topology>
<cache>
<bank id='0' level='3' type='both' 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'/>
+ <control min='768' unit='KiB' type='code' maxAllocs='8'/>
+ <control min='768' unit='KiB' type='data' maxAllocs='8'/>
</bank>
<bank id='1' level='3' type='both' 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'/>
+ <control min='768' unit='KiB' type='code' maxAllocs='8'/>
+ <control min='768' unit='KiB' type='data' maxAllocs='8'/>
</bank>
</cache>
</host>
diff --git i/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml w/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
index 04a5eb895727..cb78b4ab788d 100644
--- i/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
+++ w/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
@@ -42,10 +42,10 @@
</topology>
<cache>
<bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'>
- <control min='768' unit='KiB' scope='both' max_allocation='4'/>
+ <control min='1536' unit='KiB' type='both' maxAllocs='4'/>
</bank>
<bank id='1' level='3' type='both' size='15360' unit='KiB' cpus='6-11'>
- <control min='768' unit='KiB' scope='both' max_allocation='4'/>
+ <control min='1536' unit='KiB' type='both' maxAllocs='4'/>
</bank>
</cache>
</host>
--
Martin
 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux