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

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

 


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@xxxxxxxxx>
---

[...]

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