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