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

Martin Kletzander mkletzan at redhat.com
Mon Jun 5 08:05:57 UTC 2017


On Mon, Jun 05, 2017 at 03:20:56PM +0800, Eli Qiao wrote:
>Martin:
>
>Please hold on merge this first.
>
>I found we still need some modification.
>
>> >
>> > 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);
>> >
>
>This could be wrong on new platform in Intel’s SKX CPU after check with platform guys.
>
>The cbm_mask is “7ff” (11 bits) on SKX. I will refine this by counting the bits.
>
>We can virFileReadValueString() then convert it to unsigned int, then count the bits of ‘1’.
>
>thought ? or a new utils function in virfile is required?
>

I missed this patch, sorry.  I was thinking about changing it to bit
counting, but I thought it will always be divisible by 4.  I will send a
trivial update to this, but it would be nice to have test cases for it.
Could you get some of that data from such a machine n the meantime?  If
not, I can just coy what we have and change the mask.

>I am not sure min_cbm_bits should be used.
>
>Besides, on old platform (haswell), the min_cbm_bits is 2, but we still can specify a cbm like
>“0x7” ) 3bit while do the cache allocation.
>

Yes, you can.  min_cbm_bits is not a granularity.  it is the minimum
number of consecutive bits that you need to have when allocating.  We
should also add the granularity there.  I'll fix that as well then.

>Later we need this on doing cache allocation(instead of read these value from the host again), we will
>need
>1. total cache size
>2. what the cache size of 1 (CBM) bit stand for, so that we can calculate how many bits we want.
>
>Maybe we need to add another filed like ’step’ (to indicate 1 bits stand for) ?
>
>> >      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/fcda9765/attachment-0001.sig>


More information about the libvir-list mailing list