[libvirt] [PATCH RFC V2] RFC: Reimplement cache allocation for a VM
Martin Kletzander
mkletzan at redhat.com
Fri Apr 28 12:16:17 UTC 2017
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...
>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
>---
> include/libvirt/virterror.h | 1 +
> src/Makefile.am | 1 +
> src/libvirt_private.syms | 6 +
> src/util/virerror.c | 1 +
> src/util/virresctrl.c | 423 ++++++++++++++++++++++++++++++
> src/util/virresctrl.h | 86 ++++++
> tests/Makefile.am | 7 +-
> tests/virresctrldata/L3-free.schemata | 1 +
> tests/virresctrldata/L3CODE-free.schemata | 1 +
> tests/virresctrldata/L3DATA-free.schemata | 1 +
> tests/virresctrltest.c | 117 +++++++++
> 11 files changed, 644 insertions(+), 1 deletion(-)
> create mode 100644 src/util/virresctrl.c
> create mode 100644 src/util/virresctrl.h
> create mode 100644 tests/virresctrldata/L3-free.schemata
> create mode 100644 tests/virresctrldata/L3CODE-free.schemata
> create mode 100644 tests/virresctrldata/L3DATA-free.schemata
> create mode 100644 tests/virresctrltest.c
>
>diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
>index 2efee8f..4bc0c74 100644
>--- a/include/libvirt/virterror.h
>+++ b/include/libvirt/virterror.h
>@@ -132,6 +132,7 @@ typedef enum {
>
> VIR_FROM_PERF = 65, /* Error from perf */
> VIR_FROM_LIBSSH = 66, /* Error from libssh connection transport */
>+ VIR_FROM_RESCTRL = 67, /* Error from resctrl */
>
> # ifdef VIR_ENUM_SENTINELS
> VIR_ERR_DOMAIN_LAST
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 60eba37..0ae2af5 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -165,6 +165,7 @@ UTIL_SOURCES = \
> util/virprocess.c util/virprocess.h \
> util/virqemu.c util/virqemu.h \
> util/virrandom.h util/virrandom.c \
>+ util/virresctrl.h util/virresctrl.c \
> util/virrotatingfile.h util/virrotatingfile.c \
> util/virscsi.c util/virscsi.h \
> util/virscsihost.c util/virscsihost.h \
>diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>index 9d7760d..b7225fe 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -2396,6 +2396,12 @@ virRandomGenerateWWN;
> virRandomInt;
>
>
>+# util/virresctrl.h
>+virResctrlFreeSchemata;
>+virResctrlGetFreeCache;
>+virResctrlTypeToString;
>+
>+
> # util/virrotatingfile.h
> virRotatingFileReaderConsume;
> virRotatingFileReaderFree;
>diff --git a/src/util/virerror.c b/src/util/virerror.c
>index ef17fb5..02fabcc 100644
>--- a/src/util/virerror.c
>+++ b/src/util/virerror.c
>@@ -139,6 +139,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
>
> "Perf", /* 65 */
> "Libssh transport layer",
>+ "Resource Control",
> )
>
>
>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>new file mode 100644
>index 0000000..778c2ec
>--- /dev/null
>+++ b/src/util/virresctrl.c
>@@ -0,0 +1,423 @@
>+/*
>+ * virresctrl.c: methods for managing resource 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>
>+ */
>+
>+#include <config.h>
>+#include <fcntl.h>
>+#include <sys/file.h>
>+#include <sys/stat.h>
>+#include <sys/types.h>
>+
>+#include "virresctrl.h"
>+#include "virerror.h"
>+#include "virlog.h"
>+#include "viralloc.h"
>+#include "virstring.h"
>+#include "virfile.h"
>+
>+VIR_LOG_INIT("util.resctrl");
>+
>+#define VIR_FROM_THIS VIR_FROM_RESCTRL
>+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
>+
>+VIR_ENUM_IMPL(virResctrl, VIR_RESCTRL_TYPE_LAST,
>+ "L3",
>+ "L3CODE",
>+ "L3DATA",
>+ "L2")
Is there CAT for L2 currently?
>+
>+/**
>+ * 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.
>+ 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.
>+ size_t n_schematas; /* number of schemata the resource group contains,
>+ eg: 2 */
again, no need for
>+ virResctrlSchemataPtr *schematas; /* scheamta list */
'schemata' is plural, there is no such thing as 'schematas'.
>+};
>+
>+/* 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?
>+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.
>+}
>+
>+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.
>+
>+ 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.
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.
>+
>+ 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.
/That also reminds me I haven't seen Inception in a while, where's my
spinning top, BTW/
>+ 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.
>+ 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.
>+ 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...
>+ 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.
>+ 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?
>+
>+ 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?
>+ 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?
>+ 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.
>+ 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?
>+ 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.
>+ }
>+ }
>+ }
>+}
>+
>+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>
>+ */
>+
>+#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
>+ */
>+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.
>+ 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.
>+};
>+
>+/*
>+ * 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)
>+void virResctrlFreeSchemata(virResctrlSchemataPtr ptr);
>+#endif
>diff --git a/tests/Makefile.am b/tests/Makefile.am
>index 3cc828d..0e09e43 100644
>--- a/tests/Makefile.am
>+++ b/tests/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 \
> virnetdevbandwidthmock.la \
> 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?
> 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>
>+ */
>+
>+#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?
>+{
>+ size_t i;
Empty line after variable declarations makes it nicer to read. But
that's just my weird thing.
>+ 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.
>+ 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?
>+ return ret;
>+}
>+
>+VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virnumamock.so")
What is the need for this?
>--
>1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170428/545a23a1/attachment-0001.sig>
More information about the libvir-list
mailing list