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 <[email protected] (mailto:[email protected])> > > > > * 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 <[email protected] > > (mailto:[email protected])> > > --- > > 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 [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
