<div>
hi Martin,
</div><div><br></div><div>Thanks for the reviewing, this’s the RFC for the reimplement cache allocation, so I don’t have the</div><div>code very cleanup yet,</div><div>I have a quick summaries:</div><div><br></div><div>1. We can use flock while read/writing /sys/fs//resctrl</div><div>2. I will do some renaming for the variables (plural or singular)</div><div>3. Will consider to refine the memory </div><div><br></div><div>For the struct design, I am not sure if it’s the best way to have the schema a list, it doesn’t support </div><div>‘indexing’ by cache type (L3/L3DATA/L3CODE), how about we pre-create a struct with the supported</div><div>cache type (we know what we support), instead of reading them from /sys/fs/resctrl ?</div><div><br></div>
<div></div>
<p style="color: #A0A0A8;">On Friday, 28 April 2017 at 8:16 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 Fri, Apr 14, 2017 at 06:01:46PM +0800, Eli Qiao wrote:</div><blockquote type="cite"><div><div>This is a RFC patch for the reimplement of `support cache tune(CAT) in</div><div>libvirt`[1].</div></div></blockquote><div><br></div><div>I searched for 'lock' in this mail and found nothing...</div><div><br></div></div></div></span></blockquote><div>No, I don’t add lock for now, this is the initial patch, aimed to avoid global variable.</div><div><br></div><div>We can use flock when access /sys/fs/resctrl/ (write/read). I don’t think that’s a big deal for now. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div>This patch defines some structs to represent data struct in linux</div><div>resctrl fs which will be used later to do cache allocation.</div><div><br></div><div>The patch expose a private interface `virResctrlFreeSchemata`, which</div><div>will be used to query the cache allocation on the host.</div><div><br></div><div>Also added unit test cases to test this interface can works well.</div><div><br></div><div>There are already patch sets[2] to address it, and functional</div><div>works, but people doesn't like it cause it has global variable, and</div><div>missing unit test case for new added capabilites, etc.</div><div><br></div><div>Martin has proposed a test infra to do vircaps2xmltest, and I extened it</div><div>on top of it to extend resctrl control[3], this is kinds of new desiged</div><div>apart from [2], so I propose this RFC patch to do some rework on it.</div><div><br></div><div>[1] <a href="https://www.redhat.com/archives/libvir-list/2017-January/msg00683.html">https://www.redhat.com/archives/libvir-list/2017-January/msg00683.html</a></div><div>[2] <a href="https://www.redhat.com/archives/libvir-list/2017-March/msg00181.html">https://www.redhat.com/archives/libvir-list/2017-March/msg00181.html</a></div><div>[3] <a href="https://www.redhat.com/archives/libvir-list/2017-April/msg00516.html">https://www.redhat.com/archives/libvir-list/2017-April/msg00516.html</a></div><div>---</div><div><br></div><div>+ "L2")</div></blockquote><div><br></div><div>Is there CAT for L2 currently?</div></div></span></blockquote><div><br></div><div>No for now. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+</div><div>+/**</div><div>+ * a virResctrlDomain represents a resource control group, it's a directory</div><div>+ * under /sys/fs/resctrl.</div><div>+ * eg: /sys/fs/resctrl/CG1</div><div>+ * |-- cpus</div><div>+ * |-- schemata</div><div>+ * `-- tasks</div><div>+ * # cat schemata</div><div>+ * L3DATA:0=fffff;1=fffff</div><div>+ * L3CODE:0=fffff;1=fffff</div><div>+ *</div><div>+ * Besides, it can also represent the default resource control group of the</div><div>+ * host.</div><div>+ */</div><div>+</div><div>+typedef struct _virResctrlGroup virResctrlGroup;</div><div>+typedef virResctrlGroup *virResctrlGroupPtr;</div><div>+struct _virResctrlGroup {</div><div>+ char *name; /* resource group name, eg: CG1. If it represent host's</div></div></blockquote><div><br></div><div>s/eg/e.g./, but I feel the example is unnecessary</div><div><br></div><blockquote type="cite"><div>+ default resource group name, should be a NULL pointer */</div></blockquote><div><br></div><div>Just "NULL for the default host group" is more than enough.</div><div><br></div></div></span></blockquote><div><br></div><div>Make sense.</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div>+ size_t n_tasks; /* number of task assigned to the resource group */</div></blockquote><div><br></div><div>s/task /tasks/</div><div><br></div><blockquote type="cite"><div><div>+ char **tasks; /* task list which contains task id eg: 77454 */</div><div>+</div></div></blockquote><div><br></div><div>Why is this list of strings? Hopefully I'll see that after I burrow</div><div>down a few more lines.</div><div><br></div></div></span></blockquote><div>I tried in my old design, we need to write tasks in to sysfile one task one time, maybe I am wrong, I am okay to modify it as a string with ‘\n'</div><div><span style="color: rgb(49, 49, 49); font-family: STHeiti;">as separator, not big deal.</span> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ size_t n_schematas; /* number of schemata the resource group contains,</div><div>+ eg: 2 */</div></div></blockquote><div><br></div><div>again, no need for</div><div><br></div></div></span></blockquote><div>Sure </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div>+ virResctrlSchemataPtr *schematas; /* scheamta list */</div></blockquote><div><br></div><div>'schemata' is plural, there is no such thing as 'schematas'.</div><div></div></div></span></blockquote><div>Ops, good to know,( not native speaker :( ) </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div> </div></div></span></blockquote><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><blockquote type="cite"><div><div>+};</div><div>+</div><div>+/* All resource control groups on this host, including default resource group */</div><div>+typedef struct _virResctrlDomain virResctrlDomain;</div><div>+typedef virResctrlDomain *virResctrlDomainPtr;</div></div></blockquote><div><br></div><div>If they are on the host, why is "Domain" in the name?</div><div><br></div></div></span></blockquote><div>Prefer Host then Domain? okay, will change. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+struct _virResctrlDomain {</div><div>+ size_t n_groups; /* number of resource control group */</div><div>+ virResctrlGroupPtr *groups; /* list of resource control group */</div><div>+};</div><div>+</div><div>+void</div><div>+virResctrlFreeSchemata(virResctrlSchemataPtr ptr)</div><div>+{</div><div>+ size_t i;</div><div>+</div><div>+ if (!ptr)</div><div>+ return;</div><div>+</div><div>+ for (i = 0; i < ptr->n_schemata_items; i++)</div><div>+ VIR_FREE(ptr->schemata_items[i]);</div></div></blockquote><div><br></div><div>Who is freeing ptr->schemata_items ?</div><div><br></div><div>Also, it is common to free the @ptr here as well.</div><div><br></div></div></span></blockquote><div>Okay. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+}</div><div>+</div><div>+static void</div><div>+virResctrlFreeGroup(virResctrlGroupPtr ptr)</div><div>+{</div><div>+ size_t i;</div><div>+</div><div>+ if (!ptr)</div><div>+ return;</div><div>+</div><div>+ for (i = 0; i < ptr->n_tasks; i++)</div><div>+ VIR_FREE(ptr->tasks[i]);</div></div></blockquote><div><br></div><div>if it needs to be strings (which I still doubt), then just use</div><div>virStringList* functions.</div></div></span></blockquote><div>Okay, good to know. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+</div><div>+ for (i = 0; i < ptr->n_schematas; i++) {</div><div>+ virResctrlFreeSchemata(ptr->schematas[i]);</div><div>+ VIR_FREE(ptr->schematas[i]);</div><div>+ }</div></div></blockquote><div><br></div><div>Who is freeing ptr->schematas ?</div><div><br></div><div>Also same here with the @ptr</div><div><br></div><blockquote type="cite"><div><div>+}</div><div>+</div><div>+static void</div><div>+virResctrlFreeDomain(virResctrlDomainPtr ptr)</div><div>+{</div><div>+ size_t i;</div><div>+</div><div>+ if (!ptr)</div><div>+ return;</div><div>+</div><div>+ for (i = 0; i < ptr->n_groups; i++) {</div><div>+ virResctrlFreeGroup(ptr->groups[i]);</div><div>+ VIR_FREE(ptr->groups[i]);</div><div>+ }</div></div></blockquote><div><br></div><div>Who is freeing ptr->groups ?</div><div><br></div><div>Also same here with the @ptr</div><div><br></div><blockquote type="cite"><div><div>+}</div><div>+</div><div>+static int</div><div>+virResctrlCopySchemata(virResctrlSchemataPtr src,</div><div>+ virResctrlSchemataPtr *dst)</div><div>+{</div><div>+ size_t i;</div><div>+ virResctrlSchemataItemPtr schemataitem;</div><div>+ virResctrlSchemataPtr schemata;</div></div></blockquote><div><br></div><div>schemata_item? It's really hard to read all this. So let's get some</div><div>things straight. Each group can have some schemata (plural of</div><div>schema/scheme). Each schema can then have multiple allocations (one per</div><div>cache id or host socket or whatever is that). So if one schema is one</div><div>line of the 'schemata' file, then 'group' should have 'schemata',</div><div>'schema' (singular) can then have 'items' or 'masks' or whatever.</div><div><br></div></div></span></blockquote><div>* 1 group have 1 schemata (a schemata file)</div><div><div>taget@s2600wt:/sys/fs/resctrl/CG115:45$ cat schemata</div><div>L3DATA:0=fffff;1=fffff</div><div>L3CODE:0=fffff;1=fffff</div></div><div>* 1 schemata has multiple schema </div><div>the schemata is L3DATA:0=fffff;1=fffff</div><div>* 1 schemata has multiple ‘allocation’/‘items’/‘masks’ </div><div>the mask is 0=ffff</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>I know it sounds like bikeshedding, but it's really hard to review this</div><div>way. Maybe just removing the plurals would make it more readable.</div></div></span></blockquote><div>Sure, I will remove plurals, to make things easy reviewing.</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+</div><div>+ if (VIR_ALLOC(schemata) < 0)</div><div>+ return -1;</div><div>+</div><div>+ schemata->type = src->type;</div><div>+</div><div>+ for (i = 0; i < src->n_schemata_items; i++) {</div><div>+ if (VIR_ALLOC(schemataitem) < 0)</div><div>+ goto error;</div><div>+</div><div>+ schemataitem->cache_id = src->schemata_items[i]->cache_id;</div><div>+ schemataitem->continuous_schemata = src->schemata_items[i]->continuous_schemata;</div><div>+ schemataitem->schemata = src->schemata_items[i]->schemata;</div></div></blockquote><div><br></div><div>What I don't get is how come 'schemataitem' can have 'schemata' in it.</div><div><br></div></div></span></blockquote><div><br></div><div>Here :</div><div><br></div><div><i>schemataitem is 0=ffff</i></div><div><i>schemata is ffff.</i></div><div> </div><div>Oh, sorry, I will rename schemata to mask.</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> /That also reminds me I haven't seen Inception in a while, where's my</div><div> spinning top, BTW/</div></div></span></blockquote><div>Haha… I will find the spinning for you :), sorry for the confusing ….</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ schemataitem->size = src->schemata_items[i]->size;</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(schemata->schemata_items,</div><div>+ schemata->n_schemata_items,</div><div>+ schemataitem) < 0)</div></div></blockquote><div><br></div><div>Too many reallocations. You already know how many of them you have.</div><div>Just VIR_ALLOC_N all of them, and then just do items[i] = src->items[i].</div><div>There will also be no need for checking errors every cycle.</div><div><br></div><div>... I just came back from a bunch of lines below. If there will be</div><div>something allocated inside this item, then you can't do the simple</div><div>assignment, so you can create a function for deep-copying this struct as</div><div>well.</div><div><br></div><blockquote type="cite"><div><div>+ goto error;</div><div>+ }</div><div>+</div><div>+ *dst = schemata;</div><div>+</div><div>+ return 0;</div><div>+</div><div>+ error:</div><div>+ virResctrlFreeSchemata(schemata);</div></div></blockquote><div><br></div><div>Due to the way you did the *Free() functions, you leak 'schemata' here.</div><div><br></div></div></span></blockquote><div>Hmm… I am confused too... </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ return -1;</div><div>+}</div><div>+</div><div>+static int</div><div>+virResctrlGetSchemataString(virResctrlType type,</div><div>+ const char *name,</div><div>+ char **schemata)</div><div>+{</div><div>+ int rc = -1;</div><div>+ char *tmp = NULL;</div><div>+ char *end = NULL;</div><div>+ char *buf = NULL;</div><div>+ char *type_suffix = NULL;</div><div>+</div><div>+ if (virFileReadValueString(&buf,</div><div>+ SYSFS_RESCTRL_PATH "%s/schemata",</div><div>+ name ? name : "") < 0)</div><div>+ return -1;</div><div>+</div><div>+ if (virAsprintf(&type_suffix,</div><div>+ "%s:",</div><div>+ virResctrlTypeToString(type)) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ tmp = strstr(buf, type_suffix);</div><div>+</div></div></blockquote><div><br></div><div>You are doing all this ^^ for every schema libvirt knows. How about</div><div>reading the file once, doing virStringSplit() to split lines and then</div><div>just parsing each line? Or while you are not keeping the lines</div><div>anywhere, you can just simply walk the whole file in one or two loops</div><div>over the read string, that might be a bit more readable.</div><div><br></div></div></span></blockquote><div>Okay, good idea.</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ if (!tmp)</div><div>+ goto cleanup;</div><div>+</div><div>+ end = strchr(tmp, '\n');</div><div>+ if (end != NULL)</div><div>+ *end = '\0';</div><div>+</div><div>+ if (VIR_STRDUP(*schemata, tmp) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ rc = 0;</div><div>+</div><div>+ cleanup:</div><div>+ VIR_FREE(buf);</div><div>+ VIR_FREE(type_suffix);</div><div>+ return rc;</div><div>+}</div><div>+</div><div>+static int</div><div>+virResctrlLoadSchemata(const char* schemata_str,</div><div>+ virResctrlSchemataPtr schemata)</div><div>+{</div><div>+ VIR_DEBUG("%s, %p\n", schemata_str, schemata);</div><div>+</div><div>+ int ret = -1;</div><div>+ char **lists = NULL;</div><div>+ char **sms = NULL;</div><div>+ char **sis = NULL;</div></div></blockquote><div><br></div><div>Again, I know it sounds like bikeshedding, but these variable names...</div><div><br></div></div></span></blockquote><div>I am headache for the naming too… I will consider for better naming…</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ size_t i;</div><div>+ virResctrlSchemataItemPtr si;</div><div>+</div><div>+ /* parse L3:0=fffff;1=f */</div><div>+ lists = virStringSplit(schemata_str, ":", 2);</div><div>+</div></div></blockquote><div><br></div><div>Hint: If you are doing 'virStringSplit(..., 2)', then you probably want</div><div> to just use strchr(). </div></div></span></blockquote><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ if ((!lists) || (!lists[1]))</div><div>+ goto cleanup;</div><div>+</div><div>+ /* parse 0=fffff;1=f */</div><div>+ sms = virStringSplit(lists[1], ";", 0);</div><div>+ if (!sms)</div><div>+ goto cleanup;</div><div>+</div><div>+ for (i = 0; sms[i] != NULL; i++) {</div><div>+ /* parse 0=fffff */</div><div>+ sis = virStringSplit(sms[i], "=", 2);</div></div></blockquote><div><br></div><div>I already mentioned what's wrong here.</div></div></span></blockquote><div>use strchr() </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ if (!sis)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (VIR_ALLOC(si) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (virStrToLong_ui(sis[0], NULL, 10, &si->cache_id) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (virStrToLong_ui(sis[1], NULL, 16, &si->continuous_schemata) < 0)</div><div>+ goto cleanup;</div></div></blockquote><div><br></div><div>Indentation of gotos ^^</div><div><br></div><div>Also, why is the mask saved as unsigned long? Wouldn't keeping it as</div><div>virBitmap make more sense?</div></div></span></blockquote><div>Sure, good idea.</div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+</div><div>+ si->schemata = si->continuous_schemata;</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(schemata->schemata_items,</div><div>+ schemata->n_schemata_items,</div><div>+ si) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ }</div><div>+</div><div>+ ret = 0;</div><div>+</div><div>+ cleanup:</div><div>+ VIR_FREE(si);</div><div>+ virStringListFree(lists);</div><div>+ virStringListFree(sms);</div><div>+ virStringListFree(sis);</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+static int</div><div>+virResctrlLoadGroup(const char *name,</div><div>+ virResctrlDomainPtr dom)</div><div>+{</div><div>+ VIR_DEBUG("%s, %p\n", name, dom);</div><div>+</div></div></blockquote><div><br></div><div>At least name=%s, dom=%p</div><div><br></div><blockquote type="cite"><div><div>+ int ret = -1;</div><div>+ char *path = NULL;</div><div>+ char *schemata_str;</div><div>+ virResctrlType i;</div><div>+ int rv;</div><div>+ virResctrlGroupPtr grp;</div><div>+ virResctrlSchemataPtr schemata;</div><div>+</div><div>+ if ((virAsprintf(&path, "%s/%s", SYSFS_RESCTRL_PATH, name)) < 0)</div></div></blockquote><div><br></div><div>Why so many parentheses?</div></div></span></blockquote><div>ops. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ return -1;</div><div>+</div><div>+ if (!virFileExists(path))</div></div></blockquote><div><br></div><div>You can check '-2' as a return value of virFileReadValue*() if you use</div><div>the approach for parsing I mentioned above.</div><div><br></div><blockquote type="cite"><div><div>+ goto cleanup;</div><div>+</div><div>+ if (VIR_ALLOC(grp) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (VIR_STRDUP(grp->name, name) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ for (i = 0; i < VIR_RESCTRL_TYPE_LAST; i++) {</div><div>+ rv = virResctrlGetSchemataString(i, name, &schemata_str);</div><div>+ if (rv < 0)</div><div>+ continue;</div><div>+</div><div>+ if (VIR_ALLOC(schemata) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ schemata->type = i;</div><div>+</div><div>+ if (virResctrlLoadSchemata(schemata_str, schemata) < 0)</div><div>+ goto cleanup;</div></div></blockquote><div><br></div><div>You leak schemata if this function fails. And schemata_str.</div><div><br></div><blockquote type="cite"><div><div>+</div><div>+ VIR_FREE(schemata_str);</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(grp->schematas,</div><div>+ grp->n_schematas,</div><div>+ schemata) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ virResctrlFreeSchemata(schemata);</div></div></blockquote><div><br></div><div>I think you meant:</div><div><br></div><div> if (VIR_APPEND_ELEMENT(grp->schematas, grp->n_schematas, schemata) < 0) {</div><div> virResctrlFreeSchemata(schemata);</div><div> goto cleanup;</div><div> }</div><div><br></div><div>Right? This way you are freeing everything in the schemata that you</div><div>just appended to the group. This does not crash for you?</div><div><br></div><blockquote type="cite"><div><div>+ }</div><div>+</div><div>+ if (VIR_APPEND_ELEMENT(dom->groups,</div><div>+ dom->n_groups,</div><div>+ grp) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ ret = 0;</div><div>+</div><div>+ cleanup:</div><div>+ VIR_FREE(path);</div><div>+ virResctrlFreeGroup(grp);</div></div></blockquote><div><br></div><div>Same here. This really works for you?</div><div><br></div></div></span></blockquote><div>yes, worked for you.</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ return ret;</div><div>+}</div><div>+</div><div>+static int</div><div>+virResctrlLoadDomain(virResctrlDomainPtr dom)</div><div>+{</div><div>+ int ret = -1;</div><div>+ int rv = -1;</div><div>+ DIR *dirp = NULL;</div><div>+ char *path = NULL;</div><div>+ struct dirent *ent;</div><div>+</div><div>+ VIR_DEBUG("%s, %p\n", "", dom);</div><div>+</div><div>+ rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH);</div><div>+</div><div>+ if (rv < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ /* load default resctrl group */</div><div>+ if (virResctrlLoadGroup("", dom) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ while ((rv = virDirRead(dirp, &ent, path)) > 0) {</div><div>+ /* only read directory in resctrl */</div><div>+ if ((ent->d_type != DT_DIR) || STREQ(ent->d_name, "info"))</div><div>+ continue;</div><div>+</div></div></blockquote><div><br></div><div>Isn't the resctrl hierarchical? Maybe it was supposed to be and it</div><div>changed. I can't seem to recall that right now.</div><div><br></div></div></span></blockquote><div>No, it’s different with cgroup, so kernel team create a new system file. :(</div><div> </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ if (virResctrlLoadGroup(ent->d_name, dom) < 0)</div><div>+ goto cleanup;</div><div>+ }</div><div>+</div><div>+ ret = 0;</div><div>+</div><div>+ cleanup:</div><div>+ virDirClose(&dirp);</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+static void</div><div>+virResctrlRefreshDom(virResctrlDomainPtr dom, virResctrlType type)</div><div>+{</div><div>+ size_t i;</div><div>+ size_t j;</div><div>+ size_t k;</div><div>+</div><div>+ virResctrlGroupPtr default_grp = NULL;</div><div>+ virResctrlGroupPtr grp = NULL;</div><div>+ virResctrlSchemataPtr schemata = NULL;</div><div>+ virResctrlSchemataItemPtr schemataitem = NULL;</div><div>+</div><div>+ default_grp = dom->groups[0];</div><div>+</div><div>+ /* We are sure that the first group is the default one */</div></div></blockquote><div><br></div><div>In case that could also be mentioned when parsing them, just so any</div><div>future rework knows some other code in the tree depends on it.</div><div><br></div><blockquote type="cite"><div><div>+ for (i = 1; i < dom->n_groups; i++) {</div><div>+ grp = dom->groups[i];</div><div>+ for (j = 0; j < grp->n_schematas; j++) {</div><div>+ schemata = grp->schematas[j];</div><div>+ /* we can only calculate one type of schemata */</div><div>+ if (schemata->type != type)</div><div>+ continue;</div><div>+ for (k = 0; k < schemata->n_schemata_items; k++) {</div><div>+ schemataitem = schemata->schemata_items[k];</div><div>+ /* if the schemata = 1, ignore it */</div></div></blockquote><div><br></div><div>Any explanation for that?</div></div></span></blockquote><div> As the schemata can not be zero (hardware required), so 1 == 0 here. the lowest bit should be reserved if we don’t want to do any allocation on that cache id.</div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ if (schemataitem->continuous_schemata > 1)</div><div>+ /* calculate default schemata, it can be non-continuous */</div><div>+ default_grp->schematas[j]->schemata_items[k]->schemata &= ~(schemataitem->continuous_schemata);</div></div></blockquote><div><br></div><div>Please elaborate on what are you trying to do here.</div></div></span></blockquote><div>Think about that we have CG1 and CG2, and there’s schemata for L3 cache is </div><div>default = fffff</div><div>CG1= f0000</div><div>CG2= 00f00</div><div><br></div><div>we want to calculate the default group’s schemata</div><div><br></div><div>we need to clear the bits which has allocated by CG1 and CG2</div><div>so for each loop, we remove bits from default</div><div><br></div><div>loop 1:</div><div>~CG1 ====== > 0ffff</div><div>default &= ~CG1 =======> default = 0ffff</div><div><br></div><div>loop 2:</div><div>~CG2 ======> ff0ff</div><div><div>default &= ~CG1 =======> default = 0f0ff</div></div><div><br></div><div><br></div><div>As kernel doesn’t accept non-continus bits, so we have schemata and continuous_schemata in a group, only for do the schemata(mask) calculation.</div><div>and write the continuous_schemata back /sys/fs/resctrl/schemata file.</div><div><br></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><br></div><blockquote type="cite"><div><div>+ }</div><div>+ }</div><div>+ }</div><div>+}</div><div>+</div><div>+int virResctrlGetFreeCache(virResctrlType type,</div><div>+ virResctrlSchemataPtr *schemata)</div><div>+{</div><div>+ VIR_DEBUG("%d, %p\n", type, schemata);</div><div>+ int ret = -1;</div><div>+ size_t i;</div><div>+ virResctrlDomainPtr dom = NULL;</div><div>+ virResctrlGroupPtr grp = NULL;</div><div>+</div><div>+ if (VIR_ALLOC(dom) < 0)</div><div>+ return -1;</div><div>+</div><div>+ if (virResctrlLoadDomain(dom) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ virResctrlRefreshDom(dom, type);</div><div>+ grp = dom->groups[0];</div><div>+</div><div>+ for (i = 0; i < grp->n_schematas; i ++)</div><div>+ if (grp->schematas[i]->type == type)</div><div>+ if (virResctrlCopySchemata(grp->schematas[i], schemata) < 0)</div><div>+ goto cleanup;</div></div></blockquote><div><br></div><div>Brackets around multiline 'if' and 'for' bodies. Also if you find the</div><div>right one you still loop until you went through all of them. Luckily</div><div>there should not be two types. Otherwise this would also be a leak.</div><div><br></div><blockquote type="cite"><div><div>+</div><div>+ if (schemata != NULL)</div><div>+ ret = 0;</div></div></blockquote><div><br></div><div>You don't need the whole Copy stuff. Since you are just copying</div><div>something you are going to free anyway, you can just steal the pointer</div><div>and return it. no need to return 0/-1, no need to do a deep-copy, it</div><div>will get much simpler.</div><div><br></div><blockquote type="cite"><div><div>+</div><div>+ cleanup:</div><div>+ virResctrlFreeDomain(dom);</div><div>+ return ret;</div><div>+}</div><div>diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h</div><div>new file mode 100644</div><div>index 0000000..1b040d4</div><div>--- /dev/null</div><div>+++ b/src/util/virresctrl.h</div><div>@@ -0,0 +1,86 @@</div><div>+/*</div><div>+ * virresctrl.h: header for managing resctrl control</div><div>+ *</div><div>+ * Copyright (C) 2017 Intel, Inc.</div><div>+ *</div><div>+ * This library is free software; you can redistribute it and/or</div><div>+ * modify it under the terms of the GNU Lesser General Public</div><div>+ * License as published by the Free Software Foundation; either</div><div>+ * version 2.1 of the License, or (at your option) any later version.</div><div>+ *</div><div>+ * This library is distributed in the hope that it will be useful,</div><div>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</div><div>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU</div><div>+ * Lesser General Public License for more details.</div><div>+ *</div><div>+ * You should have received a copy of the GNU Lesser General Public</div><div>+ * License along with this library. If not, see</div><div>+ * <<a href="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</a>>.</div><div>+ *</div><div>+ * Authors:</div><div>+ * Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>+ */</div><div>+</div><div>+#ifndef __VIR_RESCTRL_H__</div><div>+# define __VIR_RESCTRL_H__</div><div>+</div><div>+#include "virutil.h"</div><div>+</div><div>+typedef enum {</div><div>+ VIR_RESCTRL_TYPE_L3,</div><div>+ VIR_RESCTRL_TYPE_L3_CODE,</div><div>+ VIR_RESCTRL_TYPE_L3_DATA,</div><div>+ VIR_RESCTRL_TYPE_L2,</div><div>+</div><div>+ VIR_RESCTRL_TYPE_LAST</div><div>+} virResctrlType;</div><div>+</div><div>+VIR_ENUM_DECL(virResctrl);</div><div>+</div><div>+/*</div><div>+ * a virResctrlSchemataItem represents one of schemata object in a</div><div>+ * resource control group.</div><div>+ * eg: 0=f</div></div></blockquote><div><br></div><div>schemata object? that's pretty vague</div></div></span></blockquote><div>Okay, I think change it to mask ’s better. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ */</div><div>+typedef struct _virResctrlSchemataItem virResctrlSchemataItem;</div><div>+typedef virResctrlSchemataItem *virResctrlSchemataItemPtr;</div><div>+struct _virResctrlSchemataItem {</div><div>+ unsigned int cache_id; /* cache resource id, eg: 0 */</div><div>+ unsigned int continuous_schemata; /* schemata, should be a continuous bits,</div><div>+ eg: f, this schemata can be persisted</div><div>+ to sysfs */</div><div>+ unsigned int schemata; /* schemata eg: f0f, a schemata which is calculated</div><div>+ at running time */</div></div></blockquote><div><br></div><div>I still don't understand the difference between continuous_schemata and</div><div>schemata. Even after reading this.</div></div></span></blockquote><div> schemata(mask) can be non-continus one e.g. 0f00f which can not be write back to kernel sys fs.</div><div>this schemata(mask) is only used for the default group ’s cache calculation. better to rename it to temp_mask I think.</div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ unsigned long long size; /* the cache size schemata represented in B,</div><div>+ eg: (min * bits of continuous_schemata) */</div></div></blockquote><div><br></div><div>I don't see it used anywhere. Just copied in one place.</div><div></div></div></span></blockquote><div>It will be used to expose to users, and can be also used while creating cache allocation for a newly created VM.</div><div>this can be calculated by the schemata(mask) </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div> </div></div></span></blockquote><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+};</div><div>+</div><div>+/*</div><div>+ * a virResctrlSchemata represents schemata objects of specific type of</div><div>+ * resource in a resource control group.</div><div>+ * eg: L3:0=f,1=ff</div><div>+ */</div><div>+typedef struct _virResctrlSchemata virResctrlSchemata;</div><div>+typedef virResctrlSchemata *virResctrlSchemataPtr;</div><div>+struct _virResctrlSchemata {</div><div>+ virResctrlType type; /* resource control type, eg: L3 */</div><div>+ size_t n_schemata_items; /* number of schemata item, eg: 2 */</div><div>+ virResctrlSchemataItemPtr *schemata_items; /* pointer list of schemata item */</div><div>+};</div><div>+</div><div>+/* Get free cache of the host, result saved in schemata */</div><div>+int virResctrlGetFreeCache(virResctrlType type,</div><div>+ virResctrlSchemataPtr *schemata);</div><div>+</div><div>+</div><div>+/* TODO Need to first define virDomainCachetunePtr */</div><div>+/* Set cache allocation for a VM domain */</div><div>+// int virResctrlSetCacheBanks(virDomainCachetunePtr cachetune,</div><div>+// unsigned char *group_name,</div><div>+// size_t n_pids,</div><div>+// pid_t *pids);</div><div>+//</div><div>+/* remove cache allocation for a VM domain */</div><div>+// int virResctrlRemoveCacheBanks(unsigned char *group_name);</div></div></blockquote><div><br></div><div>Don't add stuff you don't use in this patch unless it is 'uncomment and</div><div>compile' type of thing that just shows how stuff should be used in the</div><div>future in case of implementing something we don't yet have.</div><div><br></div><div>Also we don't use // comments (especially not in new code, there are</div><div>only few lines left from some old days, I believe)</div></div></span></blockquote><div>Okay, Since this is the RFC patch, I would like show what’t it will be like .. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+void virResctrlFreeSchemata(virResctrlSchemataPtr ptr);</div><div>+#endif</div><div>diff --git a/tests/<a href="http://Makefile.am">Makefile.am</a> b/tests/<a href="http://Makefile.am">Makefile.am</a></div><div>index 3cc828d..0e09e43 100644</div><div>--- a/tests/<a href="http://Makefile.am">Makefile.am</a></div><div>+++ b/tests/<a href="http://Makefile.am">Makefile.am</a></div><div>@@ -229,6 +229,7 @@ if WITH_LINUX</div><div>test_programs += fchosttest</div><div>test_programs += scsihosttest</div><div>test_programs += vircaps2xmltest</div><div>+test_programs += virresctrltest</div><div>test_libraries += <a href="http://virusbmock.la">virusbmock.la</a> \</div><div> <a href="http://virnetdevbandwidthmock.la">virnetdevbandwidthmock.la</a> \</div><div> <a href="http://virnumamock.la">virnumamock.la</a> \</div><div>@@ -1150,6 +1151,10 @@ vircaps2xmltest_SOURCES = \</div><div> vircaps2xmltest.c testutils.h testutils.c virfilemock.c</div><div>vircaps2xmltest_LDADD = $(LDADDS)</div><div><br></div><div>+virresctrltest_SOURCES = \</div><div>+ virresctrltest.c testutils.h testutils.c virfilemock.c</div><div>+virresctrltest_LDADD = $(LDADDS)</div><div>+</div><div>virnumamock_la_SOURCES = \</div><div> virnumamock.c</div><div>virnumamock_la_CFLAGS = $(AM_CFLAGS)</div><div>@@ -1157,7 +1162,7 @@ virnumamock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)</div><div>virnumamock_la_LIBADD = $(MOCKLIBS_LIBS)</div><div><br></div><div>else ! WITH_LINUX</div><div>-EXTRA_DIST += vircaps2xmltest.c virnumamock.c</div><div>+EXTRA_DIST += vircaps2xmltest.c virresctrltest.c virnumamock.c</div></div></blockquote><div><br></div><div>Yet another thing that's weird to review. Normally, I would expect</div><div>people to add stuff at the end or in alphabetical order. When I look</div><div>there, I see no difference and I'm rescanning the line. Why not just</div><div>add it at the end?</div></div></span></blockquote><div><br></div><div>I am just lazy, will change it after we are agree with the RFC patch. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>endif ! WITH_LINUX</div><div><br></div><div>if WITH_NSS</div><div>diff --git a/tests/virresctrldata/L3-free.schemata b/tests/virresctrldata/L3-free.schemata</div><div>new file mode 100644</div><div>index 0000000..9b47d25</div><div>--- /dev/null</div><div>+++ b/tests/virresctrldata/L3-free.schemata</div><div>@@ -0,0 +1 @@</div><div>+L3:0=1ffff;1=1ffff</div><div>diff --git a/tests/virresctrldata/L3CODE-free.schemata b/tests/virresctrldata/L3CODE-free.schemata</div><div>new file mode 100644</div><div>index 0000000..7039c45</div><div>--- /dev/null</div><div>+++ b/tests/virresctrldata/L3CODE-free.schemata</div><div>@@ -0,0 +1 @@</div><div>+L3CODE:0=cffff;1=cffff</div><div>diff --git a/tests/virresctrldata/L3DATA-free.schemata b/tests/virresctrldata/L3DATA-free.schemata</div><div>new file mode 100644</div><div>index 0000000..30f1cbd</div><div>--- /dev/null</div><div>+++ b/tests/virresctrldata/L3DATA-free.schemata</div><div>@@ -0,0 +1 @@</div><div>+L3DATA:0=3ffff;1=3ffff</div><div>diff --git a/tests/virresctrltest.c b/tests/virresctrltest.c</div><div>new file mode 100644</div><div>index 0000000..4926468</div><div>--- /dev/null</div><div>+++ b/tests/virresctrltest.c</div><div>@@ -0,0 +1,117 @@</div><div>+/*</div><div>+ * Copyright (C) Intel, Inc. 2017</div><div>+ *</div><div>+ * This library is free software; you can redistribute it and/or</div><div>+ * modify it under the terms of the GNU Lesser General Public</div><div>+ * License as published by the Free Software Foundation; either</div><div>+ * version 2.1 of the License, or (at your option) any later version.</div><div>+ *</div><div>+ * This library is distributed in the hope that it will be useful,</div><div>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</div><div>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU</div><div>+ * Lesser General Public License for more details.</div><div>+ *</div><div>+ * You should have received a copy of the GNU Lesser General Public</div><div>+ * License along with this library. If not, see</div><div>+ * <<a href="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</a>>.</div><div>+ *</div><div>+ * Authors:</div><div>+ * Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>+ */</div><div>+</div><div>+#include <config.h></div><div>+#include <stdlib.h></div><div>+</div><div>+#include "testutils.h"</div><div>+#include "virbitmap.h"</div><div>+#include "virfilemock.h"</div><div>+#include "virresctrl.h"</div><div>+</div><div>+</div><div>+#define VIR_FROM_THIS VIR_FROM_NONE</div><div>+</div><div>+struct virResctrlData {</div><div>+ const char *filename;</div><div>+ virResctrlType type;</div><div>+};</div><div>+</div><div>+static void</div><div>+GetSchemataStr(virResctrlSchemataPtr schemata, char **str)</div></div></blockquote><div><br></div><div>Is this the only function in libvirt starting with uppercase letter?</div><div><br></div></div></span></blockquote><div>Yes, it’s the only function. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+{</div><div>+ size_t i;</div></div></blockquote><div><br></div><div>Empty line after variable declarations makes it nicer to read. But</div><div>that's just my weird thing.</div></div></span></blockquote><div>Okay. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><br></div><blockquote type="cite"><div><div>+ virBuffer buf = VIR_BUFFER_INITIALIZER;</div><div>+ virBufferAsprintf(&buf, "%s:%u=%x",</div><div>+ virResctrlTypeToString(schemata->type),</div><div>+ schemata->schemata_items[0]->cache_id,</div><div>+ schemata->schemata_items[0]->schemata);</div><div>+</div><div>+ for (i = 1; i < schemata->n_schemata_items; i ++)</div><div>+ virBufferAsprintf(&buf, ";%u=%x",</div><div>+ schemata->schemata_items[i]->cache_id,</div><div>+ schemata->schemata_items[i]->schemata);</div><div>+</div><div>+ *str = virBufferContentAndReset(&buf);</div><div>+}</div><div>+</div><div>+static int</div><div>+test_virResctrl(const void *opaque)</div><div>+{</div><div>+ struct virResctrlData *data = (struct virResctrlData *) opaque;</div><div>+ char *dir = NULL;</div><div>+ char *resctrl = NULL;</div><div>+ int ret = -1;</div><div>+ virResctrlSchemataPtr schemata = NULL;</div><div>+ char *schemata_str;</div><div>+ char *schemata_file;</div><div>+</div><div>+ if (virAsprintf(&resctrl, "%s/virresctrldata/linux-%s/resctrl",</div></div></blockquote><div> </div></div></span></blockquote><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><div>No such file is added anywhere in this patch.</div><div><br></div></div></span></blockquote><div>Oh, I missed to add soft link …. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ abs_srcdir, data->filename) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ if (virAsprintf(&schemata_file, "%s/virresctrldata/%s-free.schemata",</div><div>+ abs_srcdir, virResctrlTypeToString(data->type)) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl);</div><div>+</div><div>+ if (virResctrlGetFreeCache(data->type, &schemata) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ GetSchemataStr(schemata, &schemata_str);</div><div>+</div><div>+ if (virTestCompareToFile(schemata_str, schemata_file) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ virFileMockClearPrefixes();</div><div>+</div><div>+ ret = 0;</div><div>+</div><div>+ cleanup:</div><div>+ VIR_FREE(dir);</div><div>+ VIR_FREE(resctrl);</div><div>+ VIR_FREE(schemata_str);</div><div>+ virResctrlFreeSchemata(schemata);</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+static int</div><div>+mymain(void)</div><div>+{</div><div>+ int ret = 0;</div><div>+</div><div>+#define DO_TEST_FULL(filename, type) \</div><div>+ do { \</div><div>+ struct virResctrlData data = {filename, \</div><div>+ type}; \</div><div>+ if (virTestRun(filename, test_virResctrl, &data) < 0) \</div><div>+ ret = -1; \</div><div>+ } while (0)</div><div>+</div><div>+ DO_TEST_FULL("resctrl", VIR_RESCTRL_TYPE_L3);</div><div>+ DO_TEST_FULL("resctrl-cdp", VIR_RESCTRL_TYPE_L3_CODE);</div><div>+ DO_TEST_FULL("resctrl-cdp", VIR_RESCTRL_TYPE_L3_DATA);</div><div>+</div></div></blockquote><div><br></div><div>So this really does not fail for you? Maybe check that you haven't</div><div>missed adding some files into git's index?</div><div> </div></div></span></blockquote><div>I missed to add it to git. </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div></div><blockquote type="cite"><div><div>+ return ret;</div><div>+}</div><div>+</div><div>+VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virnumamock.so")</div></div></blockquote><div><br></div><div>What is the need for this?</div><div><br></div><blockquote type="cite"><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></span>
</blockquote>
<div>
<br>
</div>