3:01 a.m.
Pushed now.
On Mon, Jun 05, 2017 at 09:31:17AM +0800, Eli Qiao wrote:
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(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
>
>