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