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

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(a)intel.com (mailto:liyong.qiao@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(a)redhat.com (mailto:libvir-list@redhat.com) > > https://www.redhat.com/mailman/listinfo/libvir-list > > > > > -- > libvir-list mailing list > libvir-list(a)redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list > >