<div>
<br>
</div>
<div></div>
<p style="color: #A0A0A8;">On Thursday, 6 April 2017 at 8:46 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 08:20:56PM +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></blockquote><div><br></div><div>It helps a lot if you wait for a conclusion on a patch before sending</div><div>another version as soon as you can change one line.</div><div><br></div></div></div></span></blockquote><div>Okay, I will keep my patience next time, this time is just because I was working over time yesterday.</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> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'></div><div> <control min='768' unit='KiB' type='unified' nallocations='4'/></div><div> </bank></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' nallocations='4'/></div><div> </bank></div><div></cache></div></div></blockquote><div><br></div><div>I know Dan proposed "nallocations", but it sounds like one word. I</div><div>would rather use "allocations" or "max_allocs" or something</div><div>understandable. The reason for it? We have no documentation for our</div><div>capabilities XML. And nobody is trying to create one as far as I know.</div><div>So at least the naming should be more intuitive.</div><div><br></div></div></div></span></blockquote><div>yep, I will wait for the final decision. </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>Along with vircaps2xmltest case updated.</div></blockquote><div><br></div><div>I'm sensing you haven't ran the tests. Am I right?</div><div><br></div></div></div></span></blockquote><div>Why ?</div><div><br></div><div>taget@s2600wt:~/libvirt$ ./tests/vircaps2xmltest</div><div>TEST: vircaps2xmltest</div><div> .... 4 OK </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>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 | 120 ++++++++++++++++++++++-</div><div>src/conf/capabilities.h | 13 ++-</div><div>tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +-</div><div>tests/vircaps2xmltest.c | 13 +++</div><div>4 files changed, 148 insertions(+), 6 deletions(-)</div><div><br></div><div>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c</div><div>index 416dd1a..3094e48 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>@@ -894,13 +896,32 @@ virCapabilitiesFormatCaches(virBufferPtr buf,</div><div> */</div><div> virBufferAsprintf(buf,</div><div> "<bank id='%u' level='%u' type='%s' "</div><div>- "size='%llu' unit='%s' cpus='%s'/>\n",</div><div>+ "size='%llu' unit='%s' cpus='%s'",</div><div> bank->id, bank->level,</div><div> virCacheTypeToString(bank->type),</div><div> bank->size >> (kilos * 10),</div><div> kilos ? "KiB" : "B",</div><div> cpus_str);</div><div><br></div><div>+ if (bank->ncontrol > 0) {</div><div>+ virBufferAddLit(buf, ">\n");</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' nallocations='%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]->nallocations);</div><div>+ }</div><div>+ virBufferAdjustIndent(buf, -2);</div><div>+ virBufferAddLit(buf, "</bank>\n");</div><div>+ } else {</div><div>+ virBufferAddLit(buf, "/>\n");</div><div>+ }</div><div>+</div></div></blockquote><div><br></div><div>There's virBufferAddBuffer() for easier usage of this. Just grep for</div><div>childrenBuf and you'll see the usage.</div><div><br></div></div></div></span></blockquote><div>Good to know that , thx. </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> VIR_FREE(cpus_str);</div><div> }</div><div><br></div><div>@@ -1513,13 +1534,97 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,</div><div>void</div><div>virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)</div><div>{</div><div>+ size_t i;</div><div>+</div><div> if (!ptr)</div><div> return;</div><div><br></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->nallocations,</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>+}</div><div>+</div><div>int</div><div>virCapabilitiesInitCaches(virCapsPtr caps)</div><div>{</div><div>@@ -1595,7 +1700,18 @@ 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>+ ret = virCapabilitiesGetCacheControlType(bank);</div><div>+</div><div>+ if ((bank->level >= cache_min_level) || (ret >= 0)) {</div><div>+ if (ret == 0) {</div><div>+ if (virCapabilitiesGetCacheControl(bank, "") < 0)</div><div>+ goto cleanup;</div><div>+ } else if (ret == 1) {</div><div>+ if ((virCapabilitiesGetCacheControl(bank, "CODE") < 0) ||</div><div>+ (virCapabilitiesGetCacheControl(bank, "DATA") < 0))</div><div>+ goto cleanup;</div><div>+ }</div><div>+ } else {</div><div> virCapsHostCacheBankFree(bank);</div><div> bank = NULL;</div><div> continue;</div><div>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h</div><div>index e099ccc..1007c30 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 nallocations; /* number of supported allocation */</div></div></blockquote><div><br></div><div>"number of supported allocations"</div><div><br></div><blockquote type="cite"><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></blockquote><div><br></div><div>ncontrols. as I said, look at the code around, try reasoning about</div><div>differences if it's already inconsistent.</div><div><br></div><blockquote type="cite"><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-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>index c30ea87..32a9549 100644</div><div>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml</div><div>@@ -41,8 +41,12 @@</div><div> </cells></div><div> </topology></div><div> <cache></div><div>- <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/></div><div>- <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/></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' nallocations='4'/></div><div>+ </bank></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' nallocations='4'/></div><div>+ </bank></div><div> </cache></div><div> </host></div></div></blockquote><div><br></div><div>You could add new test to also show how it looks on other machines. For</div><div>example how does this look like when CDP is enabled. Just copy related</div><div>directories and files (not all of them, just use common sense) from your</div><div>machine or some machine you have access to and it differs from what we</div><div>have in the tests already.</div></div></div></span></blockquote><div>Okay, I can do that. </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>diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c</div><div>index f590249..3d1ad43 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></blockquote><div><br></div><div>in this case "dir" should be removed so the naming is more intuitive.</div><div><br></div><blockquote type="cite"><div><div> int ret = -1;</div><div><br></div><div> /*</div><div>@@ -58,6 +59,17 @@ 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/resctrl",</div><div>+ abs_srcdir, data->filename) < 0)</div><div>+ goto cleanup;</div><div>+ } else {</div><div>+ /* Mock to bad directory to avoid using /sys/fs/resctrl */</div><div>+ if (VIR_STRDUP(resctrl_dir, "") < 0)</div><div>+ goto cleanup;</div><div>+ }</div><div>+</div></div></blockquote><div><br></div><div>Why so complicated? Why not just doing the virAsprintf unconditionally?</div></div></div></span></blockquote><div>Okay, that’s good, </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>+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);</div><div> virFileMockAddPrefix("/sys/devices/system", dir);</div><div> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);</div><div><br></div><div>@@ -84,6 +96,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></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>