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

Eli Qiao qiaoliyong at gmail.com
Thu Apr 6 10:56:09 UTC 2017



On Thursday, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:

> On Thu, Apr 06, 2017 at 04:20:06PM +0800, Eli Qiao wrote:
> > This patch is based on Martin's cache branch.
> >  
> > This patch amends the cache bank capability as follow:
> >  
> > <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
> > <control min='768' unit='KiB' type='unified' nclos='4'/>
> > <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
> > <control min='768' unit='KiB' type='unified' nclos='4'/>
> >  
>  
>  
> Were we exposing the number of CLoS IDs before? Was there a discussion
> about it? Do we want to expose them? Probably yes, I'm just wondering.
>  

Just saw Daniel’s comments, we may need it, with ‘nallocations’, is that okay?

Do you want me to do a reversion now?

  
>  
> > Along with vircaps2xmltest case updated.
> >  
> > Signed-off-by: Eli Qiao <liyong.qiao at intel.com (mailto:liyong.qiao at intel.com)>
> > ---
> > src/conf/capabilities.c | 112 +++++++++++++++++++++--
> > src/conf/capabilities.h | 13 ++-
> > tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +
> > tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 2 +
> > tests/vircaps2xmltest.c | 11 +++
> > 5 files changed, 132 insertions(+), 8 deletions(-)
> >  
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index 416dd1a..75c0bec 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,7 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > virCapsHostCacheBankPtr *caches)
> > {
> > size_t i = 0;
> > + size_t j = 0;
> >  
> > if (!ncaches)
> > return 0;
> > @@ -900,6 +902,18 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > bank->size >> (kilos * 10),
> > kilos ? "KiB" : "B",
> > cpus_str);
> > + virBufferAdjustIndent(buf, 2);
> > + for (j = 0; j < bank->ncontrol; j++) {
> > + bool min_kilos = !(bank->controls[j]->min % 1024);
> > + virBufferAsprintf(buf,
> > + "<control min='%llu' unit='%s' "
> > + "type='%s' nclos='%u'/>\n",
> > + bank->controls[j]->min >> (min_kilos * 10),
> > + min_kilos ? "KiB" : "B",
> > + virCacheTypeToString(bank->controls[j]->type),
> > + bank->controls[j]->nclos);
> > + }
> > + virBufferAdjustIndent(buf, -2);
> >  
> > VIR_FREE(cpus_str);
> > }
> > @@ -1516,10 +1530,93 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> > if (!ptr)
> > return;
> >  
> > + size_t i;
>  
> Upstream frowns upon C99 initialization in the middle of the code.
>  
Okay, I can more it before return.  
>  
> > virBitmapFree(ptr->cpus);
> > + for (i = 0; i < ptr->ncontrol; i++)
> > + VIR_FREE(ptr->controls[i]);
> > + VIR_FREE(ptr->controls);
> > VIR_FREE(ptr);
> > }
> >  
> > +/* test which kinds 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);
> > + 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, const char* type)
> > +{
> > + int ret = -1;
> > + char *path = NULL;
> > + char *cbm_mask = NULL;
> > + virCapsHostCacheControlPtr control;
> > +
> > + if (VIR_ALLOC(control) < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueUint(&control->nclos,
> > + SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
> > + bank->level,
> > + type) < 0)
> > + goto cleanup;
> > +
> > + if (virFileReadValueString(&cbm_mask,
> > + SYSFS_RESCTRL_PATH
> > + "info/L%u%s/cbm_mask",
> > + bank->level,
> > + type) < 0)
> > + goto cleanup;
> > +
> > + virStringTrimOptionalNewline(cbm_mask);
> > +
> > + control->min = bank->size / (strlen(cbm_mask) * 4);
> > +
> > + if (STREQ("", type))
> > + control->type = VIR_CACHE_TYPE_UNIFIED;
> > + else if (STREQ("CODE", type))
> > + control->type = VIR_CACHE_TYPE_INSTRUCTION;
> > + else if (STREQ("DATA", type))
> > + control->type = VIR_CACHE_TYPE_DATA;
> > + else
> > + goto cleanup;
> > +
> > + if (VIR_APPEND_ELEMENT(bank->controls,
> > + bank->ncontrol,
> > + control) < 0)
> > + goto error;
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + VIR_FREE(path);
> > + return ret;
> > + error:
> > + VIR_FREE(control);
> > + return -1;
> >  
>  
>  
> The return value is not used anywhere.
yes, I think we can ignore this value or not? if we enabled resctrl (we can always read correct thing from /sys/fs/resctrl/info/…)  
>  
> > +}
> > +
> > int
> > virCapabilitiesInitCaches(virCapsPtr caps)
> > {
> > @@ -1533,11 +1630,6 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> > struct dirent *ent = NULL;
> > virCapsHostCacheBankPtr bank = NULL;
> >  
> > - /* Minimum level to expose in capabilities. Can be lowered or removed (with
> > - * the appropriate code below), but should not be increased, because we'd
> > - * lose information. */
> > - const int cache_min_level = 3;
> > -
> >  
>  
>  
> You are removing this and only reporting it for those we can control.
> But I believe the cache information can be valuable by itself, even
> for those who can't change it. If this was intentional, it should be
> mentioned in the commit message.
>  
Okay, I thought it was a temporary work around before we get resctrl information.

I can add it back if you think this is useful.
>  
> > /* offline CPUs don't provide cache info */
> > if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
> > return -1;
> > @@ -1595,12 +1687,19 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> > pos, ent->d_name) < 0)
> > goto cleanup;
> >  
> > - if (bank->level < cache_min_level) {
> > + if ((ret = virCapabilitiesGetCacheControlType(bank)) < 0) {
> > virCapsHostCacheBankFree(bank);
> > bank = NULL;
> > continue;
> > }
> >  
> > + if (ret == 0) {
> > + virCapabilitiesGetCacheControl(bank, "");
> > + } else if (ret == 1) {
> > + virCapabilitiesGetCacheControl(bank, "CODE");
> > + virCapabilitiesGetCacheControl(bank, "DATA");
> > + }
> > +
> > for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
> > *tmp_c = c_tolower(*tmp_c);
> >  
> > @@ -1617,6 +1716,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> > if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
> > break;
> > }
> > +
> > if (i == caps->host.ncaches) {
> > if (VIR_APPEND_ELEMENT(caps->host.caches,
> > caps->host.ncaches,
> > diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> > index e099ccc..13effdd 100644
> > --- a/src/conf/capabilities.h
> > +++ b/src/conf/capabilities.h
> > @@ -139,15 +139,22 @@ struct _virCapsHostSecModel {
> > };
> >  
> > typedef enum {
> > - VIR_CACHE_TYPE_DATA,
> > - VIR_CACHE_TYPE_INSTRUCTION,
> > VIR_CACHE_TYPE_UNIFIED,
> > + VIR_CACHE_TYPE_INSTRUCTION,
> > + VIR_CACHE_TYPE_DATA,
> >  
> > VIR_CACHE_TYPE_LAST
> > } virCacheType;
> >  
> > VIR_ENUM_DECL(virCache);
> >  
> > +typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> > +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> > +struct _virCapsHostCacheControl {
> > + unsigned long long min; /* B */
> > + virCacheType type; /* Data, Instruction or Unified */
> > + unsigned int nclos; /* number class of id */
> > +};
> > typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
> > typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;
> > struct _virCapsHostCacheBank {
> > @@ -156,6 +163,8 @@ struct _virCapsHostCacheBank {
> > unsigned long long size; /* B */
> > virCacheType type; /* Data, Instruction or Unified */
> > virBitmapPtr cpus; /* All CPUs that share this bank */
> > + size_t ncontrol;
> > + virCapsHostCacheControlPtr *controls;
> > };
> >  
> > typedef struct _virCapsHost virCapsHost;
> > diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> > index f2da28e..61269ea 100644
> > --- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> > +++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> > @@ -30,6 +30,8 @@
> > </topology>
> > <cache>
> > <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/>
> > + <control min='419430' unit='B' type='instruction' nclos='8'/>
> > + <control min='419430' unit='B' type='data' nclos='8'/>
> >  
>  
>  
> This file should have no <control/> in it. There is no resctrl for
> these. I don't see the bug immeadiatelly, though.
>  
Yes, I know the reason now,
On my local test host, I have enabled resctrl with CDP, but in the test case, we don’t mock /sys/fs/resctrl,
so it pick the real one.

I will remove it.

But the solution maybe we alway mock /sys/fs/resctrl to avoid use host’s default one?

is that okay?
>  
> > </cache>
> > </host>
> >  
> > diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> > index c30ea87..df27b94 100644
> > --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> > +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> > @@ -42,7 +42,9 @@
> > </topology>
> > <cache>
> > <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
> > + <control min='768' unit='KiB' type='unified' nclos='4'/>
> > <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
> > + <control min='768' unit='KiB' type='unified' nclos='4'/>
> > </cache>
> > </host>
> >  
> > diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> > index f590249..93f776a 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_dir = NULL;
> > int ret = -1;
> >  
> > /*
> > @@ -58,6 +59,15 @@ test_virCapabilities(const void *opaque)
> > data->resctrl ? "/system" : "") < 0)
> > goto cleanup;
> >  
> > + if (data->resctrl) {
> > + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s%s",
> >  
>  
>  
> just /resctrl instead of the last %s

Ah ha, stupid me, I get it now.  
>  
> > + abs_srcdir, data->filename,
> > + "/resctrl") < 0)
> > + goto cleanup;
> > + virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);
> > + }
> > +
> > +
> > virFileMockAddPrefix("/sys/devices/system", dir);
> > caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
> >  
> > @@ -84,6 +94,7 @@ test_virCapabilities(const void *opaque)
> >  
> > cleanup:
> > VIR_FREE(dir);
> > + VIR_FREE(resctrl_dir);
> > VIR_FREE(path);
> > VIR_FREE(capsXML);
> > virObjectUnref(caps);
> > --
> > 1.9.1
> >  
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com (mailto:libvir-list at redhat.com)
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >  
>  
>  
> --
> 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/20170406/eaf7883f/attachment-0001.htm>


More information about the libvir-list mailing list