[libvirt] [PATCH V4] Expose resource control capabilites on cache bank
Eli Qiao
qiaoliyong at gmail.com
Tue Apr 11 08:14:32 UTC 2017
On Tuesday, 11 April 2017 at 3:48 PM, Peter Krempa wrote:
> On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> > This patch is based on Martin's cache branch.
> >
> > * 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.
> >
>
>
> We dont need to do that. The attributes should be lower case. The code
> can convert it to anything it needs.
>
>
what I did is that expose what the host’s sys/fs/resctrl/info looks like, if people
think we should use lower case, I am okay to change.
>
> >
> > Signed-off-by: Eli Qiao <liyong.qiao at intel.com (mailto:liyong.qiao at intel.com)>
> > ---
> >
>
>
> [...]
>
> > diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> > index 2080953..bfed4f8 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'>
> > + <choice>
> > + <value>BOTH</value>
> > + <value>CODE</value>
> > + <value>DATA</value>
> >
>
>
> Why are the values all caps? We prefer lowercase attributes in the XML.
>
Answer in previous reply.
>
> > + </choice>
> > + </attribute>
> > + <attribute name='max_allocation'>
> > + <ref name='unsignedInt'/>
> > + </attribute>
> > + </element>
> > + </zeroOrMore>
> > </element>
> > </oneOrMore>
> > </element>
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index 416dd1a..c6641d1 100644
> > --- a/src/conf/capabilities.c
> > +++ b/src/conf/capabilities.c
> > @@ -52,6 +52,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")
> >
> > @@ -873,6 +874,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;
> > @@ -894,13 +898,38 @@ 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);
> >
> > + /* Format control */
> > + virBufferAdjustIndent(&controlBuf, indent + 4);
> >
>
>
> This looks wrong. You should increase/decrease the indent by some
> number. The old value should not be used.
>
I have test case covered, please check tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
>
> > + 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",
> > + virResctrlCacheTypeToString(bank->controls[j]->scope),
> > + bank->controls[j]->max_allocation);
> > + }
> > +
> > + /* Put it all together */
> >
>
>
> You don't need to point out obvious things.
Just copy from other place, I am okay to remove it.
> > + if (virBufferUse(&controlBuf)) {
>
>
> This logic looks weird and opaque. Can you decide by any other means
> than the filling of the buffer?
>
>
It’s same meaning with bank->ncontrols > 0
this testfile will help you to easy understand what’s doing here.
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
I am okay to change if you feel it’s hard to get understand.
>
> > + virBufferAddLit(buf, ">\n");
> > + virBufferAddBuffer(buf, &controlBuf);
> > + virBufferAddLit(buf, "</bank>\n");
> > +
> > + } else {
> > + virBufferAddLit(buf, "/>\n");
> > + }
> > +
> > +
> > + virBufferFreeAndReset(&controlBuf);
> > VIR_FREE(cpus_str);
> > }
> >
> > @@ -1513,13 +1542,101 @@ 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);
> > }
> >
> > +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> > + "BOTH",
> > + "CODE",
> > + "DATA")
> > +
> > +/* test which TYPE of cache control supported
> > + * -1: don't support
> > + * 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)
> > + return -1;
> > +
> > + if (virFileExists(path)) {
> > + ret = 0;
> > + } else {
> > + VIR_FREE(path);
> > + /* for CDP enabled case, CODE and DATA will show together */
> > + 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,
> > + virResctrlCacheType scope)
> > +{
> > + int ret = -1;
> > + char *path = NULL;
> > + char *cbm_mask = NULL;
> > + virCapsHostCacheControlPtr control;
> > +
> > + if (VIR_ALLOC(control) < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueUint(&control->max_allocation,
> > + SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
> > + bank->level,
> > + scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueString(&cbm_mask,
> > + SYSFS_RESCTRL_PATH
> > + "info/L%u%s/cbm_mask",
> > + bank->level,
> > + scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> > + goto cleanup;
> > +
> > + virStringTrimOptionalNewline(cbm_mask);
> > +
> > + control->min = bank->size / (strlen(cbm_mask) * 4);
> >
>
>
> What is this calculating? Using length of a string looks weird. Please
> add a comment explaining this.
>
>
Sure, here cbm_mask is a hex, I will amend comments.
#cat resctrl/info/L3CODE/cbm_mask
fffff
>
> > +
> > + control->scope = scope;
> > +
> > + if (VIR_APPEND_ELEMENT(bank->controls,
> > + bank->ncontrols,
> > + control) < 0)
> > + goto error;
> > +
> > + ret = 0;
> >
>
>
> cbm_mask is leaked.
ops, thx for pointing it out.
>
> > +
> > + cleanup:
> > + VIR_FREE(path);
> > + return ret;
> > + error:
> > + VIR_FREE(control);
> > + return -1;
> >
>
>
> Why do you need an error label?
What should I do if VIR_APPEND_ELEMENT fail, I need to free that piece of memory.
Any suggestions?
>
> > +}
> > +
> > int
> > virCapabilitiesInitCaches(virCapsPtr caps)
> > {
> > @@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> > pos, ent->d_name) < 0)
> > goto cleanup;
> >
> > - if (bank->level < cache_min_level) {
> > + ret = virCapabilitiesGetCacheControlType(bank);
> >
>
>
> This overwrites ret. The function uses ret for the final return. This
> might break it. I'd suggest you use a different variable.
>
>
Okay, good catch.
>
> > +
> > + if ((bank->level >= cache_min_level) || (ret >= 0)) {
> > + if (ret == 0) {
> >
>
>
> Here ret is 0. (success)
>
> > + if (virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)
> > + goto cleanup;
> >
>
>
> And if this fails you return success.
I will add another tmp variable instead of using ret.
>
> > + } else if (ret == 1) {
> > + if ((virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||
> > + (virCapabilitiesGetCacheControl(bank,
> > + VIR_RESCTRL_CACHE_TYPE_DATA) < 0))
> > + goto cleanup;
> > + }
> > + } else {
> > virCapsHostCacheBankFree(bank);
> > bank = NULL;
> > continue;
> > diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> > index e099ccc..5fd3e57 100644
> > --- a/src/conf/capabilities.h
> > +++ b/src/conf/capabilities.h
> > @@ -138,10 +138,30 @@ struct _virCapsHostSecModel {
> > virCapsHostSecModelLabelPtr labels;
> > };
> >
> > +/* the enum value is equal to virCacheType, but we define a new enum
> > + as for resctrl it has different statement */
> >
>
>
> I don't know what you wanted to say with this comment.
>
My bad.
We read the cache type /sys/devices/system
cat /sys/devices/system/cpu/cpu0/cache/index3/type
Unified
but, if the host enabled CAT, the l3 cache can be divided to L3CODE L3DATA
$ ls /sys/fs/resctrl/info/
L3CODE L3DATA
Sorry for poor English, some previous discussion can be found from https://www.redhat.com/archives/libvir-list/2017-April/msg00333.html
> > +typedef enum {
> > + VIR_RESCTRL_CACHE_TYPE_BOTH,
> > + VIR_RESCTRL_CACHE_TYPE_CODE,
> > + VIR_RESCTRL_CACHE_TYPE_DATA,
> > +
> > + VIR_RESCTRL_CACHE_TYPE_LAST
> > +} virResctrlCacheType;
> > +
> > +VIR_ENUM_DECL(virResctrlCache);
> > +
> > +typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> > +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> > +struct _virCapsHostCacheControl {
> > + unsigned long long min; /* B */
> >
>
>
> B ?
unit is B.
>
> > + virResctrlCacheType scope; /* Data, Code or Both */
> > + unsigned int max_allocation; /* max number of supported allocations */
> > +};
> > +
> > typedef enum {
> > - VIR_CACHE_TYPE_DATA,
> > - VIR_CACHE_TYPE_INSTRUCTION,
> > VIR_CACHE_TYPE_UNIFIED,
> > + VIR_CACHE_TYPE_INSTRUCTION,
> > + VIR_CACHE_TYPE_DATA,
> >
>
>
> This is suspicious. Any explanation for moving them around?
Yes, this is a mistake, I’v pointed it out to martin.
Please note that this patch is based on Martin’s `cache` branch not against master, I’v wrote down
it in the commit message.
>
> > VIR_CACHE_TYPE_LAST
> > } virCacheType;
> > @@ -156,6 +176,8 @@ struct _virCapsHostCacheBank {
> > unsigned long long size; /* B */
> > virCacheType type; /* Data, Instruction or Unified */
> > virBitmapPtr cpus; /* All CPUs that share this bank */
> > + size_t ncontrols;
> > + virCapsHostCacheControlPtr *controls;
> > };
> >
> > typedef struct _virCapsHost virCapsHost;
> >
>
>
> > diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> > index f590249..db7de4c 100644
> > --- a/tests/vircaps2xmltest.c
> > +++ b/tests/vircaps2xmltest.c
> > @@ -47,6 +47,7 @@ test_virCapabilities(const void *opaque)
> > char *capsXML = NULL;
> > char *path = NULL;
> > char *dir = NULL;
> > + char *resctrl = NULL;
> > int ret = -1;
> >
> > /*
> > @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
> > data->resctrl ? "/system" : "") < 0)
> > goto cleanup;
> >
> > + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
> > + abs_srcdir,
> > + data->resctrl ? data->filename : "foo") < 0)
> >
>
>
> "foo"? Some testing code leftover?
No, foo is correct here.
This is for the test case don’t pick the system’s /sys/fs/resctrl to avoid wrong test result.
>
> > + goto cleanup;
> > +
> > +
> >
>
>
> Too many empty lines.
Sure, will remove.
>
> > + virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
> > virFileMockAddPrefix("/sys/devices/system", dir);
> > caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
> >
> >
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com (mailto:libvir-list at redhat.com)
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170411/2a102b0b/attachment-0001.htm>
More information about the libvir-list
mailing list