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

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 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.

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 > > > > > > > >