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 discussionabout 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@xxxxxxxxx>---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.cindex 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/…)
+}+intvirCapabilitiesInitCaches(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, evenfor those who can't change it. If this was intentional, it should bementioned 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.hindex 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.xmlindex 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 forthese. 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.xmlindex 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.cindex 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 mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
