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

On Wed, May 17, 2017 at 05:08:33PM +0800, taget wrote: > From: Eli Qiao <liyong.qiao(a)intel.com (mailto:liyong.qiao@intel.com)> > > * 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(a)intel.com (mailto:liyong.qiao@intel.com)> > --- > 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