<div>
<br>
</div>
<div></div>
<p style="color: #A0A0A8;">On Tuesday, 11 April 2017 at 3:48 PM, Peter Krempa wrote:</p>
<blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;">
<span><div><div><div>On Tue, Apr 11, 2017 at 13:44:26 +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><cache></div><div> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'></div><div> <control min='768' unit='KiB' scope='BOTH' max_allocation='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' scope='BOTH' max_allocation='4'/></div><div> </bank></div><div></cache></div><div><br></div><div>For CDP enabled on x86 arch, we will have:</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' scope='CODE' max_allocation='4'/></div><div> <control min='768' unit='KiB' scope='DATA' max_allocation='4'/></div><div> </bank></div><div>...</div><div><br></div><div>* Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled</div><div>case.</div><div><br></div><div>* Along with vircaps2xmltest case updated.</div><div><br></div><div>P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I</div><div>keep it capital upper as I need to use a macro to convert from enum to</div><div>string easily.</div></div></blockquote><div><br></div><div>We dont need to do that. The attributes should be lower case. The code</div><div>can convert it to anything it needs.</div></div></div></span></blockquote><div>what I did is that expose what the host’s sys/fs/resctrl/info looks like, if people</div><div>think we should use lower case, I am okay to change. </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><br></div><div>Signed-off-by: Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>---</div></div></blockquote><div><br></div><div>[...]</div><div><br></div><blockquote type="cite"><div><div>diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng</div><div>index 2080953..bfed4f8 100644</div><div>--- a/docs/schemas/capability.rng</div><div>+++ b/docs/schemas/capability.rng</div><div>@@ -277,6 +277,26 @@</div><div> <attribute name='cpus'></div><div> <ref name='cpuset'/></div><div> </attribute></div><div>+ <zeroOrMore></div><div>+ <element name='control'></div><div>+ <attribute name='min'></div><div>+ <ref name='unsignedInt'/></div><div>+ </attribute></div><div>+ <attribute name='unit'></div><div>+ <ref name='unit'/></div><div>+ </attribute></div><div>+ <attribute name='scope'></div><div>+ <choice></div><div>+ <value>BOTH</value></div><div>+ <value>CODE</value></div><div>+ <value>DATA</value></div></div></blockquote><div><br></div><div>Why are the values all caps? We prefer lowercase attributes in the XML.</div><div><br></div></div></div></span></blockquote><div>Answer in previous reply. </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>+ </choice></div><div>+ </attribute></div><div>+ <attribute name='max_allocation'></div><div>+ <ref name='unsignedInt'/></div><div>+ </attribute></div><div>+ </element></div><div>+ </zeroOrMore></div><div> </element></div><div> </oneOrMore></div><div> </element></div><div>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c</div><div>index 416dd1a..c6641d1 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> </div><div> #define SYSFS_SYSTEM_PATH "/sys/devices/system/"</div><div>+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"</div><div> </div><div> VIR_LOG_INIT("conf.capabilities")</div><div> </div><div>@@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,</div><div> virCapsHostCacheBankPtr *caches)</div><div> {</div><div> size_t i = 0;</div><div>+ size_t j = 0;</div><div>+ int indent = virBufferGetIndent(buf, false);</div><div>+ virBuffer controlBuf = VIR_BUFFER_INITIALIZER;</div><div> </div><div> if (!ncaches)</div><div> return 0;</div><div>@@ -894,13 +898,38 @@ 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> </div><div>+ /* Format control */</div><div>+ virBufferAdjustIndent(&controlBuf, indent + 4);</div></div></blockquote><div><br></div><div>This looks wrong. You should increase/decrease the indent by some</div><div>number. The old value should not be used.</div><div><br></div></div></div></span></blockquote><div>I have test case covered, please check tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml </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>+ for (j = 0; j < bank->ncontrols; j++) {</div><div>+ bool min_kilos = !(bank->controls[j]->min % 1024);</div><div>+ virBufferAsprintf(&controlBuf,</div><div>+ "<control min='%llu' unit='%s' "</div><div>+ "scope='%s' max_allocation='%u'/>\n",</div><div>+ bank->controls[j]->min >> (min_kilos * 10),</div><div>+ min_kilos ? "KiB" : "B",</div><div>+ virResctrlCacheTypeToString(bank->controls[j]->scope),</div><div>+ bank->controls[j]->max_allocation);</div><div>+ }</div><div>+</div><div>+ /* Put it all together */</div></div></blockquote><div><br></div><div>You don't need to point out obvious things.</div></div></div></span></blockquote><div><br></div><div>Just copy from other place, I am okay to remove it. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><blockquote type="cite"><div>+ if (virBufferUse(&controlBuf)) {</div></blockquote><div><br></div><div>This logic looks weird and opaque. Can you decide by any other means</div><div>than the filling of the buffer?</div></div></div></span></blockquote><div>It’s same meaning with bank->ncontrols > 0</div><div>this testfile will help you to easy understand what’s doing here.</div><div>tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml </div><div><br></div><div>I am okay to change if you feel it’s hard to get understand.</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>+ virBufferAddLit(buf, ">\n");</div><div>+ virBufferAddBuffer(buf, &controlBuf);</div><div>+ virBufferAddLit(buf, "</bank>\n");</div><div>+</div><div>+ } else {</div><div>+ virBufferAddLit(buf, "/>\n");</div><div>+ }</div><div>+</div><div>+</div><div>+ virBufferFreeAndReset(&controlBuf);</div><div> VIR_FREE(cpus_str);</div><div> }</div><div> </div><div>@@ -1513,13 +1542,101 @@ 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> </div><div> virBitmapFree(ptr->cpus);</div><div>+ for (i = 0; i < ptr->ncontrols; i++)</div><div>+ VIR_FREE(ptr->controls[i]);</div><div>+ VIR_FREE(ptr->controls);</div><div> VIR_FREE(ptr);</div><div> }</div><div> </div><div>+VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,</div><div>+ "BOTH",</div><div>+ "CODE",</div><div>+ "DATA")</div><div>+</div><div>+/* test which TYPE 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,</div><div>+ SYSFS_RESCTRL_PATH "info/L%u",</div><div>+ 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>+ /* for CDP enabled case, CODE and DATA will show together */</div><div>+ if (virAsprintf(&path,</div><div>+ SYSFS_RESCTRL_PATH "info/L%uCODE",</div><div>+ 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,</div><div>+ virResctrlCacheType scope)</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->max_allocation,</div><div>+ SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",</div><div>+ bank->level,</div><div>+ scope ? virResctrlCacheTypeToString(scope) : "") < 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>+ scope ? virResctrlCacheTypeToString(scope) : "") < 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></blockquote><div><br></div><div>What is this calculating? Using length of a string looks weird. Please</div><div>add a comment explaining this.</div></div></div></span></blockquote><div><br></div><div>Sure, here cbm_mask is a hex, I will amend comments.</div><div>#cat resctrl/info/L3CODE/cbm_mask</div><div>fffff</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>+ control->scope = scope;</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(bank->controls,</div><div>+ bank->ncontrols,</div><div>+ control) < 0)</div><div>+ goto error;</div><div>+</div><div>+ ret = 0;</div></div></blockquote><div><br></div><div>cbm_mask is leaked.</div></div></div></span></blockquote><div>ops, thx for pointing it out. </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>+ 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>Why do you need an error label?</div></div></div></span></blockquote><div><br></div><div>What should I do if VIR_APPEND_ELEMENT fail, I need to free that piece of memory.</div><div>Any suggestions? </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>@@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)</div><div> pos, ent->d_name) < 0)</div><div> goto cleanup;</div><div> </div><div>- if (bank->level < cache_min_level) {</div><div>+ ret = virCapabilitiesGetCacheControlType(bank);</div></div></blockquote><div><br></div><div>This overwrites ret. The function uses ret for the final return. This</div><div>might break it. I'd suggest you use a different variable.</div></div></div></span></blockquote><div>Okay, good catch. </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>+ if ((bank->level >= cache_min_level) || (ret >= 0)) {</div><div>+ if (ret == 0) {</div></div></blockquote><div><br></div><div>Here ret is 0. (success)</div><div><br></div><blockquote type="cite"><div><div>+ if (virCapabilitiesGetCacheControl(bank,</div><div>+ VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)</div><div>+ goto cleanup;</div></div></blockquote><div><br></div><div>And if this fails you return success.</div></div></div></span></blockquote><div>I will add another tmp variable instead of using ret. </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>+ } else if (ret == 1) {</div><div>+ if ((virCapabilitiesGetCacheControl(bank,</div><div>+ VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||</div><div>+ (virCapabilitiesGetCacheControl(bank,</div><div>+ VIR_RESCTRL_CACHE_TYPE_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..5fd3e57 100644</div><div>--- a/src/conf/capabilities.h</div><div>+++ b/src/conf/capabilities.h</div><div>@@ -138,10 +138,30 @@ struct _virCapsHostSecModel {</div><div> virCapsHostSecModelLabelPtr labels;</div><div> };</div><div> </div><div>+/* the enum value is equal to virCacheType, but we define a new enum</div><div>+ as for resctrl it has different statement */</div></div></blockquote><div><br></div><div>I don't know what you wanted to say with this comment.</div><div><br></div></div></div></span></blockquote><div>My bad.</div><div><br></div><div>We read the cache type /sys/devices/system</div><div><br></div><div><div>cat /sys/devices/system/cpu/cpu0/cache/index3/type</div><div>Unified</div></div><div><br></div><div>but, if the host enabled CAT, the l3 cache can be divided to L3CODE L3DATA</div><div><br></div><div><div>$ ls /sys/fs/resctrl/info/</div><div>L3CODE L3DATA</div></div><div><br></div><div>Sorry for poor English, some previous discussion can be found from <a href="https://www.redhat.com/archives/libvir-list/2017-April/msg00333.html">https://www.redhat.com/archives/libvir-list/2017-April/msg00333.html</a></div><div><br></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>+typedef enum {</div><div>+ VIR_RESCTRL_CACHE_TYPE_BOTH,</div><div>+ VIR_RESCTRL_CACHE_TYPE_CODE,</div><div>+ VIR_RESCTRL_CACHE_TYPE_DATA,</div><div>+</div><div>+ VIR_RESCTRL_CACHE_TYPE_LAST</div><div>+} virResctrlCacheType;</div><div>+</div><div>+VIR_ENUM_DECL(virResctrlCache);</div><div>+</div><div>+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;</div><div>+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;</div><div>+struct _virCapsHostCacheControl {</div><div>+ unsigned long long min; /* B */</div></div></blockquote><div><br></div><div>B ?</div></div></div></span></blockquote><div>unit is B. </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>+ virResctrlCacheType scope; /* Data, Code or Both */</div><div>+ unsigned int max_allocation; /* max number of supported allocations */</div><div>+};</div><div>+</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></blockquote><div><br></div><div>This is suspicious. Any explanation for moving them around?</div></div></div></span></blockquote><div>Yes, this is a mistake, I’v pointed it out to martin.</div><div>Please note that this patch is based on Martin’s `cache` branch not against master, I’v wrote down</div><div>it in the commit message.</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> VIR_CACHE_TYPE_LAST</div><div> } virCacheType;</div><div>@@ -156,6 +176,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 ncontrols;</div><div>+ virCapsHostCacheControlPtr *controls;</div><div> };</div><div> </div><div> typedef struct _virCapsHost virCapsHost;</div></div></blockquote><div><br></div><blockquote type="cite"><div><div>diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c</div><div>index f590249..db7de4c 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 = NULL;</div><div> int ret = -1;</div><div> </div><div> /*</div><div>@@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)</div><div> data->resctrl ? "/system" : "") < 0)</div><div> goto cleanup;</div><div> </div><div>+ if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",</div><div>+ abs_srcdir,</div><div>+ data->resctrl ? data->filename : "foo") < 0)</div></div></blockquote><div><br></div><div>"foo"? Some testing code leftover?</div></div></div></span></blockquote><div>No, foo is correct here.</div><div>This is for the test case don’t pick the system’s /sys/fs/resctrl to avoid wrong test result. </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>+ goto cleanup;</div><div>+</div><div>+</div></div></blockquote><div><br></div><div>Too many empty lines.</div></div></div></span></blockquote><div>Sure, will remove. </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);</div><div> virFileMockAddPrefix("/sys/devices/system", dir);</div><div> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);</div><div> </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>