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

Martin Kletzander mkletzan at redhat.com
Mon Jun 5 08:01:38 UTC 2017


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 at intel.com (mailto:liyong.qiao at 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 at intel.com (mailto:liyong.qiao at 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
>>
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170605/d164cf16/attachment-0001.sig>


More information about the libvir-list mailing list