<div><br></div><p style="color: #A0A0A8;">On Thursday, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:</p>
<blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;">
<span><div><div><div>On Thu, Apr 06, 2017 at 04:20:06PM +0800, Eli Qiao wrote:</div><blockquote type="cite"><div><div>This patch is based on Martin's cache branch.</div><div><br></div><div>This patch amends the cache bank capability as follow:</div><div><br></div><div><bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/></div><div> <control min='768' unit='KiB' type='unified' nclos='4'/></div><div><bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/></div><div> <control min='768' unit='KiB' type='unified' nclos='4'/></div></div></blockquote><div><br></div><div>Were we exposing the number of CLoS IDs before? Was there a discussion</div><div>about it? Do we want to expose them? Probably yes, I'm just wondering.</div><div><br></div></div></div></span></blockquote><div><br></div><div>Just saw Daniel’s comments, we may need it, with ‘nallocations’, is that okay?</div><div><br></div><div>Do you want me to do a reversion now?</div><div><br></div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div>Along with vircaps2xmltest case updated.</div><div><br></div><div>Signed-off-by: Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>---</div><div>src/conf/capabilities.c | 112 +++++++++++++++++++++--</div><div>src/conf/capabilities.h | 13 ++-</div><div>tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +</div><div>tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 2 +</div><div>tests/vircaps2xmltest.c | 11 +++</div><div>5 files changed, 132 insertions(+), 8 deletions(-)</div><div><br></div><div>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c</div><div>index 416dd1a..75c0bec 100644</div><div>--- a/src/conf/capabilities.c</div><div>+++ b/src/conf/capabilities.c</div><div>@@ -52,6 +52,7 @@</div><div>#define VIR_FROM_THIS VIR_FROM_CAPABILITIES</div><div><br></div><div>#define SYSFS_SYSTEM_PATH "/sys/devices/system/"</div><div>+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"</div><div><br></div><div>VIR_LOG_INIT("conf.capabilities")</div><div><br></div><div>@@ -873,6 +874,7 @@ virCapabilitiesFormatCaches(virBufferPtr buf,</div><div> virCapsHostCacheBankPtr *caches)</div><div>{</div><div> size_t i = 0;</div><div>+ size_t j = 0;</div><div><br></div><div> if (!ncaches)</div><div> return 0;</div><div>@@ -900,6 +902,18 @@ virCapabilitiesFormatCaches(virBufferPtr buf,</div><div> bank->size >> (kilos * 10),</div><div> kilos ? "KiB" : "B",</div><div> cpus_str);</div><div>+ virBufferAdjustIndent(buf, 2);</div><div>+ for (j = 0; j < bank->ncontrol; j++) {</div><div>+ bool min_kilos = !(bank->controls[j]->min % 1024);</div><div>+ virBufferAsprintf(buf,</div><div>+ "<control min='%llu' unit='%s' "</div><div>+ "type='%s' nclos='%u'/>\n",</div><div>+ bank->controls[j]->min >> (min_kilos * 10),</div><div>+ min_kilos ? "KiB" : "B",</div><div>+ virCacheTypeToString(bank->controls[j]->type),</div><div>+ bank->controls[j]->nclos);</div><div>+ }</div><div>+ virBufferAdjustIndent(buf, -2);</div><div><br></div><div> VIR_FREE(cpus_str);</div><div> }</div><div>@@ -1516,10 +1530,93 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)</div><div> if (!ptr)</div><div> return;</div><div><br></div><div>+ size_t i;</div></div></blockquote><div><br></div><div>Upstream frowns upon C99 initialization in the middle of the code.</div><div><br></div></div></div></span></blockquote><div>Okay, I can more it before return. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div> virBitmapFree(ptr->cpus);</div><div>+ for (i = 0; i < ptr->ncontrol; i++)</div><div>+ VIR_FREE(ptr->controls[i]);</div><div>+ VIR_FREE(ptr->controls);</div><div> VIR_FREE(ptr);</div><div>}</div><div><br></div><div>+/* test which kinds of cache control supported</div><div>+ * -1: don't support</div><div>+ * 0: cat</div><div>+ * 1: cdp</div><div>+ */</div><div>+static int</div><div>+virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)</div><div>+{</div><div>+ int ret = -1;</div><div>+ char *path = NULL;</div><div>+ if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%u", bank->level) < 0)</div><div>+ return -1;</div><div>+</div><div>+ if (virFileExists(path)) {</div><div>+ ret = 0;</div><div>+ } else {</div><div>+ VIR_FREE(path);</div><div>+ if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%uCODE", bank->level) < 0)</div><div>+ return -1;</div><div>+ if (virFileExists(path))</div><div>+ ret = 1;</div><div>+ }</div><div>+</div><div>+ VIR_FREE(path);</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+static int</div><div>+virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank, const char* type)</div><div>+{</div><div>+ int ret = -1;</div><div>+ char *path = NULL;</div><div>+ char *cbm_mask = NULL;</div><div>+ virCapsHostCacheControlPtr control;</div><div>+</div><div>+ if (VIR_ALLOC(control) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (virFileReadValueUint(&control->nclos,</div><div>+ SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",</div><div>+ bank->level,</div><div>+ type) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (virFileReadValueString(&cbm_mask,</div><div>+ SYSFS_RESCTRL_PATH</div><div>+ "info/L%u%s/cbm_mask",</div><div>+ bank->level,</div><div>+ type) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ virStringTrimOptionalNewline(cbm_mask);</div><div>+</div><div>+ control->min = bank->size / (strlen(cbm_mask) * 4);</div><div>+</div><div>+ if (STREQ("", type))</div><div>+ control->type = VIR_CACHE_TYPE_UNIFIED;</div><div>+ else if (STREQ("CODE", type))</div><div>+ control->type = VIR_CACHE_TYPE_INSTRUCTION;</div><div>+ else if (STREQ("DATA", type))</div><div>+ control->type = VIR_CACHE_TYPE_DATA;</div><div>+ else</div><div>+ goto cleanup;</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(bank->controls,</div><div>+ bank->ncontrol,</div><div>+ control) < 0)</div><div>+ goto error;</div><div>+</div><div>+ ret = 0;</div><div>+</div><div>+ cleanup:</div><div>+ VIR_FREE(path);</div><div>+ return ret;</div><div>+ error:</div><div>+ VIR_FREE(control);</div><div>+ return -1;</div></div></blockquote><div><br></div><div>The return value is not used anywhere.</div></div></div></span></blockquote><div>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/…) </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div><br></div><blockquote type="cite"><div><div>+}</div><div>+</div><div>int</div><div>virCapabilitiesInitCaches(virCapsPtr caps)</div><div>{</div><div>@@ -1533,11 +1630,6 @@ virCapabilitiesInitCaches(virCapsPtr caps)</div><div> struct dirent *ent = NULL;</div><div> virCapsHostCacheBankPtr bank = NULL;</div><div><br></div><div>- /* Minimum level to expose in capabilities. Can be lowered or removed (with</div><div>- * the appropriate code below), but should not be increased, because we'd</div><div>- * lose information. */</div><div>- const int cache_min_level = 3;</div><div>-</div></div></blockquote><div><br></div><div>You are removing this and only reporting it for those we can control.</div><div>But I believe the cache information can be valuable by itself, even</div><div>for those who can't change it. If this was intentional, it should be</div><div>mentioned in the commit message.</div><div><br></div></div></div></span></blockquote><div>Okay, I thought it was a temporary work around before we get resctrl information.</div><div><br></div><div>I can add it back if you think this is useful.</div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div> /* offline CPUs don't provide cache info */</div><div> if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)</div><div> return -1;</div><div>@@ -1595,12 +1687,19 @@ virCapabilitiesInitCaches(virCapsPtr caps)</div><div> pos, ent->d_name) < 0)</div><div> goto cleanup;</div><div><br></div><div>- if (bank->level < cache_min_level) {</div><div>+ if ((ret = virCapabilitiesGetCacheControlType(bank)) < 0) {</div><div> virCapsHostCacheBankFree(bank);</div><div> bank = NULL;</div><div> continue;</div><div> }</div><div><br></div><div>+ if (ret == 0) {</div><div>+ virCapabilitiesGetCacheControl(bank, "");</div><div>+ } else if (ret == 1) {</div><div>+ virCapabilitiesGetCacheControl(bank, "CODE");</div><div>+ virCapabilitiesGetCacheControl(bank, "DATA");</div><div>+ }</div><div>+</div><div> for (tmp_c = type; *tmp_c != '\0'; tmp_c++)</div><div> *tmp_c = c_tolower(*tmp_c);</div><div><br></div><div>@@ -1617,6 +1716,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)</div><div> if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))</div><div> break;</div><div> }</div><div>+</div><div> if (i == caps->host.ncaches) {</div><div> if (VIR_APPEND_ELEMENT(caps->host.caches,</div><div> caps->host.ncaches,</div><div>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h</div><div>index e099ccc..13effdd 100644</div><div>--- a/src/conf/capabilities.h</div><div>+++ b/src/conf/capabilities.h</div><div>@@ -139,15 +139,22 @@ struct _virCapsHostSecModel {</div><div>};</div><div><br></div><div>typedef enum {</div><div>- VIR_CACHE_TYPE_DATA,</div><div>- VIR_CACHE_TYPE_INSTRUCTION,</div><div> VIR_CACHE_TYPE_UNIFIED,</div><div>+ VIR_CACHE_TYPE_INSTRUCTION,</div><div>+ VIR_CACHE_TYPE_DATA,</div><div><br></div><div> VIR_CACHE_TYPE_LAST</div><div>} virCacheType;</div><div><br></div><div>VIR_ENUM_DECL(virCache);</div><div><br></div><div>+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;</div><div>+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;</div><div>+struct _virCapsHostCacheControl {</div><div>+ unsigned long long min; /* B */</div><div>+ virCacheType type; /* Data, Instruction or Unified */</div><div>+ unsigned int nclos; /* number class of id */</div><div>+};</div><div>typedef struct _virCapsHostCacheBank virCapsHostCacheBank;</div><div>typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;</div><div>struct _virCapsHostCacheBank {</div><div>@@ -156,6 +163,8 @@ struct _virCapsHostCacheBank {</div><div> unsigned long long size; /* B */</div><div> virCacheType type; /* Data, Instruction or Unified */</div><div> virBitmapPtr cpus; /* All CPUs that share this bank */</div><div>+ size_t ncontrol;</div><div>+ virCapsHostCacheControlPtr *controls;</div><div>};</div><div><br></div><div>typedef struct _virCapsHost virCapsHost;</div><div>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml</div><div>index f2da28e..61269ea 100644</div><div>--- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml</div><div>+++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml</div><div>@@ -30,6 +30,8 @@</div><div> </topology></div><div> <cache></div><div> <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/></div><div>+ <control min='419430' unit='B' type='instruction' nclos='8'/></div><div>+ <control min='419430' unit='B' type='data' nclos='8'/></div></div></blockquote><div><br></div><div>This file should have no <control/> in it. There is no resctrl for</div><div>these. I don't see the bug immeadiatelly, though.</div><div><br></div></div></div></span></blockquote><div>Yes, I know the reason now,</div><div>On my local test host, I have enabled resctrl with CDP, but in the test case, we don’t mock /sys/fs/resctrl,</div><div>so it pick the real one.</div><div><br></div><div>I will remove it.</div><div><br></div><div>But the solution maybe we alway mock /sys/fs/resctrl to avoid use host’s default one?</div><div><br></div><div>is that okay?</div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div> </cache></div><div> </host></div><div><br></div><div>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>index c30ea87..df27b94 100644</div><div>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>@@ -42,7 +42,9 @@</div><div> </topology></div><div> <cache></div><div> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/></div><div>+ <control min='768' unit='KiB' type='unified' nclos='4'/></div><div> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/></div><div>+ <control min='768' unit='KiB' type='unified' nclos='4'/></div><div> </cache></div><div> </host></div><div><br></div><div>diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c</div><div>index f590249..93f776a 100644</div><div>--- a/tests/vircaps2xmltest.c</div><div>+++ b/tests/vircaps2xmltest.c</div><div>@@ -47,6 +47,7 @@ test_virCapabilities(const void *opaque)</div><div> char *capsXML = NULL;</div><div> char *path = NULL;</div><div> char *dir = NULL;</div><div>+ char *resctrl_dir = NULL;</div><div> int ret = -1;</div><div><br></div><div> /*</div><div>@@ -58,6 +59,15 @@ test_virCapabilities(const void *opaque)</div><div> data->resctrl ? "/system" : "") < 0)</div><div> goto cleanup;</div><div><br></div><div>+ if (data->resctrl) {</div><div>+ if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s%s",</div></div></blockquote><div><br></div><div>just /resctrl instead of the last %s</div></div></div></span></blockquote><div><br></div><div>Ah ha, stupid me, I get it now. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div><br></div><blockquote type="cite"><div><div>+ abs_srcdir, data->filename,</div><div>+ "/resctrl") < 0)</div><div>+ goto cleanup;</div><div>+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);</div><div>+ }</div><div>+</div><div>+</div><div> virFileMockAddPrefix("/sys/devices/system", dir);</div><div> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);</div><div><br></div><div>@@ -84,6 +94,7 @@ test_virCapabilities(const void *opaque)</div><div><br></div><div> cleanup:</div><div> VIR_FREE(dir);</div><div>+ VIR_FREE(resctrl_dir);</div><div> VIR_FREE(path);</div><div> VIR_FREE(capsXML);</div><div> virObjectUnref(caps);</div><div>--</div><div>1.9.1</div><div><br></div><div>--</div><div>libvir-list mailing list</div><div><a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a></div><div><a href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></div></div></blockquote></div><div><div>--</div><div>libvir-list mailing list</div><div><a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a></div><div><a href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></div></div></div></span>
</blockquote>
<div>
<br>
</div>