[v10] Support cache tune in libvirt

[libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 4 months ago

This patch adds some utils struct and functions to expose resctrl
information.

virResCtrlAvailable: if resctrl interface exist on host.
virResCtrlGet: get specific type resource control information.
virResCtrlInit: initialize resctrl struct from the host's sys fs.
resctrlall[]: an array to maintain resource control information.

Some of host cpu related information methods was added in virhostcpu.c

Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
---
 include/libvirt/virterror.h |   1 +
 po/POTFILES.in              |   1 +
 src/Makefile.am             |   1 +
 src/libvirt_private.syms    |   4 +
 src/util/virerror.c         |   1 +
 src/util/virhostcpu.c       | 186 ++++++++++++++++++++++++++++++++++++----
 src/util/virhostcpu.h       |   6 ++
 src/util/virresctrl.c       | 201 ++++++++++++++++++++++++++++++++++++++++++++
 src/util/virresctrl.h       |  78 +++++++++++++++++
 9 files changed, 462 insertions(+), 17 deletions(-)
 create mode 100644 src/util/virresctrl.c
 create mode 100644 src/util/virresctrl.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 2efee8f..3dd2d08 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 resource control */
 
 # ifdef VIR_ENUM_SENTINELS
     VIR_ERR_DOMAIN_LAST
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 7c7f530..4147bc6 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -241,6 +241,7 @@ src/util/virportallocator.c
 src/util/virprocess.c
 src/util/virqemu.c
 src/util/virrandom.c
+src/util/virresctrl.c
 src/util/virrotatingfile.c
 src/util/virscsi.c
 src/util/virscsihost.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 7d42eac..edb946a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -162,6 +162,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 aed1d3d..bb7c3ad 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2320,6 +2320,10 @@ virRandomGenerateWWN;
 virRandomInt;
 
 
+# util/virresctrl.h
+virResCtrlAvailable;
+virResCtrlInit;
+
 # util/virrotatingfile.h
 virRotatingFileReaderConsume;
 virRotatingFileReaderFree;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index ef17fb5..0ba15e6 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",
+              "Resouce Control",
     )
 
 
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index f29f312..e6d5102 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -206,29 +206,21 @@ void virHostCPUSetSysFSSystemPathLinux(const char *path)
         sysfs_system_path = SYSFS_SYSTEM_PATH;
 }
 
-/* Return the positive decimal contents of the given
- * DIR/cpu%u/FILE, or -1 on error.  If DEFAULT_VALUE is non-negative
- * and the file could not be found, return that instead of an error;
- * this is useful for machines that cannot hot-unplug cpu0, or where
- * hot-unplugging is disabled, or where the kernel is too old
- * to support NUMA cells, etc.  */
+/* Get a String value*/
 static int
-virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
-                   int default_value)
+virHostCPUGetStrValue(const char *dir, unsigned int cpu, const char *file, char *value_str)
 {
     char *path;
     FILE *pathfp;
-    int value = -1;
-    char value_str[INT_BUFSIZE_BOUND(value)];
-    char *tmp;
+    int ret = -1;
 
     if (virAsprintf(&path, "%s/cpu%u/%s", dir, cpu, file) < 0)
         return -1;
 
     pathfp = fopen(path, "r");
     if (pathfp == NULL) {
-        if (default_value >= 0 && errno == ENOENT)
-            value = default_value;
+        if (errno == ENOENT)
+            return -2;
         else
             virReportSystemError(errno, _("cannot open %s"), path);
         goto cleanup;
@@ -238,17 +230,84 @@ virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
         virReportSystemError(errno, _("cannot read from %s"), path);
         goto cleanup;
     }
+
+    ret = 0;
+
+ cleanup:
+    VIR_FORCE_FCLOSE(pathfp);
+    VIR_FREE(path);
+    return ret;
+}
+
+
+/* Return the positive decimal contents of the given
+ * DIR/cpu%u/FILE, or -1 on error.  If DEFAULT_VALUE is non-negative
+ * and the file could not be found, return that instead of an error;
+ * this is useful for machines that cannot hot-unplug cpu0, or where
+ * hot-unplugging is disabled, or where the kernel is too old
+ * to support NUMA cells, etc.  */
+static int
+virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
+                   int default_value)
+{
+    int value = -1;
+    char value_str[INT_BUFSIZE_BOUND(value)];
+    char *tmp;
+    int ret;
+
+    if ((ret = (virHostCPUGetStrValue(dir, cpu, file, value_str))) < 0) {
+        if (ret == -2 && default_value >= 0)
+            return default_value;
+        else
+            return -1;
+    }
+
     if (virStrToLong_i(value_str, &tmp, 10, &value) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("could not convert '%s' to an integer"),
                        value_str);
-        goto cleanup;
+        return -1;
     }
+    return value;
+}
 
- cleanup:
-    VIR_FORCE_FCLOSE(pathfp);
-    VIR_FREE(path);
+/* Return specific type cache size in KiB of given cpu
+   -1 on error happened */
+static
+int virHostCPUGetCache(unsigned int cpu, unsigned int type)
+{
+    char *cachedir = NULL;
+    char *cpudir;
+    char *unit = NULL;
+    char *tmp;
+    int value = -1;
+    unsigned long long size;
+    char value_str[INT_BUFSIZE_BOUND(value)];
 
+    if (virAsprintf(&cpudir, "%s/cpu", sysfs_system_path) < 0)
+        return -1;
+
+    if (virAsprintf(&cachedir, "cache/index%u/size", type) < 0)
+        goto error;
+
+    if (virHostCPUGetStrValue(cpudir, cpu, cachedir, value_str) < 0)
+        goto error;
+
+    if ((tmp = strchr(value_str, '\n'))) *tmp = '\0';
+
+    if (virStrToLong_i(value_str, &unit, 10, &value) < 0)
+        goto error;
+
+    size = value;
+
+    if (virScaleInteger(&size, unit, 1, ULLONG_MAX) < 0)
+        goto error;
+
+    return size / 1024;
+
+ error:
+    VIR_FREE(cpudir);
+    VIR_FREE(cachedir);
     return value;
 }
 
@@ -301,6 +360,23 @@ virHostCPUParseSocket(const char *dir,
     return ret;
 }
 
+/* return socket id of a given cpu*/
+static
+int virHostCPUGetSocketId(virArch hostarch, unsigned int cpu)
+{
+    char *cpu_dir;
+    int ret = -1;
+
+    if (virAsprintf(&cpu_dir, "%s/cpu", sysfs_system_path) < 0)
+        goto cleanup;
+
+    ret = virHostCPUParseSocket(cpu_dir, hostarch, cpu);
+
+ cleanup:
+    VIR_FREE(cpu_dir);
+    return ret;
+}
+
 /* parses a node entry, returning number of processors in the node and
  * filling arguments */
 static int
@@ -1346,3 +1422,79 @@ virHostCPUGetKVMMaxVCPUs(void)
     return -1;
 }
 #endif /* HAVE_LINUX_KVM_H */
+
+/* Fill all cache bank informations
+ * Return a list of virResCacheBankPtr, and fill cache bank information
+ * by loop for all cpus on host, number of cache bank will be set in nbanks
+ *
+ * NULL if error happened, and nbanks will be set 0. */
+virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch, int type, size_t *nbanks, int cbm_len)
+{
+    int npresent_cpus;
+    int idx;
+    size_t i;
+    virResCacheBankPtr bank;
+
+    *nbanks = 0;
+    if ((npresent_cpus = virHostCPUGetCount()) < 0)
+        return NULL;
+
+    switch (type) {
+        case VIR_RDT_RESOURCE_L3:
+        case VIR_RDT_RESOURCE_L3DATA:
+        case VIR_RDT_RESOURCE_L3CODE:
+            idx = 3;
+            break;
+        case VIR_RDT_RESOURCE_L2:
+            idx = 2;
+            break;
+        default:
+            idx = -1;
+    }
+
+    if (idx == -1)
+        return NULL;
+
+    if (VIR_ALLOC_N(bank, 1) < 0)
+        return NULL;
+
+    *nbanks = 1;
+
+    for (i = 0; i < npresent_cpus; i ++) {
+        int s_id;
+        int cache_size;
+
+        if ((s_id = virHostCPUGetSocketId(arch, i)) < 0)
+            goto error;
+
+        /* Expand cache bank array */
+        if (s_id > (*nbanks - 1)) {
+            size_t cur = *nbanks;
+            size_t exp = s_id - (*nbanks) + 1;
+            if (VIR_EXPAND_N(bank, cur, exp) < 0)
+                goto error;
+            *nbanks = s_id + 1;
+        }
+
+        if (bank[s_id].cpu_mask == NULL) {
+            if (!(bank[s_id].cpu_mask = virBitmapNew(npresent_cpus)))
+                goto error;
+        }
+
+        ignore_value(virBitmapSetBit(bank[s_id].cpu_mask, i));
+
+        if (bank[s_id].cache_size == 0) {
+            if ((cache_size = virHostCPUGetCache(i, idx)) < 0)
+                goto error;
+
+            bank[s_id].cache_size = cache_size;
+            bank[s_id].cache_min = cache_size / cbm_len;
+        }
+    }
+    return bank;
+
+ error:
+    *nbanks = 0;
+    VIR_FREE(bank);
+    return NULL;
+}
diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index 39f7cf8..27f208e 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -27,6 +27,7 @@
 # include "internal.h"
 # include "virarch.h"
 # include "virbitmap.h"
+# include "virresctrl.h"
 
 # define VIR_HOST_CPU_MASK_LEN 1024
 
@@ -58,4 +59,9 @@ int virHostCPUStatsAssign(virNodeCPUStatsPtr param,
                           const char *name,
                           unsigned long long value);
 
+virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch,
+                                           int type,
+                                           size_t *nbanks,
+                                           int cbm_len);
+
 #endif /* __VIR_HOSTCPU_H__*/
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
new file mode 100644
index 0000000..44a47cc
--- /dev/null
+++ b/src/util/virresctrl.c
@@ -0,0 +1,201 @@
+/*
+ * virresctrl.c: methods for managing resource control
+ *
+ * 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@intel.com>
+ */
+#include <config.h>
+
+#include "virresctrl.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virhostcpu.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virarch.h"
+
+VIR_LOG_INIT("util.resctrl");
+
+#define VIR_FROM_THIS VIR_FROM_RESCTRL
+
+#define RESCTRL_DIR "/sys/fs/resctrl"
+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
+#define MAX_CPU_SOCKET_NUM 8
+#define MAX_CBM_BIT_LEN 32
+#define MAX_SCHEMATA_LEN 1024
+#define MAX_FILE_LEN (10 * 1024 * 1024)
+
+static unsigned int host_id;
+
+static virResCtrl resctrlall[] = {
+    {
+        .name = "L3",
+        .cache_level = "l3",
+    },
+    {
+        .name = "L3DATA",
+        .cache_level = "l3",
+    },
+    {
+        .name = "L3CODE",
+        .cache_level = "l3",
+    },
+    {
+        .name = "L2",
+        .cache_level = "l2",
+    },
+};
+
+static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
+{
+    int ret = 0;
+    char *tmp;
+    char *path;
+
+    if (virAsprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, resctrlall[type].name, item) < 0)
+        return -1;
+    if (virFileReadAll(path, 10, str) < 0) {
+        ret = -1;
+        goto cleanup;
+    }
+
+    if ((tmp = strchr(*str, '\n'))) *tmp = '\0';
+
+ cleanup:
+    VIR_FREE(path);
+    return ret;
+}
+
+static int virResCtrlReadConfig(virArch arch, int type)
+{
+    int ret;
+    size_t i, nbanks;
+    char *str;
+
+    /* Read num_closids from resctrl.
+       eg: /sys/fs/resctrl/info/L3/num_closids */
+    if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)
+        goto error;
+
+    if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid)) < 0)
+        goto error;
+
+    VIR_FREE(str);
+
+    /* Read min_cbm_bits from resctrl.
+       eg: /sys/fs/resctrl/info/L3/cbm_mask */
+    if ((ret = virResCtrlGetInfoStr(type, "min_cbm_bits", &str)) < 0)
+        goto error;
+
+    if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_bits)) < 0)
+        goto error;
+
+    VIR_FREE(str);
+
+    /* Read cbm_mask string from resctrl.
+       eg: /sys/fs/resctrl/info/L3/cbm_mask */
+    if ((ret = virResCtrlGetInfoStr(type, "cbm_mask", &str)) < 0)
+        goto error;
+
+    /* cbm_mask is in hex, eg: "fffff", calculate cbm length from the default
+       cbm_mask. */
+    resctrlall[type].cbm_len = strlen(str) * 4;
+
+    /* Get all cache bank informations */
+    resctrlall[type].cache_banks = virHostCPUGetCacheBanks(arch,
+                                                           type,
+                                                           &nbanks, resctrlall[type].cbm_len);
+
+    if (resctrlall[type].cache_banks == NULL)
+        goto error;
+
+    resctrlall[type].num_banks = nbanks;
+
+    for (i = 0; i < resctrlall[type].num_banks; i++) {
+        /* L3CODE and L3DATA shares same L3 resource, so they should
+         * have same host_id. */
+        if (type == VIR_RDT_RESOURCE_L3CODE)
+            resctrlall[type].cache_banks[i].host_id = resctrlall[VIR_RDT_RESOURCE_L3DATA].cache_banks[i].host_id;
+        else
+            resctrlall[type].cache_banks[i].host_id = host_id++;
+    }
+
+    resctrlall[type].enabled = true;
+
+    ret = 0;
+
+ error:
+    VIR_FREE(str);
+    return ret;
+}
+
+int
+virResCtrlInit(void)
+{
+    size_t i = 0;
+    char *tmp;
+    int rc = 0;
+
+    virArch hostarch;
+
+    hostarch = virArchFromHost();
+
+    for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
+        if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Failed to initialize resource control config"));
+            goto cleanup;
+        }
+
+        if (virFileExists(tmp)) {
+            if ((rc = virResCtrlReadConfig(hostarch, i)) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("Failed to get resource control config"));
+                goto cleanup;
+            }
+        }
+        VIR_FREE(tmp);
+    }
+
+ cleanup:
+    VIR_FREE(tmp);
+    return rc;
+}
+
+/*
+ * Test whether the host support resource control
+ */
+bool
+virResCtrlAvailable(void)
+{
+    if (!virFileExists(RESCTRL_INFO_DIR))
+        return false;
+    return true;
+}
+
+/*
+ * Return an virResCtrlPtr point to virResCtrl object,
+ * We should not modify it out side of virresctrl.c
+ */
+virResCtrlPtr
+virResCtrlGet(int type)
+{
+    return &resctrlall[type];
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
new file mode 100644
index 0000000..5a6a344
--- /dev/null
+++ b/src/util/virresctrl.h
@@ -0,0 +1,78 @@
+/*
+ * virresctrl.h: header for managing resctrl control
+ *
+ * Copyright (C) 2016 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@intel.com>
+ */
+
+#ifndef __VIR_RESCTRL_H__
+# define __VIR_RESCTRL_H__
+
+# include "virbitmap.h"
+
+enum {
+    VIR_RDT_RESOURCE_L3,
+    VIR_RDT_RESOURCE_L3DATA,
+    VIR_RDT_RESOURCE_L3CODE,
+    VIR_RDT_RESOURCE_L2,
+    /* Must be the last */
+    VIR_RDT_RESOURCE_LAST,
+};
+
+
+typedef struct _virResCacheBank virResCacheBank;
+typedef virResCacheBank *virResCacheBankPtr;
+struct _virResCacheBank {
+    unsigned int host_id;
+    unsigned long long cache_size;
+    unsigned long long cache_left;
+    unsigned long long cache_min;
+    virBitmapPtr cpu_mask;
+};
+
+/**
+ * struct rdt_resource - attributes of an RDT resource
+ * @enabled:                    Is this feature enabled on this machine
+ * @name:                       Name to use in "schemata" file
+ * @num_closid:                 Number of CLOSIDs available
+ * @max_cbm:                    Largest Cache Bit Mask allowed
+ * @min_cbm_bits:               Minimum number of consecutive bits to be set
+ *                              in a cache bit mask
+ * @cache_level:                Which cache level defines scope of this domain
+ * @num_banks:                  Number of cache bank on this machine.
+ * @cache_banks:                Array of cache bank
+ */
+typedef struct _virResCtrl virResCtrl;
+typedef virResCtrl *virResCtrlPtr;
+struct _virResCtrl {
+        bool                    enabled;
+        const char              *name;
+        int                     num_closid;
+        int                     cbm_len;
+        int                     min_cbm_bits;
+        const char*             cache_level;
+        int                     num_banks;
+        virResCacheBankPtr      cache_banks;
+};
+
+bool virResCtrlAvailable(void);
+int virResCtrlInit(void);
+virResCtrlPtr virResCtrlGet(int);
+
+#endif
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
>This patch adds some utils struct and functions to expose resctrl
>information.
>
>virResCtrlAvailable: if resctrl interface exist on host.
>virResCtrlGet: get specific type resource control information.
>virResCtrlInit: initialize resctrl struct from the host's sys fs.
>resctrlall[]: an array to maintain resource control information.
>
>Some of host cpu related information methods was added in virhostcpu.c
>
>Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
>---
> include/libvirt/virterror.h |   1 +
> po/POTFILES.in              |   1 +
> src/Makefile.am             |   1 +
> src/libvirt_private.syms    |   4 +
> src/util/virerror.c         |   1 +
> src/util/virhostcpu.c       | 186 ++++++++++++++++++++++++++++++++++++----
> src/util/virhostcpu.h       |   6 ++
> src/util/virresctrl.c       | 201 ++++++++++++++++++++++++++++++++++++++++++++
> src/util/virresctrl.h       |  78 +++++++++++++++++
> 9 files changed, 462 insertions(+), 17 deletions(-)
> create mode 100644 src/util/virresctrl.c
> create mode 100644 src/util/virresctrl.h
>
>diff --git a/src/util/virerror.c b/src/util/virerror.c
>index ef17fb5..0ba15e6 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",
>+              "Resouce Control",

s/resouce/resource/

>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>new file mode 100644
>index 0000000..44a47cc
>--- /dev/null
>+++ b/src/util/virresctrl.c
>@@ -0,0 +1,201 @@

[...]

>+
>+static unsigned int host_id;
>+
>+static virResCtrl resctrlall[] = {
>+    {
>+        .name = "L3",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L3DATA",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L3CODE",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L2",
>+        .cache_level = "l2",
>+    },
>+};
>+

You are using global variables, still.  But I *still* see no locking.
What if yet another driver (not just QEMU) will want to use resctrl?
Bunch of these accesses can happen at the same time and break
everything.  How much of this information do we really need to keep (and
not reload)?

For example host_id can screw up a lot of things.  I might be discussing
in the latter patches as well.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Wednesday, 15 March 2017 at 8:20 PM, Martin Kletzander wrote:

> On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > This patch adds some utils struct and functions to expose resctrl
> > information.
> >  
> > virResCtrlAvailable: if resctrl interface exist on host.
> > virResCtrlGet: get specific type resource control information.
> > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > resctrlall[]: an array to maintain resource control information.
> >  
> > Some of host cpu related information methods was added in virhostcpu.c
> >  
> > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@intel.com)>
> > ---
> > include/libvirt/virterror.h | 1 +
> > po/POTFILES.in (http://POTFILES.in) | 1 +
> > src/Makefile.am (http://Makefile.am) | 1 +
> > src/libvirt_private.syms | 4 +
> > src/util/virerror.c | 1 +
> > src/util/virhostcpu.c | 186 ++++++++++++++++++++++++++++++++++++----
> > src/util/virhostcpu.h | 6 ++
> > src/util/virresctrl.c | 201 ++++++++++++++++++++++++++++++++++++++++++++
> > src/util/virresctrl.h | 78 +++++++++++++++++
> > 9 files changed, 462 insertions(+), 17 deletions(-)
> > create mode 100644 src/util/virresctrl.c
> > create mode 100644 src/util/virresctrl.h
> >  
> > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > index ef17fb5..0ba15e6 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",
> > + "Resouce Control",
> >  
>  
>  
> s/resouce/resource/
>  
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > new file mode 100644
> > index 0000000..44a47cc
> > --- /dev/null
> > +++ b/src/util/virresctrl.c
> > @@ -0,0 +1,201 @@
> >  
>  
>  
> [...]
>  
> > +
> > +static unsigned int host_id;
> > +
> > +static virResCtrl resctrlall[] = {
> > + {
> > + .name = "L3",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3DATA",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3CODE",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L2",
> > + .cache_level = "l2",
> > + },
> > +};
> > +
> >  
>  
>  
> You are using global variables, still. But I *still* see no locking.
> What if yet another driver (not just QEMU) will want to use resctrl?
> Bunch of these accesses can happen at the same time and break
> everything. How much of this information do we really need to keep (and
> not reload)?
>  
Yes, we need to maintain a global one as /sys/fs/resctrl is a global one.

most of these information are in-mutble and don’t need to reload.
  
>  
> For example host_id can screw up a lot of things. I might be discussing
> in the latter patches as well.
>  
>  

yep. I see them.  
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
>This patch adds some utils struct and functions to expose resctrl
>information.
>
>virResCtrlAvailable: if resctrl interface exist on host.
>virResCtrlGet: get specific type resource control information.
>virResCtrlInit: initialize resctrl struct from the host's sys fs.
>resctrlall[]: an array to maintain resource control information.
>
>Some of host cpu related information methods was added in virhostcpu.c
>

So to be able to test all this we need to make a bit different approach.
I'm not in favour of pushing this without proper tests.  Some paths need
to be configurable, some readings should be unified.  Unfortunately lot
of the code is just copy-paste mess from the past.  Fortunately for you,
I'm working on cleaning this up, at least a little bit, so that we can
add the tests easily.  I got almost up to the test (I stumbled upon few
rabbit holes on the way and some clean-ups went wrong along the way), so
it should be pretty easy for you to modify this code to use what I'll be
proposing to add.  It's not ready yet, but you can start rebasing your
series on top of my branch pre-cat from my github repo [1].  The commits
are not very well described right now (for some temporary ones I used
whatthecommit.com, sorry), but I'll fix all that.  I'll be updating the
branch, but it will be done with force pushes, so be careful when
rebasing on top of newer versions.

I can do that if you don't want, just let me know so we can coordinate.

Just a quick heads-up, there will be virsysfs that will be used for the
reads, some additional helper functions in virhostcpu and virfile, test
that scans copy of /sys/devices/system (with that path faked thanks to
using the aforementioned virsysfs) and outputs capabilities so that we
can check the capability XML and so on.

Martin

[1] https://github.com/nertpinx/libvirt

>Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
>---
> include/libvirt/virterror.h |   1 +
> po/POTFILES.in              |   1 +
> src/Makefile.am             |   1 +
> src/libvirt_private.syms    |   4 +
> src/util/virerror.c         |   1 +
> src/util/virhostcpu.c       | 186 ++++++++++++++++++++++++++++++++++++----
> src/util/virhostcpu.h       |   6 ++
> src/util/virresctrl.c       | 201 ++++++++++++++++++++++++++++++++++++++++++++
> src/util/virresctrl.h       |  78 +++++++++++++++++
> 9 files changed, 462 insertions(+), 17 deletions(-)
> create mode 100644 src/util/virresctrl.c
> create mode 100644 src/util/virresctrl.h
>
>diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
>index 2efee8f..3dd2d08 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 resource control */
>
> # ifdef VIR_ENUM_SENTINELS
>     VIR_ERR_DOMAIN_LAST
>diff --git a/po/POTFILES.in b/po/POTFILES.in
>index 7c7f530..4147bc6 100644
>--- a/po/POTFILES.in
>+++ b/po/POTFILES.in
>@@ -241,6 +241,7 @@ src/util/virportallocator.c
> src/util/virprocess.c
> src/util/virqemu.c
> src/util/virrandom.c
>+src/util/virresctrl.c
> src/util/virrotatingfile.c
> src/util/virscsi.c
> src/util/virscsihost.c
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 7d42eac..edb946a 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -162,6 +162,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 aed1d3d..bb7c3ad 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -2320,6 +2320,10 @@ virRandomGenerateWWN;
> virRandomInt;
>
>
>+# util/virresctrl.h
>+virResCtrlAvailable;
>+virResCtrlInit;
>+
> # util/virrotatingfile.h
> virRotatingFileReaderConsume;
> virRotatingFileReaderFree;
>diff --git a/src/util/virerror.c b/src/util/virerror.c
>index ef17fb5..0ba15e6 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",
>+              "Resouce Control",
>     )
>
>
>diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
>index f29f312..e6d5102 100644
>--- a/src/util/virhostcpu.c
>+++ b/src/util/virhostcpu.c
>@@ -206,29 +206,21 @@ void virHostCPUSetSysFSSystemPathLinux(const char *path)
>         sysfs_system_path = SYSFS_SYSTEM_PATH;
> }
>
>-/* Return the positive decimal contents of the given
>- * DIR/cpu%u/FILE, or -1 on error.  If DEFAULT_VALUE is non-negative
>- * and the file could not be found, return that instead of an error;
>- * this is useful for machines that cannot hot-unplug cpu0, or where
>- * hot-unplugging is disabled, or where the kernel is too old
>- * to support NUMA cells, etc.  */
>+/* Get a String value*/
> static int
>-virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
>-                   int default_value)
>+virHostCPUGetStrValue(const char *dir, unsigned int cpu, const char *file, char *value_str)
> {
>     char *path;
>     FILE *pathfp;
>-    int value = -1;
>-    char value_str[INT_BUFSIZE_BOUND(value)];
>-    char *tmp;
>+    int ret = -1;
>
>     if (virAsprintf(&path, "%s/cpu%u/%s", dir, cpu, file) < 0)
>         return -1;
>
>     pathfp = fopen(path, "r");
>     if (pathfp == NULL) {
>-        if (default_value >= 0 && errno == ENOENT)
>-            value = default_value;
>+        if (errno == ENOENT)
>+            return -2;
>         else
>             virReportSystemError(errno, _("cannot open %s"), path);
>         goto cleanup;
>@@ -238,17 +230,84 @@ virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
>         virReportSystemError(errno, _("cannot read from %s"), path);
>         goto cleanup;
>     }
>+
>+    ret = 0;
>+
>+ cleanup:
>+    VIR_FORCE_FCLOSE(pathfp);
>+    VIR_FREE(path);
>+    return ret;
>+}
>+
>+
>+/* Return the positive decimal contents of the given
>+ * DIR/cpu%u/FILE, or -1 on error.  If DEFAULT_VALUE is non-negative
>+ * and the file could not be found, return that instead of an error;
>+ * this is useful for machines that cannot hot-unplug cpu0, or where
>+ * hot-unplugging is disabled, or where the kernel is too old
>+ * to support NUMA cells, etc.  */
>+static int
>+virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
>+                   int default_value)
>+{
>+    int value = -1;
>+    char value_str[INT_BUFSIZE_BOUND(value)];
>+    char *tmp;
>+    int ret;
>+
>+    if ((ret = (virHostCPUGetStrValue(dir, cpu, file, value_str))) < 0) {
>+        if (ret == -2 && default_value >= 0)
>+            return default_value;
>+        else
>+            return -1;
>+    }
>+
>     if (virStrToLong_i(value_str, &tmp, 10, &value) < 0) {
>         virReportError(VIR_ERR_INTERNAL_ERROR,
>                        _("could not convert '%s' to an integer"),
>                        value_str);
>-        goto cleanup;
>+        return -1;
>     }
>+    return value;
>+}
>
>- cleanup:
>-    VIR_FORCE_FCLOSE(pathfp);
>-    VIR_FREE(path);
>+/* Return specific type cache size in KiB of given cpu
>+   -1 on error happened */
>+static
>+int virHostCPUGetCache(unsigned int cpu, unsigned int type)
>+{
>+    char *cachedir = NULL;
>+    char *cpudir;
>+    char *unit = NULL;
>+    char *tmp;
>+    int value = -1;
>+    unsigned long long size;
>+    char value_str[INT_BUFSIZE_BOUND(value)];
>
>+    if (virAsprintf(&cpudir, "%s/cpu", sysfs_system_path) < 0)
>+        return -1;
>+
>+    if (virAsprintf(&cachedir, "cache/index%u/size", type) < 0)
>+        goto error;
>+
>+    if (virHostCPUGetStrValue(cpudir, cpu, cachedir, value_str) < 0)
>+        goto error;
>+
>+    if ((tmp = strchr(value_str, '\n'))) *tmp = '\0';
>+
>+    if (virStrToLong_i(value_str, &unit, 10, &value) < 0)
>+        goto error;
>+
>+    size = value;
>+
>+    if (virScaleInteger(&size, unit, 1, ULLONG_MAX) < 0)
>+        goto error;
>+
>+    return size / 1024;
>+
>+ error:
>+    VIR_FREE(cpudir);
>+    VIR_FREE(cachedir);
>     return value;
> }
>
>@@ -301,6 +360,23 @@ virHostCPUParseSocket(const char *dir,
>     return ret;
> }
>
>+/* return socket id of a given cpu*/
>+static
>+int virHostCPUGetSocketId(virArch hostarch, unsigned int cpu)
>+{
>+    char *cpu_dir;
>+    int ret = -1;
>+
>+    if (virAsprintf(&cpu_dir, "%s/cpu", sysfs_system_path) < 0)
>+        goto cleanup;
>+
>+    ret = virHostCPUParseSocket(cpu_dir, hostarch, cpu);
>+
>+ cleanup:
>+    VIR_FREE(cpu_dir);
>+    return ret;
>+}
>+
> /* parses a node entry, returning number of processors in the node and
>  * filling arguments */
> static int
>@@ -1346,3 +1422,79 @@ virHostCPUGetKVMMaxVCPUs(void)
>     return -1;
> }
> #endif /* HAVE_LINUX_KVM_H */
>+
>+/* Fill all cache bank informations
>+ * Return a list of virResCacheBankPtr, and fill cache bank information
>+ * by loop for all cpus on host, number of cache bank will be set in nbanks
>+ *
>+ * NULL if error happened, and nbanks will be set 0. */
>+virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch, int type, size_t *nbanks, int cbm_len)
>+{
>+    int npresent_cpus;
>+    int idx;
>+    size_t i;
>+    virResCacheBankPtr bank;
>+
>+    *nbanks = 0;
>+    if ((npresent_cpus = virHostCPUGetCount()) < 0)
>+        return NULL;
>+
>+    switch (type) {
>+        case VIR_RDT_RESOURCE_L3:
>+        case VIR_RDT_RESOURCE_L3DATA:
>+        case VIR_RDT_RESOURCE_L3CODE:
>+            idx = 3;
>+            break;
>+        case VIR_RDT_RESOURCE_L2:
>+            idx = 2;
>+            break;
>+        default:
>+            idx = -1;
>+    }
>+
>+    if (idx == -1)
>+        return NULL;
>+
>+    if (VIR_ALLOC_N(bank, 1) < 0)
>+        return NULL;
>+
>+    *nbanks = 1;
>+
>+    for (i = 0; i < npresent_cpus; i ++) {
>+        int s_id;
>+        int cache_size;
>+
>+        if ((s_id = virHostCPUGetSocketId(arch, i)) < 0)
>+            goto error;
>+
>+        /* Expand cache bank array */
>+        if (s_id > (*nbanks - 1)) {
>+            size_t cur = *nbanks;
>+            size_t exp = s_id - (*nbanks) + 1;
>+            if (VIR_EXPAND_N(bank, cur, exp) < 0)
>+                goto error;
>+            *nbanks = s_id + 1;
>+        }
>+
>+        if (bank[s_id].cpu_mask == NULL) {
>+            if (!(bank[s_id].cpu_mask = virBitmapNew(npresent_cpus)))
>+                goto error;
>+        }
>+
>+        ignore_value(virBitmapSetBit(bank[s_id].cpu_mask, i));
>+
>+        if (bank[s_id].cache_size == 0) {
>+            if ((cache_size = virHostCPUGetCache(i, idx)) < 0)
>+                goto error;
>+
>+            bank[s_id].cache_size = cache_size;
>+            bank[s_id].cache_min = cache_size / cbm_len;
>+        }
>+    }
>+    return bank;
>+
>+ error:
>+    *nbanks = 0;
>+    VIR_FREE(bank);
>+    return NULL;
>+}
>diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
>index 39f7cf8..27f208e 100644
>--- a/src/util/virhostcpu.h
>+++ b/src/util/virhostcpu.h
>@@ -27,6 +27,7 @@
> # include "internal.h"
> # include "virarch.h"
> # include "virbitmap.h"
>+# include "virresctrl.h"
>
> # define VIR_HOST_CPU_MASK_LEN 1024
>
>@@ -58,4 +59,9 @@ int virHostCPUStatsAssign(virNodeCPUStatsPtr param,
>                           const char *name,
>                           unsigned long long value);
>
>+virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch,
>+                                           int type,
>+                                           size_t *nbanks,
>+                                           int cbm_len);
>+
> #endif /* __VIR_HOSTCPU_H__*/
>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>new file mode 100644
>index 0000000..44a47cc
>--- /dev/null
>+++ b/src/util/virresctrl.c
>@@ -0,0 +1,201 @@
>+/*
>+ * virresctrl.c: methods for managing resource control
>+ *
>+ * 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@intel.com>
>+ */
>+#include <config.h>
>+
>+#include "virresctrl.h"
>+#include "viralloc.h"
>+#include "virerror.h"
>+#include "virfile.h"
>+#include "virhostcpu.h"
>+#include "virlog.h"
>+#include "virstring.h"
>+#include "virarch.h"
>+
>+VIR_LOG_INIT("util.resctrl");
>+
>+#define VIR_FROM_THIS VIR_FROM_RESCTRL
>+
>+#define RESCTRL_DIR "/sys/fs/resctrl"
>+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
>+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
>+
>+#define MAX_CPU_SOCKET_NUM 8
>+#define MAX_CBM_BIT_LEN 32
>+#define MAX_SCHEMATA_LEN 1024
>+#define MAX_FILE_LEN (10 * 1024 * 1024)
>+
>+static unsigned int host_id;
>+
>+static virResCtrl resctrlall[] = {
>+    {
>+        .name = "L3",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L3DATA",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L3CODE",
>+        .cache_level = "l3",
>+    },
>+    {
>+        .name = "L2",
>+        .cache_level = "l2",
>+    },
>+};
>+
>+static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
>+{
>+    int ret = 0;
>+    char *tmp;
>+    char *path;
>+
>+    if (virAsprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, resctrlall[type].name, item) < 0)
>+        return -1;
>+    if (virFileReadAll(path, 10, str) < 0) {
>+        ret = -1;
>+        goto cleanup;
>+    }
>+
>+    if ((tmp = strchr(*str, '\n'))) *tmp = '\0';
>+
>+ cleanup:
>+    VIR_FREE(path);
>+    return ret;
>+}
>+
>+static int virResCtrlReadConfig(virArch arch, int type)
>+{
>+    int ret;
>+    size_t i, nbanks;
>+    char *str;
>+
>+    /* Read num_closids from resctrl.
>+       eg: /sys/fs/resctrl/info/L3/num_closids */
>+    if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)
>+        goto error;
>+
>+    if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid)) < 0)
>+        goto error;
>+
>+    VIR_FREE(str);
>+
>+    /* Read min_cbm_bits from resctrl.
>+       eg: /sys/fs/resctrl/info/L3/cbm_mask */
>+    if ((ret = virResCtrlGetInfoStr(type, "min_cbm_bits", &str)) < 0)
>+        goto error;
>+
>+    if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_bits)) < 0)
>+        goto error;
>+
>+    VIR_FREE(str);
>+
>+    /* Read cbm_mask string from resctrl.
>+       eg: /sys/fs/resctrl/info/L3/cbm_mask */
>+    if ((ret = virResCtrlGetInfoStr(type, "cbm_mask", &str)) < 0)
>+        goto error;
>+
>+    /* cbm_mask is in hex, eg: "fffff", calculate cbm length from the default
>+       cbm_mask. */
>+    resctrlall[type].cbm_len = strlen(str) * 4;
>+
>+    /* Get all cache bank informations */
>+    resctrlall[type].cache_banks = virHostCPUGetCacheBanks(arch,
>+                                                           type,
>+                                                           &nbanks, resctrlall[type].cbm_len);
>+
>+    if (resctrlall[type].cache_banks == NULL)
>+        goto error;
>+
>+    resctrlall[type].num_banks = nbanks;
>+
>+    for (i = 0; i < resctrlall[type].num_banks; i++) {
>+        /* L3CODE and L3DATA shares same L3 resource, so they should
>+         * have same host_id. */
>+        if (type == VIR_RDT_RESOURCE_L3CODE)
>+            resctrlall[type].cache_banks[i].host_id = resctrlall[VIR_RDT_RESOURCE_L3DATA].cache_banks[i].host_id;
>+        else
>+            resctrlall[type].cache_banks[i].host_id = host_id++;
>+    }
>+
>+    resctrlall[type].enabled = true;
>+
>+    ret = 0;
>+
>+ error:
>+    VIR_FREE(str);
>+    return ret;
>+}
>+
>+int
>+virResCtrlInit(void)
>+{
>+    size_t i = 0;
>+    char *tmp;
>+    int rc = 0;
>+
>+    virArch hostarch;
>+
>+    hostarch = virArchFromHost();
>+
>+    for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
>+        if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {
>+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                           _("Failed to initialize resource control config"));
>+            goto cleanup;
>+        }
>+
>+        if (virFileExists(tmp)) {
>+            if ((rc = virResCtrlReadConfig(hostarch, i)) < 0) {
>+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                               _("Failed to get resource control config"));
>+                goto cleanup;
>+            }
>+        }
>+        VIR_FREE(tmp);
>+    }
>+
>+ cleanup:
>+    VIR_FREE(tmp);
>+    return rc;
>+}
>+
>+/*
>+ * Test whether the host support resource control
>+ */
>+bool
>+virResCtrlAvailable(void)
>+{
>+    if (!virFileExists(RESCTRL_INFO_DIR))
>+        return false;
>+    return true;
>+}
>+
>+/*
>+ * Return an virResCtrlPtr point to virResCtrl object,
>+ * We should not modify it out side of virresctrl.c
>+ */
>+virResCtrlPtr
>+virResCtrlGet(int type)
>+{
>+    return &resctrlall[type];
>+}
>diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>new file mode 100644
>index 0000000..5a6a344
>--- /dev/null
>+++ b/src/util/virresctrl.h
>@@ -0,0 +1,78 @@
>+/*
>+ * virresctrl.h: header for managing resctrl control
>+ *
>+ * Copyright (C) 2016 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@intel.com>
>+ */
>+
>+#ifndef __VIR_RESCTRL_H__
>+# define __VIR_RESCTRL_H__
>+
>+# include "virbitmap.h"
>+
>+enum {
>+    VIR_RDT_RESOURCE_L3,
>+    VIR_RDT_RESOURCE_L3DATA,
>+    VIR_RDT_RESOURCE_L3CODE,
>+    VIR_RDT_RESOURCE_L2,
>+    /* Must be the last */
>+    VIR_RDT_RESOURCE_LAST,
>+};
>+
>+
>+typedef struct _virResCacheBank virResCacheBank;
>+typedef virResCacheBank *virResCacheBankPtr;
>+struct _virResCacheBank {
>+    unsigned int host_id;
>+    unsigned long long cache_size;
>+    unsigned long long cache_left;
>+    unsigned long long cache_min;
>+    virBitmapPtr cpu_mask;
>+};
>+
>+/**
>+ * struct rdt_resource - attributes of an RDT resource
>+ * @enabled:                    Is this feature enabled on this machine
>+ * @name:                       Name to use in "schemata" file
>+ * @num_closid:                 Number of CLOSIDs available
>+ * @max_cbm:                    Largest Cache Bit Mask allowed
>+ * @min_cbm_bits:               Minimum number of consecutive bits to be set
>+ *                              in a cache bit mask
>+ * @cache_level:                Which cache level defines scope of this domain
>+ * @num_banks:                  Number of cache bank on this machine.
>+ * @cache_banks:                Array of cache bank
>+ */
>+typedef struct _virResCtrl virResCtrl;
>+typedef virResCtrl *virResCtrlPtr;
>+struct _virResCtrl {
>+        bool                    enabled;
>+        const char              *name;
>+        int                     num_closid;
>+        int                     cbm_len;
>+        int                     min_cbm_bits;
>+        const char*             cache_level;
>+        int                     num_banks;
>+        virResCacheBankPtr      cache_banks;
>+};
>+
>+bool virResCtrlAvailable(void);
>+int virResCtrlInit(void);
>+virResCtrlPtr virResCtrlGet(int);
>+
>+#endif
>--
>1.9.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:

> On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > This patch adds some utils struct and functions to expose resctrl
> > information.
> >  
> > virResCtrlAvailable: if resctrl interface exist on host.
> > virResCtrlGet: get specific type resource control information.
> > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > resctrlall[]: an array to maintain resource control information.
> >  
> > Some of host cpu related information methods was added in virhostcpu.c
>  
> So to be able to test all this we need to make a bit different approach.
> I'm not in favour of pushing this without proper tests. Some paths need
> to be configurable, some readings should be unified. Unfortunately lot
> of the code is just copy-paste mess from the past. Fortunately for you,  
>  
>  

> I'm working on cleaning this up, at least a little bit, so that we can
>  
>  

Good news.  
> add the tests easily. I got almost up to the test (I stumbled upon few
> rabbit holes on the way and some clean-ups went wrong along the way), so
> it should be pretty easy for you to modify this code to use what I'll be
> proposing to add. It's not ready yet, but you can start rebasing your
> series on top of my branch pre-cat from my github repo [1]. The commits
> are not very well described right now (for some temporary ones I used
> whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
> branch, but it will be done with force pushes, so be careful when
> rebasing on top of newer versions.
>  
> I can do that if you don't want, just let me know so we can coordinate.
>  
of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(

  
>  
> Just a quick heads-up, there will be virsysfs that will be used for the
> reads, some additional helper functions in virhostcpu and virfile, test
> that scans copy of /sys/devices/system (with that path faked thanks to
> using the aforementioned virsysfs) and outputs capabilities so that we
> can check the capability XML and so on.
>  
>  


Ah, that’s a good news..  
>  
> Martin
>  
> [1] https://github.com/nertpinx/libvirt
>  
> > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@intel.com)>
> > ---
> > include/libvirt/virterror.h | 1 +
> > po/POTFILES.in (http://POTFILES.in) | 1 +
> > src/Makefile.am (http://Makefile.am) | 1 +
> > src/libvirt_private.syms | 4 +
> > src/util/virerror.c | 1 +
> > src/util/virhostcpu.c | 186 ++++++++++++++++++++++++++++++++++++----
> > src/util/virhostcpu.h | 6 ++
> > src/util/virresctrl.c | 201 ++++++++++++++++++++++++++++++++++++++++++++
> > src/util/virresctrl.h | 78 +++++++++++++++++
> > 9 files changed, 462 insertions(+), 17 deletions(-)
> > create mode 100644 src/util/virresctrl.c
> > create mode 100644 src/util/virresctrl.h
> >  
> > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> > index 2efee8f..3dd2d08 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 resource control */
> >  
> > # ifdef VIR_ENUM_SENTINELS
> > VIR_ERR_DOMAIN_LAST
> > diff --git a/po/POTFILES.in (http://POTFILES.in) b/po/POTFILES.in (http://POTFILES.in)
> > index 7c7f530..4147bc6 100644
> > --- a/po/POTFILES.in (http://POTFILES.in)
> > +++ b/po/POTFILES.in (http://POTFILES.in)
> > @@ -241,6 +241,7 @@ src/util/virportallocator.c
> > src/util/virprocess.c
> > src/util/virqemu.c
> > src/util/virrandom.c
> > +src/util/virresctrl.c
> > src/util/virrotatingfile.c
> > src/util/virscsi.c
> > src/util/virscsihost.c
> > diff --git a/src/Makefile.am (http://Makefile.am) b/src/Makefile.am (http://Makefile.am)
> > index 7d42eac..edb946a 100644
> > --- a/src/Makefile.am (http://Makefile.am)
> > +++ b/src/Makefile.am (http://Makefile.am)
> > @@ -162,6 +162,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 aed1d3d..bb7c3ad 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2320,6 +2320,10 @@ virRandomGenerateWWN;
> > virRandomInt;
> >  
> >  
> > +# util/virresctrl.h
> > +virResCtrlAvailable;
> > +virResCtrlInit;
> > +
> > # util/virrotatingfile.h
> > virRotatingFileReaderConsume;
> > virRotatingFileReaderFree;
> > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > index ef17fb5..0ba15e6 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",
> > + "Resouce Control",
> > )
> >  
> >  
> > diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> > index f29f312..e6d5102 100644
> > --- a/src/util/virhostcpu.c
> > +++ b/src/util/virhostcpu.c
> > @@ -206,29 +206,21 @@ void virHostCPUSetSysFSSystemPathLinux(const char *path)
> > sysfs_system_path = SYSFS_SYSTEM_PATH;
> > }
> >  
> > -/* Return the positive decimal contents of the given
> > - * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative
> > - * and the file could not be found, return that instead of an error;
> > - * this is useful for machines that cannot hot-unplug cpu0, or where
> > - * hot-unplugging is disabled, or where the kernel is too old
> > - * to support NUMA cells, etc. */
> > +/* Get a String value*/
> > static int
> > -virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > - int default_value)
> > +virHostCPUGetStrValue(const char *dir, unsigned int cpu, const char *file, char *value_str)
> > {
> > char *path;
> > FILE *pathfp;
> > - int value = -1;
> > - char value_str[INT_BUFSIZE_BOUND(value)];
> > - char *tmp;
> > + int ret = -1;
> >  
> > if (virAsprintf(&path, "%s/cpu%u/%s", dir, cpu, file) < 0)
> > return -1;
> >  
> > pathfp = fopen(path, "r");
> > if (pathfp == NULL) {
> > - if (default_value >= 0 && errno == ENOENT)
> > - value = default_value;
> > + if (errno == ENOENT)
> > + return -2;
> > else
> > virReportSystemError(errno, _("cannot open %s"), path);
> > goto cleanup;
> > @@ -238,17 +230,84 @@ virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > virReportSystemError(errno, _("cannot read from %s"), path);
> > goto cleanup;
> > }
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + VIR_FORCE_FCLOSE(pathfp);
> > + VIR_FREE(path);
> > + return ret;
> > +}
> > +
> > +
> > +/* Return the positive decimal contents of the given
> > + * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative
> > + * and the file could not be found, return that instead of an error;
> > + * this is useful for machines that cannot hot-unplug cpu0, or where
> > + * hot-unplugging is disabled, or where the kernel is too old
> > + * to support NUMA cells, etc. */
> > +static int
> > +virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > + int default_value)
> > +{
> > + int value = -1;
> > + char value_str[INT_BUFSIZE_BOUND(value)];
> > + char *tmp;
> > + int ret;
> > +
> > + if ((ret = (virHostCPUGetStrValue(dir, cpu, file, value_str))) < 0) {
> > + if (ret == -2 && default_value >= 0)
> > + return default_value;
> > + else
> > + return -1;
> > + }
> > +
> > if (virStrToLong_i(value_str, &tmp, 10, &value) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("could not convert '%s' to an integer"),
> > value_str);
> > - goto cleanup;
> > + return -1;
> > }
> > + return value;
> > +}
> >  
> > - cleanup:
> > - VIR_FORCE_FCLOSE(pathfp);
> > - VIR_FREE(path);
> > +/* Return specific type cache size in KiB of given cpu
> > + -1 on error happened */
> > +static
> > +int virHostCPUGetCache(unsigned int cpu, unsigned int type)
> > +{
> > + char *cachedir = NULL;
> > + char *cpudir;
> > + char *unit = NULL;
> > + char *tmp;
> > + int value = -1;
> > + unsigned long long size;
> > + char value_str[INT_BUFSIZE_BOUND(value)];
> >  
> > + if (virAsprintf(&cpudir, "%s/cpu", sysfs_system_path) < 0)
> > + return -1;
> > +
> > + if (virAsprintf(&cachedir, "cache/index%u/size", type) < 0)
> > + goto error;
> > +
> > + if (virHostCPUGetStrValue(cpudir, cpu, cachedir, value_str) < 0)
> > + goto error;
> > +
> > + if ((tmp = strchr(value_str, '\n'))) *tmp = '\0';
> > +
> > + if (virStrToLong_i(value_str, &unit, 10, &value) < 0)
> > + goto error;
> > +
> > + size = value;
> > +
> > + if (virScaleInteger(&size, unit, 1, ULLONG_MAX) < 0)
> > + goto error;
> > +
> > + return size / 1024;
> > +
> > + error:
> > + VIR_FREE(cpudir);
> > + VIR_FREE(cachedir);
> > return value;
> > }
> >  
> > @@ -301,6 +360,23 @@ virHostCPUParseSocket(const char *dir,
> > return ret;
> > }
> >  
> > +/* return socket id of a given cpu*/
> > +static
> > +int virHostCPUGetSocketId(virArch hostarch, unsigned int cpu)
> > +{
> > + char *cpu_dir;
> > + int ret = -1;
> > +
> > + if (virAsprintf(&cpu_dir, "%s/cpu", sysfs_system_path) < 0)
> > + goto cleanup;
> > +
> > + ret = virHostCPUParseSocket(cpu_dir, hostarch, cpu);
> > +
> > + cleanup:
> > + VIR_FREE(cpu_dir);
> > + return ret;
> > +}
> > +
> > /* parses a node entry, returning number of processors in the node and
> > * filling arguments */
> > static int
> > @@ -1346,3 +1422,79 @@ virHostCPUGetKVMMaxVCPUs(void)
> > return -1;
> > }
> > #endif /* HAVE_LINUX_KVM_H */
> > +
> > +/* Fill all cache bank informations
> > + * Return a list of virResCacheBankPtr, and fill cache bank information
> > + * by loop for all cpus on host, number of cache bank will be set in nbanks
> > + *
> > + * NULL if error happened, and nbanks will be set 0. */
> > +virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch, int type, size_t *nbanks, int cbm_len)
> > +{
> > + int npresent_cpus;
> > + int idx;
> > + size_t i;
> > + virResCacheBankPtr bank;
> > +
> > + *nbanks = 0;
> > + if ((npresent_cpus = virHostCPUGetCount()) < 0)
> > + return NULL;
> > +
> > + switch (type) {
> > + case VIR_RDT_RESOURCE_L3:
> > + case VIR_RDT_RESOURCE_L3DATA:
> > + case VIR_RDT_RESOURCE_L3CODE:
> > + idx = 3;
> > + break;
> > + case VIR_RDT_RESOURCE_L2:
> > + idx = 2;
> > + break;
> > + default:
> > + idx = -1;
> > + }
> > +
> > + if (idx == -1)
> > + return NULL;
> > +
> > + if (VIR_ALLOC_N(bank, 1) < 0)
> > + return NULL;
> > +
> > + *nbanks = 1;
> > +
> > + for (i = 0; i < npresent_cpus; i ++) {
> > + int s_id;
> > + int cache_size;
> > +
> > + if ((s_id = virHostCPUGetSocketId(arch, i)) < 0)
> > + goto error;
> > +
> > + /* Expand cache bank array */
> > + if (s_id > (*nbanks - 1)) {
> > + size_t cur = *nbanks;
> > + size_t exp = s_id - (*nbanks) + 1;
> > + if (VIR_EXPAND_N(bank, cur, exp) < 0)
> > + goto error;
> > + *nbanks = s_id + 1;
> > + }
> > +
> > + if (bank[s_id].cpu_mask == NULL) {
> > + if (!(bank[s_id].cpu_mask = virBitmapNew(npresent_cpus)))
> > + goto error;
> > + }
> > +
> > + ignore_value(virBitmapSetBit(bank[s_id].cpu_mask, i));
> > +
> > + if (bank[s_id].cache_size == 0) {
> > + if ((cache_size = virHostCPUGetCache(i, idx)) < 0)
> > + goto error;
> > +
> > + bank[s_id].cache_size = cache_size;
> > + bank[s_id].cache_min = cache_size / cbm_len;
> > + }
> > + }
> > + return bank;
> > +
> > + error:
> > + *nbanks = 0;
> > + VIR_FREE(bank);
> > + return NULL;
> > +}
> > diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
> > index 39f7cf8..27f208e 100644
> > --- a/src/util/virhostcpu.h
> > +++ b/src/util/virhostcpu.h
> > @@ -27,6 +27,7 @@
> > # include "internal.h"
> > # include "virarch.h"
> > # include "virbitmap.h"
> > +# include "virresctrl.h"
> >  
> > # define VIR_HOST_CPU_MASK_LEN 1024
> >  
> > @@ -58,4 +59,9 @@ int virHostCPUStatsAssign(virNodeCPUStatsPtr param,
> > const char *name,
> > unsigned long long value);
> >  
> > +virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch,
> > + int type,
> > + size_t *nbanks,
> > + int cbm_len);
> > +
> > #endif /* __VIR_HOSTCPU_H__*/
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > new file mode 100644
> > index 0000000..44a47cc
> > --- /dev/null
> > +++ b/src/util/virresctrl.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * virresctrl.c: methods for managing resource control
> > + *
> > + * 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@intel.com (mailto:liyong.qiao@intel.com)>
> > + */
> > +#include <config.h>
> > +
> > +#include "virresctrl.h"
> > +#include "viralloc.h"
> > +#include "virerror.h"
> > +#include "virfile.h"
> > +#include "virhostcpu.h"
> > +#include "virlog.h"
> > +#include "virstring.h"
> > +#include "virarch.h"
> > +
> > +VIR_LOG_INIT("util.resctrl");
> > +
> > +#define VIR_FROM_THIS VIR_FROM_RESCTRL
> > +
> > +#define RESCTRL_DIR "/sys/fs/resctrl"
> > +#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
> > +#define SYSFS_SYSTEM_PATH "/sys/devices/system"
> > +
> > +#define MAX_CPU_SOCKET_NUM 8
> > +#define MAX_CBM_BIT_LEN 32
> > +#define MAX_SCHEMATA_LEN 1024
> > +#define MAX_FILE_LEN (10 * 1024 * 1024)
> > +
> > +static unsigned int host_id;
> > +
> > +static virResCtrl resctrlall[] = {
> > + {
> > + .name = "L3",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3DATA",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3CODE",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L2",
> > + .cache_level = "l2",
> > + },
> > +};
> > +
> > +static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
> > +{
> > + int ret = 0;
> > + char *tmp;
> > + char *path;
> > +
> > + if (virAsprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, resctrlall[type].name, item) < 0)
> > + return -1;
> > + if (virFileReadAll(path, 10, str) < 0) {
> > + ret = -1;
> > + goto cleanup;
> > + }
> > +
> > + if ((tmp = strchr(*str, '\n'))) *tmp = '\0';
> > +
> > + cleanup:
> > + VIR_FREE(path);
> > + return ret;
> > +}
> > +
> > +static int virResCtrlReadConfig(virArch arch, int type)
> > +{
> > + int ret;
> > + size_t i, nbanks;
> > + char *str;
> > +
> > + /* Read num_closids from resctrl.
> > + eg: /sys/fs/resctrl/info/L3/num_closids */
> > + if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)
> > + goto error;
> > +
> > + if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid)) < 0)
> > + goto error;
> > +
> > + VIR_FREE(str);
> > +
> > + /* Read min_cbm_bits from resctrl.
> > + eg: /sys/fs/resctrl/info/L3/cbm_mask */
> > + if ((ret = virResCtrlGetInfoStr(type, "min_cbm_bits", &str)) < 0)
> > + goto error;
> > +
> > + if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_bits)) < 0)
> > + goto error;
> > +
> > + VIR_FREE(str);
> > +
> > + /* Read cbm_mask string from resctrl.
> > + eg: /sys/fs/resctrl/info/L3/cbm_mask */
> > + if ((ret = virResCtrlGetInfoStr(type, "cbm_mask", &str)) < 0)
> > + goto error;
> > +
> > + /* cbm_mask is in hex, eg: "fffff", calculate cbm length from the default
> > + cbm_mask. */
> > + resctrlall[type].cbm_len = strlen(str) * 4;
> > +
> > + /* Get all cache bank informations */
> > + resctrlall[type].cache_banks = virHostCPUGetCacheBanks(arch,
> > + type,
> > + &nbanks, resctrlall[type].cbm_len);
> > +
> > + if (resctrlall[type].cache_banks == NULL)
> > + goto error;
> > +
> > + resctrlall[type].num_banks = nbanks;
> > +
> > + for (i = 0; i < resctrlall[type].num_banks; i++) {
> > + /* L3CODE and L3DATA shares same L3 resource, so they should
> > + * have same host_id. */
> > + if (type == VIR_RDT_RESOURCE_L3CODE)
> > + resctrlall[type].cache_banks[i].host_id = resctrlall[VIR_RDT_RESOURCE_L3DATA].cache_banks[i].host_id;
> > + else
> > + resctrlall[type].cache_banks[i].host_id = host_id++;
> > + }
> > +
> > + resctrlall[type].enabled = true;
> > +
> > + ret = 0;
> > +
> > + error:
> > + VIR_FREE(str);
> > + return ret;
> > +}
> > +
> > +int
> > +virResCtrlInit(void)
> > +{
> > + size_t i = 0;
> > + char *tmp;
> > + int rc = 0;
> > +
> > + virArch hostarch;
> > +
> > + hostarch = virArchFromHost();
> > +
> > + for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
> > + if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Failed to initialize resource control config"));
> > + goto cleanup;
> > + }
> > +
> > + if (virFileExists(tmp)) {
> > + if ((rc = virResCtrlReadConfig(hostarch, i)) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Failed to get resource control config"));
> > + goto cleanup;
> > + }
> > + }
> > + VIR_FREE(tmp);
> > + }
> > +
> > + cleanup:
> > + VIR_FREE(tmp);
> > + return rc;
> > +}
> > +
> > +/*
> > + * Test whether the host support resource control
> > + */
> > +bool
> > +virResCtrlAvailable(void)
> > +{
> > + if (!virFileExists(RESCTRL_INFO_DIR))
> > + return false;
> > + return true;
> > +}
> > +
> > +/*
> > + * Return an virResCtrlPtr point to virResCtrl object,
> > + * We should not modify it out side of virresctrl.c
> > + */
> > +virResCtrlPtr
> > +virResCtrlGet(int type)
> > +{
> > + return &resctrlall[type];
> > +}
> > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> > new file mode 100644
> > index 0000000..5a6a344
> > --- /dev/null
> > +++ b/src/util/virresctrl.h
> > @@ -0,0 +1,78 @@
> > +/*
> > + * virresctrl.h: header for managing resctrl control
> > + *
> > + * Copyright (C) 2016 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@intel.com (mailto:liyong.qiao@intel.com)>
> > + */
> > +
> > +#ifndef __VIR_RESCTRL_H__
> > +# define __VIR_RESCTRL_H__
> > +
> > +# include "virbitmap.h"
> > +
> > +enum {
> > + VIR_RDT_RESOURCE_L3,
> > + VIR_RDT_RESOURCE_L3DATA,
> > + VIR_RDT_RESOURCE_L3CODE,
> > + VIR_RDT_RESOURCE_L2,
> > + /* Must be the last */
> > + VIR_RDT_RESOURCE_LAST,
> > +};
> > +
> > +
> > +typedef struct _virResCacheBank virResCacheBank;
> > +typedef virResCacheBank *virResCacheBankPtr;
> > +struct _virResCacheBank {
> > + unsigned int host_id;
> > + unsigned long long cache_size;
> > + unsigned long long cache_left;
> > + unsigned long long cache_min;
> > + virBitmapPtr cpu_mask;
> > +};
> > +
> > +/**
> > + * struct rdt_resource - attributes of an RDT resource
> > + * @enabled: Is this feature enabled on this machine
> > + * @name: Name to use in "schemata" file
> > + * @num_closid: Number of CLOSIDs available
> > + * @max_cbm: Largest Cache Bit Mask allowed
> > + * @min_cbm_bits: Minimum number of consecutive bits to be set
> > + * in a cache bit mask
> > + * @cache_level: Which cache level defines scope of this domain
> > + * @num_banks: Number of cache bank on this machine.
> > + * @cache_banks: Array of cache bank
> > + */
> > +typedef struct _virResCtrl virResCtrl;
> > +typedef virResCtrl *virResCtrlPtr;
> > +struct _virResCtrl {
> > + bool enabled;
> > + const char *name;
> > + int num_closid;
> > + int cbm_len;
> > + int min_cbm_bits;
> > + const char* cache_level;
> > + int num_banks;
> > + virResCacheBankPtr cache_banks;
> > +};
> > +
> > +bool virResCtrlAvailable(void);
> > +int virResCtrlInit(void);
> > +virResCtrlPtr virResCtrlGet(int);
> > +
> > +#endif
> > --
> > 1.9.1
> >  
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >  
>  
>  
>  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:

>  
>  
> --  
> Best regards  
> Eli
>  
> 天涯无处不重逢
> a leaf duckweed belongs to the sea, where not to meet in life  
>  
> Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>  
>  
> On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
>  
> > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > > This patch adds some utils struct and functions to expose resctrl
> > > information.
> > >  
> > > virResCtrlAvailable: if resctrl interface exist on host.
> > > virResCtrlGet: get specific type resource control information.
> > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > resctrlall[]: an array to maintain resource control information.
> > >  
> > > Some of host cpu related information methods was added in virhostcpu.c
> >  
> > So to be able to test all this we need to make a bit different approach.
> > I'm not in favour of pushing this without proper tests. Some paths need
> > to be configurable, some readings should be unified. Unfortunately lot
> > of the code is just copy-paste mess from the past. Fortunately for you,  
> >  
> >  
> >  
> > I'm working on cleaning this up, at least a little bit, so that we can
> >  
> >  
> >  
>  
> Good news.  
> > add the tests easily. I got almost up to the test (I stumbled upon few
> > rabbit holes on the way and some clean-ups went wrong along the way), so
> > it should be pretty easy for you to modify this code to use what I'll be
> > proposing to add. It's not ready yet, but you can start rebasing your
> > series on top of my branch pre-cat from my github repo [1]. The commits
> > are not very well described right now (for some temporary ones I used
> > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
> > branch, but it will be done with force pushes, so be careful when
> > rebasing on top of newer versions.
> >  
> > I can do that if you don't want, just let me know so we can coordinate.
> >  
> of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
>  
>   
> > Just a quick heads-up, there will be virsysfs that will be used for the
> > reads, some additional helper functions in virhostcpu and virfile, test
> > that scans copy of /sys/devices/system (with that path faked thanks to
> > using the aforementioned virsysfs) and outputs capabilities so that we
> > can check the capability XML and so on.
> >  
> >  
> >  
>  
>  
> Ah, that’s a good news..  
> >  
> > Martin
> >  
> > [1] https://github.com/nertpinx/libvirt

hi Martin

So, if I understand you correctly , you want all my patch set to rebased on top of pre-cat branch [1] , I checked that the last commit is 15th March, I wonder if that ’s ready to merged into master?
so that I can start doing the rebasing

Thx Eli.  
> >  
> > > Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@intel.com)>
> > > ---
> > > include/libvirt/virterror.h | 1 +
> > > po/POTFILES.in (http://POTFILES.in) | 1 +
> > > src/Makefile.am (http://Makefile.am) | 1 +
> > > src/libvirt_private.syms | 4 +
> > > src/util/virerror.c | 1 +
> > > src/util/virhostcpu.c | 186 ++++++++++++++++++++++++++++++++++++----
> > > src/util/virhostcpu.h | 6 ++
> > > src/util/virresctrl.c | 201 ++++++++++++++++++++++++++++++++++++++++++++
> > > src/util/virresctrl.h | 78 +++++++++++++++++
> > > 9 files changed, 462 insertions(+), 17 deletions(-)
> > > create mode 100644 src/util/virresctrl.c
> > > create mode 100644 src/util/virresctrl.h
> > >  
> > > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> > > index 2efee8f..3dd2d08 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 resource control */
> > >  
> > > # ifdef VIR_ENUM_SENTINELS
> > > VIR_ERR_DOMAIN_LAST
> > > diff --git a/po/POTFILES.in (http://POTFILES.in) b/po/POTFILES.in (http://POTFILES.in)
> > > index 7c7f530..4147bc6 100644
> > > --- a/po/POTFILES.in (http://POTFILES.in)
> > > +++ b/po/POTFILES.in (http://POTFILES.in)
> > > @@ -241,6 +241,7 @@ src/util/virportallocator.c
> > > src/util/virprocess.c
> > > src/util/virqemu.c
> > > src/util/virrandom.c
> > > +src/util/virresctrl.c
> > > src/util/virrotatingfile.c
> > > src/util/virscsi.c
> > > src/util/virscsihost.c
> > > diff --git a/src/Makefile.am (http://Makefile.am) b/src/Makefile.am (http://Makefile.am)
> > > index 7d42eac..edb946a 100644
> > > --- a/src/Makefile.am (http://Makefile.am)
> > > +++ b/src/Makefile.am (http://Makefile.am)
> > > @@ -162,6 +162,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 aed1d3d..bb7c3ad 100644
> > > --- a/src/libvirt_private.syms
> > > +++ b/src/libvirt_private.syms
> > > @@ -2320,6 +2320,10 @@ virRandomGenerateWWN;
> > > virRandomInt;
> > >  
> > >  
> > > +# util/virresctrl.h
> > > +virResCtrlAvailable;
> > > +virResCtrlInit;
> > > +
> > > # util/virrotatingfile.h
> > > virRotatingFileReaderConsume;
> > > virRotatingFileReaderFree;
> > > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > > index ef17fb5..0ba15e6 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",
> > > + "Resouce Control",
> > > )
> > >  
> > >  
> > > diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> > > index f29f312..e6d5102 100644
> > > --- a/src/util/virhostcpu.c
> > > +++ b/src/util/virhostcpu.c
> > > @@ -206,29 +206,21 @@ void virHostCPUSetSysFSSystemPathLinux(const char *path)
> > > sysfs_system_path = SYSFS_SYSTEM_PATH;
> > > }
> > >  
> > > -/* Return the positive decimal contents of the given
> > > - * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative
> > > - * and the file could not be found, return that instead of an error;
> > > - * this is useful for machines that cannot hot-unplug cpu0, or where
> > > - * hot-unplugging is disabled, or where the kernel is too old
> > > - * to support NUMA cells, etc. */
> > > +/* Get a String value*/
> > > static int
> > > -virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > > - int default_value)
> > > +virHostCPUGetStrValue(const char *dir, unsigned int cpu, const char *file, char *value_str)
> > > {
> > > char *path;
> > > FILE *pathfp;
> > > - int value = -1;
> > > - char value_str[INT_BUFSIZE_BOUND(value)];
> > > - char *tmp;
> > > + int ret = -1;
> > >  
> > > if (virAsprintf(&path, "%s/cpu%u/%s", dir, cpu, file) < 0)
> > > return -1;
> > >  
> > > pathfp = fopen(path, "r");
> > > if (pathfp == NULL) {
> > > - if (default_value >= 0 && errno == ENOENT)
> > > - value = default_value;
> > > + if (errno == ENOENT)
> > > + return -2;
> > > else
> > > virReportSystemError(errno, _("cannot open %s"), path);
> > > goto cleanup;
> > > @@ -238,17 +230,84 @@ virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > > virReportSystemError(errno, _("cannot read from %s"), path);
> > > goto cleanup;
> > > }
> > > +
> > > + ret = 0;
> > > +
> > > + cleanup:
> > > + VIR_FORCE_FCLOSE(pathfp);
> > > + VIR_FREE(path);
> > > + return ret;
> > > +}
> > > +
> > > +
> > > +/* Return the positive decimal contents of the given
> > > + * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative
> > > + * and the file could not be found, return that instead of an error;
> > > + * this is useful for machines that cannot hot-unplug cpu0, or where
> > > + * hot-unplugging is disabled, or where the kernel is too old
> > > + * to support NUMA cells, etc. */
> > > +static int
> > > +virHostCPUGetValue(const char *dir, unsigned int cpu, const char *file,
> > > + int default_value)
> > > +{
> > > + int value = -1;
> > > + char value_str[INT_BUFSIZE_BOUND(value)];
> > > + char *tmp;
> > > + int ret;
> > > +
> > > + if ((ret = (virHostCPUGetStrValue(dir, cpu, file, value_str))) < 0) {
> > > + if (ret == -2 && default_value >= 0)
> > > + return default_value;
> > > + else
> > > + return -1;
> > > + }
> > > +
> > > if (virStrToLong_i(value_str, &tmp, 10, &value) < 0) {
> > > virReportError(VIR_ERR_INTERNAL_ERROR,
> > > _("could not convert '%s' to an integer"),
> > > value_str);
> > > - goto cleanup;
> > > + return -1;
> > > }
> > > + return value;
> > > +}
> > >  
> > > - cleanup:
> > > - VIR_FORCE_FCLOSE(pathfp);
> > > - VIR_FREE(path);
> > > +/* Return specific type cache size in KiB of given cpu
> > > + -1 on error happened */
> > > +static
> > > +int virHostCPUGetCache(unsigned int cpu, unsigned int type)
> > > +{
> > > + char *cachedir = NULL;
> > > + char *cpudir;
> > > + char *unit = NULL;
> > > + char *tmp;
> > > + int value = -1;
> > > + unsigned long long size;
> > > + char value_str[INT_BUFSIZE_BOUND(value)];
> > >  
> > > + if (virAsprintf(&cpudir, "%s/cpu", sysfs_system_path) < 0)
> > > + return -1;
> > > +
> > > + if (virAsprintf(&cachedir, "cache/index%u/size", type) < 0)
> > > + goto error;
> > > +
> > > + if (virHostCPUGetStrValue(cpudir, cpu, cachedir, value_str) < 0)
> > > + goto error;
> > > +
> > > + if ((tmp = strchr(value_str, '\n'))) *tmp = '\0';
> > > +
> > > + if (virStrToLong_i(value_str, &unit, 10, &value) < 0)
> > > + goto error;
> > > +
> > > + size = value;
> > > +
> > > + if (virScaleInteger(&size, unit, 1, ULLONG_MAX) < 0)
> > > + goto error;
> > > +
> > > + return size / 1024;
> > > +
> > > + error:
> > > + VIR_FREE(cpudir);
> > > + VIR_FREE(cachedir);
> > > return value;
> > > }
> > >  
> > > @@ -301,6 +360,23 @@ virHostCPUParseSocket(const char *dir,
> > > return ret;
> > > }
> > >  
> > > +/* return socket id of a given cpu*/
> > > +static
> > > +int virHostCPUGetSocketId(virArch hostarch, unsigned int cpu)
> > > +{
> > > + char *cpu_dir;
> > > + int ret = -1;
> > > +
> > > + if (virAsprintf(&cpu_dir, "%s/cpu", sysfs_system_path) < 0)
> > > + goto cleanup;
> > > +
> > > + ret = virHostCPUParseSocket(cpu_dir, hostarch, cpu);
> > > +
> > > + cleanup:
> > > + VIR_FREE(cpu_dir);
> > > + return ret;
> > > +}
> > > +
> > > /* parses a node entry, returning number of processors in the node and
> > > * filling arguments */
> > > static int
> > > @@ -1346,3 +1422,79 @@ virHostCPUGetKVMMaxVCPUs(void)
> > > return -1;
> > > }
> > > #endif /* HAVE_LINUX_KVM_H */
> > > +
> > > +/* Fill all cache bank informations
> > > + * Return a list of virResCacheBankPtr, and fill cache bank information
> > > + * by loop for all cpus on host, number of cache bank will be set in nbanks
> > > + *
> > > + * NULL if error happened, and nbanks will be set 0. */
> > > +virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch, int type, size_t *nbanks, int cbm_len)
> > > +{
> > > + int npresent_cpus;
> > > + int idx;
> > > + size_t i;
> > > + virResCacheBankPtr bank;
> > > +
> > > + *nbanks = 0;
> > > + if ((npresent_cpus = virHostCPUGetCount()) < 0)
> > > + return NULL;
> > > +
> > > + switch (type) {
> > > + case VIR_RDT_RESOURCE_L3:
> > > + case VIR_RDT_RESOURCE_L3DATA:
> > > + case VIR_RDT_RESOURCE_L3CODE:
> > > + idx = 3;
> > > + break;
> > > + case VIR_RDT_RESOURCE_L2:
> > > + idx = 2;
> > > + break;
> > > + default:
> > > + idx = -1;
> > > + }
> > > +
> > > + if (idx == -1)
> > > + return NULL;
> > > +
> > > + if (VIR_ALLOC_N(bank, 1) < 0)
> > > + return NULL;
> > > +
> > > + *nbanks = 1;
> > > +
> > > + for (i = 0; i < npresent_cpus; i ++) {
> > > + int s_id;
> > > + int cache_size;
> > > +
> > > + if ((s_id = virHostCPUGetSocketId(arch, i)) < 0)
> > > + goto error;
> > > +
> > > + /* Expand cache bank array */
> > > + if (s_id > (*nbanks - 1)) {
> > > + size_t cur = *nbanks;
> > > + size_t exp = s_id - (*nbanks) + 1;
> > > + if (VIR_EXPAND_N(bank, cur, exp) < 0)
> > > + goto error;
> > > + *nbanks = s_id + 1;
> > > + }
> > > +
> > > + if (bank[s_id].cpu_mask == NULL) {
> > > + if (!(bank[s_id].cpu_mask = virBitmapNew(npresent_cpus)))
> > > + goto error;
> > > + }
> > > +
> > > + ignore_value(virBitmapSetBit(bank[s_id].cpu_mask, i));
> > > +
> > > + if (bank[s_id].cache_size == 0) {
> > > + if ((cache_size = virHostCPUGetCache(i, idx)) < 0)
> > > + goto error;
> > > +
> > > + bank[s_id].cache_size = cache_size;
> > > + bank[s_id].cache_min = cache_size / cbm_len;
> > > + }
> > > + }
> > > + return bank;
> > > +
> > > + error:
> > > + *nbanks = 0;
> > > + VIR_FREE(bank);
> > > + return NULL;
> > > +}
> > > diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
> > > index 39f7cf8..27f208e 100644
> > > --- a/src/util/virhostcpu.h
> > > +++ b/src/util/virhostcpu.h
> > > @@ -27,6 +27,7 @@
> > > # include "internal.h"
> > > # include "virarch.h"
> > > # include "virbitmap.h"
> > > +# include "virresctrl.h"
> > >  
> > > # define VIR_HOST_CPU_MASK_LEN 1024
> > >  
> > > @@ -58,4 +59,9 @@ int virHostCPUStatsAssign(virNodeCPUStatsPtr param,
> > > const char *name,
> > > unsigned long long value);
> > >  
> > > +virResCacheBankPtr virHostCPUGetCacheBanks(virArch arch,
> > > + int type,
> > > + size_t *nbanks,
> > > + int cbm_len);
> > > +
> > > #endif /* __VIR_HOSTCPU_H__*/
> > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > > new file mode 100644
> > > index 0000000..44a47cc
> > > --- /dev/null
> > > +++ b/src/util/virresctrl.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * virresctrl.c: methods for managing resource control
> > > + *
> > > + * 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@intel.com (mailto:liyong.qiao@intel.com)>
> > > + */
> > > +#include <config.h>
> > > +
> > > +#include "virresctrl.h"
> > > +#include "viralloc.h"
> > > +#include "virerror.h"
> > > +#include "virfile.h"
> > > +#include "virhostcpu.h"
> > > +#include "virlog.h"
> > > +#include "virstring.h"
> > > +#include "virarch.h"
> > > +
> > > +VIR_LOG_INIT("util.resctrl");
> > > +
> > > +#define VIR_FROM_THIS VIR_FROM_RESCTRL
> > > +
> > > +#define RESCTRL_DIR "/sys/fs/resctrl"
> > > +#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
> > > +#define SYSFS_SYSTEM_PATH "/sys/devices/system"
> > > +
> > > +#define MAX_CPU_SOCKET_NUM 8
> > > +#define MAX_CBM_BIT_LEN 32
> > > +#define MAX_SCHEMATA_LEN 1024
> > > +#define MAX_FILE_LEN (10 * 1024 * 1024)
> > > +
> > > +static unsigned int host_id;
> > > +
> > > +static virResCtrl resctrlall[] = {
> > > + {
> > > + .name = "L3",
> > > + .cache_level = "l3",
> > > + },
> > > + {
> > > + .name = "L3DATA",
> > > + .cache_level = "l3",
> > > + },
> > > + {
> > > + .name = "L3CODE",
> > > + .cache_level = "l3",
> > > + },
> > > + {
> > > + .name = "L2",
> > > + .cache_level = "l2",
> > > + },
> > > +};
> > > +
> > > +static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
> > > +{
> > > + int ret = 0;
> > > + char *tmp;
> > > + char *path;
> > > +
> > > + if (virAsprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, resctrlall[type].name, item) < 0)
> > > + return -1;
> > > + if (virFileReadAll(path, 10, str) < 0) {
> > > + ret = -1;
> > > + goto cleanup;
> > > + }
> > > +
> > > + if ((tmp = strchr(*str, '\n'))) *tmp = '\0';
> > > +
> > > + cleanup:
> > > + VIR_FREE(path);
> > > + return ret;
> > > +}
> > > +
> > > +static int virResCtrlReadConfig(virArch arch, int type)
> > > +{
> > > + int ret;
> > > + size_t i, nbanks;
> > > + char *str;
> > > +
> > > + /* Read num_closids from resctrl.
> > > + eg: /sys/fs/resctrl/info/L3/num_closids */
> > > + if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)
> > > + goto error;
> > > +
> > > + if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid)) < 0)
> > > + goto error;
> > > +
> > > + VIR_FREE(str);
> > > +
> > > + /* Read min_cbm_bits from resctrl.
> > > + eg: /sys/fs/resctrl/info/L3/cbm_mask */
> > > + if ((ret = virResCtrlGetInfoStr(type, "min_cbm_bits", &str)) < 0)
> > > + goto error;
> > > +
> > > + if ((ret = virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_bits)) < 0)
> > > + goto error;
> > > +
> > > + VIR_FREE(str);
> > > +
> > > + /* Read cbm_mask string from resctrl.
> > > + eg: /sys/fs/resctrl/info/L3/cbm_mask */
> > > + if ((ret = virResCtrlGetInfoStr(type, "cbm_mask", &str)) < 0)
> > > + goto error;
> > > +
> > > + /* cbm_mask is in hex, eg: "fffff", calculate cbm length from the default
> > > + cbm_mask. */
> > > + resctrlall[type].cbm_len = strlen(str) * 4;
> > > +
> > > + /* Get all cache bank informations */
> > > + resctrlall[type].cache_banks = virHostCPUGetCacheBanks(arch,
> > > + type,
> > > + &nbanks, resctrlall[type].cbm_len);
> > > +
> > > + if (resctrlall[type].cache_banks == NULL)
> > > + goto error;
> > > +
> > > + resctrlall[type].num_banks = nbanks;
> > > +
> > > + for (i = 0; i < resctrlall[type].num_banks; i++) {
> > > + /* L3CODE and L3DATA shares same L3 resource, so they should
> > > + * have same host_id. */
> > > + if (type == VIR_RDT_RESOURCE_L3CODE)
> > > + resctrlall[type].cache_banks[i].host_id = resctrlall[VIR_RDT_RESOURCE_L3DATA].cache_banks[i].host_id;
> > > + else
> > > + resctrlall[type].cache_banks[i].host_id = host_id++;
> > > + }
> > > +
> > > + resctrlall[type].enabled = true;
> > > +
> > > + ret = 0;
> > > +
> > > + error:
> > > + VIR_FREE(str);
> > > + return ret;
> > > +}
> > > +
> > > +int
> > > +virResCtrlInit(void)
> > > +{
> > > + size_t i = 0;
> > > + char *tmp;
> > > + int rc = 0;
> > > +
> > > + virArch hostarch;
> > > +
> > > + hostarch = virArchFromHost();
> > > +
> > > + for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
> > > + if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {
> > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > + _("Failed to initialize resource control config"));
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (virFileExists(tmp)) {
> > > + if ((rc = virResCtrlReadConfig(hostarch, i)) < 0) {
> > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > + _("Failed to get resource control config"));
> > > + goto cleanup;
> > > + }
> > > + }
> > > + VIR_FREE(tmp);
> > > + }
> > > +
> > > + cleanup:
> > > + VIR_FREE(tmp);
> > > + return rc;
> > > +}
> > > +
> > > +/*
> > > + * Test whether the host support resource control
> > > + */
> > > +bool
> > > +virResCtrlAvailable(void)
> > > +{
> > > + if (!virFileExists(RESCTRL_INFO_DIR))
> > > + return false;
> > > + return true;
> > > +}
> > > +
> > > +/*
> > > + * Return an virResCtrlPtr point to virResCtrl object,
> > > + * We should not modify it out side of virresctrl.c
> > > + */
> > > +virResCtrlPtr
> > > +virResCtrlGet(int type)
> > > +{
> > > + return &resctrlall[type];
> > > +}
> > > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> > > new file mode 100644
> > > index 0000000..5a6a344
> > > --- /dev/null
> > > +++ b/src/util/virresctrl.h
> > > @@ -0,0 +1,78 @@
> > > +/*
> > > + * virresctrl.h: header for managing resctrl control
> > > + *
> > > + * Copyright (C) 2016 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@intel.com (mailto:liyong.qiao@intel.com)>
> > > + */
> > > +
> > > +#ifndef __VIR_RESCTRL_H__
> > > +# define __VIR_RESCTRL_H__
> > > +
> > > +# include "virbitmap.h"
> > > +
> > > +enum {
> > > + VIR_RDT_RESOURCE_L3,
> > > + VIR_RDT_RESOURCE_L3DATA,
> > > + VIR_RDT_RESOURCE_L3CODE,
> > > + VIR_RDT_RESOURCE_L2,
> > > + /* Must be the last */
> > > + VIR_RDT_RESOURCE_LAST,
> > > +};
> > > +
> > > +
> > > +typedef struct _virResCacheBank virResCacheBank;
> > > +typedef virResCacheBank *virResCacheBankPtr;
> > > +struct _virResCacheBank {
> > > + unsigned int host_id;
> > > + unsigned long long cache_size;
> > > + unsigned long long cache_left;
> > > + unsigned long long cache_min;
> > > + virBitmapPtr cpu_mask;
> > > +};
> > > +
> > > +/**
> > > + * struct rdt_resource - attributes of an RDT resource
> > > + * @enabled: Is this feature enabled on this machine
> > > + * @name: Name to use in "schemata" file
> > > + * @num_closid: Number of CLOSIDs available
> > > + * @max_cbm: Largest Cache Bit Mask allowed
> > > + * @min_cbm_bits: Minimum number of consecutive bits to be set
> > > + * in a cache bit mask
> > > + * @cache_level: Which cache level defines scope of this domain
> > > + * @num_banks: Number of cache bank on this machine.
> > > + * @cache_banks: Array of cache bank
> > > + */
> > > +typedef struct _virResCtrl virResCtrl;
> > > +typedef virResCtrl *virResCtrlPtr;
> > > +struct _virResCtrl {
> > > + bool enabled;
> > > + const char *name;
> > > + int num_closid;
> > > + int cbm_len;
> > > + int min_cbm_bits;
> > > + const char* cache_level;
> > > + int num_banks;
> > > + virResCacheBankPtr cache_banks;
> > > +};
> > > +
> > > +bool virResCtrlAvailable(void);
> > > +int virResCtrlInit(void);
> > > +virResCtrlPtr virResCtrlGet(int);
> > > +
> > > +#endif
> > > --
> > > 1.9.1
> > >  
> > > --
> > > libvir-list mailing list
> > > libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> > > https://www.redhat.com/mailman/listinfo/libvir-list
> > >  
> >  
> >  
> >  
> >  
>  
>  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Fri, Mar 24, 2017 at 09:35:33AM +0800, Eli Qiao wrote:
>
>
>--
>Best regards
>Eli
>
>天涯无处不重逢
>a leaf duckweed belongs to the sea, where not to meet in life
>
>Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>
>
>On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:
>
>>
>>
>> --
>> Best regards
>> Eli
>>
>> 天涯无处不重逢
>> a leaf duckweed belongs to the sea, where not to meet in life
>>
>> Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>>
>>
>> On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
>>
>> > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
>> > > This patch adds some utils struct and functions to expose resctrl
>> > > information.
>> > >
>> > > virResCtrlAvailable: if resctrl interface exist on host.
>> > > virResCtrlGet: get specific type resource control information.
>> > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
>> > > resctrlall[]: an array to maintain resource control information.
>> > >
>> > > Some of host cpu related information methods was added in virhostcpu.c
>> >
>> > So to be able to test all this we need to make a bit different approach.
>> > I'm not in favour of pushing this without proper tests. Some paths need
>> > to be configurable, some readings should be unified. Unfortunately lot
>> > of the code is just copy-paste mess from the past. Fortunately for you,
>> >
>> >
>> >
>> > I'm working on cleaning this up, at least a little bit, so that we can
>> >
>> >
>> >
>>
>> Good news.
>> > add the tests easily. I got almost up to the test (I stumbled upon few
>> > rabbit holes on the way and some clean-ups went wrong along the way), so
>> > it should be pretty easy for you to modify this code to use what I'll be
>> > proposing to add. It's not ready yet, but you can start rebasing your
>> > series on top of my branch pre-cat from my github repo [1]. The commits
>> > are not very well described right now (for some temporary ones I used
>> > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
>> > branch, but it will be done with force pushes, so be careful when
>> > rebasing on top of newer versions.
>> >
>> > I can do that if you don't want, just let me know so we can coordinate.
>> >
>> of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
>>
>>
>> > Just a quick heads-up, there will be virsysfs that will be used for the
>> > reads, some additional helper functions in virhostcpu and virfile, test
>> > that scans copy of /sys/devices/system (with that path faked thanks to
>> > using the aforementioned virsysfs) and outputs capabilities so that we
>> > can check the capability XML and so on.
>> >
>> >
>> >
>>
>>
>> Ah, that’s a good news..
>> >
>> > Martin
>> >
>> > [1] https://github.com/nertpinx/libvirt
>
>hi Martin
>
>So, if I understand you correctly , you want all my patch set to rebased on top of pre-cat branch [1] , I checked that the last commit is 15th March, I wonder if that ’s ready to merged into master?
>so that I can start doing the rebasing
>

I forgot to do the usual push, it's updated now.  The test fails in one
occasion, but it's the code's fault, the test is fine.  That's the last
thing I'm looking at now, after that I'll send it to the list.

Look at the changes and see what you can use, it will help simplifying
your code a lot, I thing.  You can start rebasing on top of that, I'll
do that as well after it's posted and I'll be either using and modifying
your patches or maybe doing some myself.

Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago

hi Martin

(cc libvir-list)
  
I am a little confused about cat support.  

I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?

[1] https://github.com/nertpinx/libvirt/commit/c335de47a4efeca87f23e641a93587b1e036e558

Thanks Eli.



--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Friday, 24 March 2017 at 3:42 PM, Martin Kletzander wrote:

> On Fri, Mar 24, 2017 at 09:35:33AM +0800, Eli Qiao wrote:
> >  
> >  
> > --
> > Best regards
> > Eli
> >  
> > 天涯无处不重逢
> > a leaf duckweed belongs to the sea, where not to meet in life
> >  
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> >  
> >  
> > On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:
> >  
> > >  
> > >  
> > > --
> > > Best regards
> > > Eli
> > >  
> > > 天涯无处不重逢
> > > a leaf duckweed belongs to the sea, where not to meet in life
> > >  
> > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > >  
> > >  
> > > On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
> > >  
> > > > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > > > > This patch adds some utils struct and functions to expose resctrl
> > > > > information.
> > > > >  
> > > > > virResCtrlAvailable: if resctrl interface exist on host.
> > > > > virResCtrlGet: get specific type resource control information.
> > > > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > > > resctrlall[]: an array to maintain resource control information.
> > > > >  
> > > > > Some of host cpu related information methods was added in virhostcpu.c
> > > >  
> > > > So to be able to test all this we need to make a bit different approach.
> > > > I'm not in favour of pushing this without proper tests. Some paths need
> > > > to be configurable, some readings should be unified. Unfortunately lot
> > > > of the code is just copy-paste mess from the past. Fortunately for you,
> > > >  
> > > >  
> > > >  
> > > > I'm working on cleaning this up, at least a little bit, so that we can
> > >  
> > > Good news.
> > > > add the tests easily. I got almost up to the test (I stumbled upon few
> > > > rabbit holes on the way and some clean-ups went wrong along the way), so
> > > > it should be pretty easy for you to modify this code to use what I'll be
> > > > proposing to add. It's not ready yet, but you can start rebasing your
> > > > series on top of my branch pre-cat from my github repo [1]. The commits
> > > > are not very well described right now (for some temporary ones I used
> > > > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
> > > > branch, but it will be done with force pushes, so be careful when
> > > > rebasing on top of newer versions.
> > > >  
> > > > I can do that if you don't want, just let me know so we can coordinate.
> > > of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
> > >  
> > >  
> > > > Just a quick heads-up, there will be virsysfs that will be used for the
> > > > reads, some additional helper functions in virhostcpu and virfile, test
> > > > that scans copy of /sys/devices/system (with that path faked thanks to
> > > > using the aforementioned virsysfs) and outputs capabilities so that we
> > > > can check the capability XML and so on.
> > > >  
> > >  
> > >  
> > >  
> > > Ah, that’s a good news..
> > > >  
> > > > Martin
> > > >  
> > > > [1] https://github.com/nertpinx/libvirt
> >  
> > hi Martin
> >  
> > So, if I understand you correctly , you want all my patch set to rebased on top of pre-cat branch [1] , I checked that the last commit is 15th March, I wonder if that ’s ready to merged into master?
> > so that I can start doing the rebasing
> >  
>  
>  
> I forgot to do the usual push, it's updated now. The test fails in one
> occasion, but it's the code's fault, the test is fine. That's the last
> thing I'm looking at now, after that I'll send it to the list.
>  
> Look at the changes and see what you can use, it will help simplifying
> your code a lot, I thing. You can start rebasing on top of that, I'll
> do that as well after it's posted and I'll be either using and modifying
> your patches or maybe doing some myself.
>  
> Martin  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
>hi Martin
>
>(cc libvir-list)
>
>I am a little confused about cat support.
>
>I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
>

So we can work together on that.  Since the rework of the sysfs
functions, some patches are easier to write from scratch then rewrite,
but I'm now just trying to setup the test suite, so that we have
something to test on, at least some of the code.  So where are you in
the rebase right now?  Do you think anything from the virsysfs.c code
could be enhanced?

>[1] https://github.com/nertpinx/libvirt/commit/c335de47a4efeca87f23e641a93587b1e036e558
>
>Thanks Eli.
>
>
>
>--
>Best regards
>Eli
>
>天涯无处不重逢
>a leaf duckweed belongs to the sea, where not to meet in life
>
>Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>
>
>On Friday, 24 March 2017 at 3:42 PM, Martin Kletzander wrote:
>
>> On Fri, Mar 24, 2017 at 09:35:33AM +0800, Eli Qiao wrote:
>> >
>> >
>> > --
>> > Best regards
>> > Eli
>> >
>> > 天涯无处不重逢
>> > a leaf duckweed belongs to the sea, where not to meet in life
>> >
>> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>> >
>> >
>> > On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:
>> >
>> > >
>> > >
>> > > --
>> > > Best regards
>> > > Eli
>> > >
>> > > 天涯无处不重逢
>> > > a leaf duckweed belongs to the sea, where not to meet in life
>> > >
>> > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>> > >
>> > >
>> > > On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
>> > >
>> > > > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
>> > > > > This patch adds some utils struct and functions to expose resctrl
>> > > > > information.
>> > > > >
>> > > > > virResCtrlAvailable: if resctrl interface exist on host.
>> > > > > virResCtrlGet: get specific type resource control information.
>> > > > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
>> > > > > resctrlall[]: an array to maintain resource control information.
>> > > > >
>> > > > > Some of host cpu related information methods was added in virhostcpu.c
>> > > >
>> > > > So to be able to test all this we need to make a bit different approach.
>> > > > I'm not in favour of pushing this without proper tests. Some paths need
>> > > > to be configurable, some readings should be unified. Unfortunately lot
>> > > > of the code is just copy-paste mess from the past. Fortunately for you,
>> > > >
>> > > >
>> > > >
>> > > > I'm working on cleaning this up, at least a little bit, so that we can
>> > >
>> > > Good news.
>> > > > add the tests easily. I got almost up to the test (I stumbled upon few
>> > > > rabbit holes on the way and some clean-ups went wrong along the way), so
>> > > > it should be pretty easy for you to modify this code to use what I'll be
>> > > > proposing to add. It's not ready yet, but you can start rebasing your
>> > > > series on top of my branch pre-cat from my github repo [1]. The commits
>> > > > are not very well described right now (for some temporary ones I used
>> > > > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
>> > > > branch, but it will be done with force pushes, so be careful when
>> > > > rebasing on top of newer versions.
>> > > >
>> > > > I can do that if you don't want, just let me know so we can coordinate.
>> > > of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
>> > >
>> > >
>> > > > Just a quick heads-up, there will be virsysfs that will be used for the
>> > > > reads, some additional helper functions in virhostcpu and virfile, test
>> > > > that scans copy of /sys/devices/system (with that path faked thanks to
>> > > > using the aforementioned virsysfs) and outputs capabilities so that we
>> > > > can check the capability XML and so on.
>> > > >
>> > >
>> > >
>> > >
>> > > Ah, that’s a good news..
>> > > >
>> > > > Martin
>> > > >
>> > > > [1] https://github.com/nertpinx/libvirt
>> >
>> > hi Martin
>> >
>> > So, if I understand you correctly , you want all my patch set to rebased on top of pre-cat branch [1] , I checked that the last commit is 15th March, I wonder if that ’s ready to merged into master?
>> > so that I can start doing the rebasing
>> >
>>
>>
>> I forgot to do the usual push, it's updated now. The test fails in one
>> occasion, but it's the code's fault, the test is fine. That's the last
>> thing I'm looking at now, after that I'll send it to the list.
>>
>> Look at the changes and see what you can use, it will help simplifying
>> your code a lot, I thing. You can start rebasing on top of that, I'll
>> do that as well after it's posted and I'll be either using and modifying
>> your patches or maybe doing some myself.
>>
>> Martin
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:

> On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
> > hi Martin
> >  
> > (cc libvir-list)
> >  
> > I am a little confused about cat support.
> >  
> > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
>  
> So we can work together on that. Since the rework of the sysfs
> functions, some patches are easier to write from scratch then rewrite,
> but I'm now just trying to setup the test suite, so that we have
> something to test on, at least some of the code. So where are you in
> the rebase right now? Do you think anything from the virsysfs.c code
> could be enhanced?
>  
>  



Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(  

I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c

thought ?

[1]https://github.com/taget/libvirt/commits/cdp_v11
  
>  
> > [1] https://github.com/nertpinx/libvirt/commit/c335de47a4efeca87f23e641a93587b1e036e558
> >  
> > Thanks Eli.
> >  
> >  
> >  
> > --
> > Best regards
> > Eli
> >  
> > 天涯无处不重逢
> > a leaf duckweed belongs to the sea, where not to meet in life
> >  
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> >  
> >  
> > On Friday, 24 March 2017 at 3:42 PM, Martin Kletzander wrote:
> >  
> > > On Fri, Mar 24, 2017 at 09:35:33AM +0800, Eli Qiao wrote:
> > > >  
> > > >  
> > > > --
> > > > Best regards
> > > > Eli
> > > >  
> > > > 天涯无处不重逢
> > > > a leaf duckweed belongs to the sea, where not to meet in life
> > > >  
> > > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > > >  
> > > >  
> > > > On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:
> > > >  
> > > > >  
> > > > >  
> > > > > --
> > > > > Best regards
> > > > > Eli
> > > > >  
> > > > > 天涯无处不重逢
> > > > > a leaf duckweed belongs to the sea, where not to meet in life
> > > > >  
> > > > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > > > >  
> > > > >  
> > > > > On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
> > > > >  
> > > > > > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > > > > > > This patch adds some utils struct and functions to expose resctrl
> > > > > > > information.
> > > > > > >  
> > > > > > > virResCtrlAvailable: if resctrl interface exist on host.
> > > > > > > virResCtrlGet: get specific type resource control information.
> > > > > > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > > > > > resctrlall[]: an array to maintain resource control information.
> > > > > > >  
> > > > > > > Some of host cpu related information methods was added in virhostcpu.c
> > > > > >  
> > > > > > So to be able to test all this we need to make a bit different approach.
> > > > > > I'm not in favour of pushing this without proper tests. Some paths need
> > > > > > to be configurable, some readings should be unified. Unfortunately lot
> > > > > > of the code is just copy-paste mess from the past. Fortunately for you,
> > > > > >  
> > > > > >  
> > > > > >  
> > > > > > I'm working on cleaning this up, at least a little bit, so that we can
> > > > >  
> > > > > Good news.
> > > > > > add the tests easily. I got almost up to the test (I stumbled upon few
> > > > > > rabbit holes on the way and some clean-ups went wrong along the way), so
> > > > > > it should be pretty easy for you to modify this code to use what I'll be
> > > > > > proposing to add. It's not ready yet, but you can start rebasing your
> > > > > > series on top of my branch pre-cat from my github repo [1]. The commits
> > > > > > are not very well described right now (for some temporary ones I used
> > > > > > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all that. I'll be updating the
> > > > > > branch, but it will be done with force pushes, so be careful when
> > > > > > rebasing on top of newer versions.
> > > > > >  
> > > > > > I can do that if you don't want, just let me know so we can coordinate.
> > > > > of cause we can do some coordinate, but I am glad that you can help on this to speed up the progress to merge them, as you know this patch is in V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
> > > > >  
> > > > >  
> > > > > > Just a quick heads-up, there will be virsysfs that will be used for the
> > > > > > reads, some additional helper functions in virhostcpu and virfile, test
> > > > > > that scans copy of /sys/devices/system (with that path faked thanks to
> > > > > > using the aforementioned virsysfs) and outputs capabilities so that we
> > > > > > can check the capability XML and so on.
> > > > > >  
> > > > >  
> > > > >  
> > > > >  
> > > > >  
> > > > > Ah, that’s a good news..
> > > > > >  
> > > > > > Martin
> > > > > >  
> > > > > > [1] https://github.com/nertpinx/libvirt
> > > >  
> > > > hi Martin
> > > >  
> > > > So, if I understand you correctly , you want all my patch set to rebased on top of pre-cat branch [1] , I checked that the last commit is 15th March, I wonder if that ’s ready to merged into master?
> > > > so that I can start doing the rebasing
> > > >  
> > >  
> > >  
> > >  
> > > I forgot to do the usual push, it's updated now. The test fails in one
> > > occasion, but it's the code's fault, the test is fine. That's the last
> > > thing I'm looking at now, after that I'll send it to the list.
> > >  
> > > Look at the changes and see what you can use, it will help simplifying
> > > your code a lot, I thing. You can start rebasing on top of that, I'll
> > > do that as well after it's posted and I'll be either using and modifying
> > > your patches or maybe doing some myself.
> > >  
> > > Martin  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Wed, Mar 29, 2017 at 04:22:16PM +0800, Eli Qiao wrote:
>
>
>--
>Best regards
>Eli
>
>天涯无处不重逢
>a leaf duckweed belongs to the sea, where not to meet in life
>
>Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>
>
>On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:
>
>> On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
>> > hi Martin
>> >
>> > (cc libvir-list)
>> >
>> > I am a little confused about cat support.
>> >
>> > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
>>
>> So we can work together on that. Since the rework of the sysfs
>> functions, some patches are easier to write from scratch then rewrite,
>> but I'm now just trying to setup the test suite, so that we have
>> something to test on, at least some of the code. So where are you in
>> the rebase right now? Do you think anything from the virsysfs.c code
>> could be enhanced?
>>
>>
>
>
>
>Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(
>
>I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c
>
>thought ?
>

Yeah, we should wrap around /sys/fs/resctrl as we do with
/sys/devices/system so that it can be easily tested.

Also I got another idea about keeping the resource info.  There is no
need for any global data to be stored as you are re-reading almost all
of it.  The only info that stays the same is caches (that is already
saved in capabilities) and what caches are available for resource
control (that will be there as well).  So I don't think we need yet
another global data storage.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Wednesday, 29 March 2017 at 5:19 PM, Martin Kletzander wrote:

> On Wed, Mar 29, 2017 at 04:22:16PM +0800, Eli Qiao wrote:
> >  
> >  
> > --
> > Best regards
> > Eli
> >  
> > 天涯无处不重逢
> > a leaf duckweed belongs to the sea, where not to meet in life
> >  
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> >  
> >  
> > On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:
> >  
> > > On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
> > > > hi Martin
> > > >  
> > > > (cc libvir-list)
> > > >  
> > > > I am a little confused about cat support.
> > > >  
> > > > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
> > >  
> > > So we can work together on that. Since the rework of the sysfs
> > > functions, some patches are easier to write from scratch then rewrite,
> > > but I'm now just trying to setup the test suite, so that we have
> > > something to test on, at least some of the code. So where are you in
> > > the rebase right now? Do you think anything from the virsysfs.c code
> > > could be enhanced?
> > >  
> >  
> >  
> >  
> >  
> > Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(
> >  
> > I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c
> >  
> > thought ?
>  
> Yeah, we should wrap around /sys/fs/resctrl as we do with
> /sys/devices/system so that it can be easily tested.
>  
Sure, working on it, and done, will push it for review.

Also I will push some fake data for resctrl testing..
  
  
>  
> Also I got another idea about keeping the resource info. There is no
> need for any global data to be stored as you are re-reading almost all
> of it. The only info that stays the same is caches (that is already
> saved in capabilities) and what caches are available for resource
> control (that will be there as well). So I don't think we need yet
> another global data storage.
>  
>  

Do you mean, we re-create all struct (reading them from /sys/fs/resctrl) when we create/destroy instance?  
also, for get free cache ?

This is what I did in my early PoC, that will much easier… but please keep in mind that only one thread can read/write to /sys/fs/resctrl at one time.

the neck bottle is /sys/fs/resctrl
  
>   
>  
>  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Wed, Mar 29, 2017 at 05:31:42PM +0800, Eli Qiao wrote:
>
>
>--
>Best regards
>Eli
>
>天涯无处不重逢
>a leaf duckweed belongs to the sea, where not to meet in life
>
>Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>
>

OK, please do something with your client.  Having the footer here on top
for every reply is *sooooo* bothersome when you are replying inline
(that part is fine).

>On Wednesday, 29 March 2017 at 5:19 PM, Martin Kletzander wrote:
>
>> On Wed, Mar 29, 2017 at 04:22:16PM +0800, Eli Qiao wrote:
>> >
>> >
>> > --
>> > Best regards
>> > Eli
>> >
>> > 天涯无处不重逢
>> > a leaf duckweed belongs to the sea, where not to meet in life
>> >
>> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>> >
>> >
>> > On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:
>> >
>> > > On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
>> > > > hi Martin
>> > > >
>> > > > (cc libvir-list)
>> > > >
>> > > > I am a little confused about cat support.
>> > > >
>> > > > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
>> > >
>> > > So we can work together on that. Since the rework of the sysfs
>> > > functions, some patches are easier to write from scratch then rewrite,
>> > > but I'm now just trying to setup the test suite, so that we have
>> > > something to test on, at least some of the code. So where are you in
>> > > the rebase right now? Do you think anything from the virsysfs.c code
>> > > could be enhanced?
>> > >
>> >
>> >
>> >
>> >
>> > Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(
>> >
>> > I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c
>> >
>> > thought ?
>>
>> Yeah, we should wrap around /sys/fs/resctrl as we do with
>> /sys/devices/system so that it can be easily tested.
>>
>Sure, working on it, and done, will push it for review.
>
>Also I will push some fake data for resctrl testing..
>
>
>>
>> Also I got another idea about keeping the resource info. There is no
>> need for any global data to be stored as you are re-reading almost all
>> of it. The only info that stays the same is caches (that is already
>> saved in capabilities) and what caches are available for resource
>> control (that will be there as well). So I don't think we need yet
>> another global data storage.
>>
>>
>
>Do you mean, we re-create all struct (reading them from /sys/fs/resctrl) when we create/destroy instance?
>also, for get free cache ?
>

You have to update that for every request anyway, so what's the point of
keeping the data when they immediately become old?

>This is what I did in my early PoC, that will much easier… but please keep in mind that only one thread can read/write to /sys/fs/resctrl at one time.
>

Yeah, that's what we have locks for.

>the neck bottle is /sys/fs/resctrl
>

Sure you mean bottleneck, right? :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Eli Qiao 8 years, 3 months ago


On Wednesday, 29 March 2017 at 5:47 PM, Martin Kletzander wrote:

> On Wed, Mar 29, 2017 at 05:31:42PM +0800, Eli Qiao wrote:
> >  
> >  
> > --
> > Best regards
> > Eli
> >  
> > 天涯无处不重逢
> > a leaf duckweed belongs to the sea, where not to meet in life
> >  
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>  
> OK, please do something with your client. Having the footer here on top
> for every reply is *sooooo* bothersome when you are replying inline
> (that part is fine).
>  
Sorry, I removed footer, better now?
  
>  
> > On Wednesday, 29 March 2017 at 5:19 PM, Martin Kletzander wrote:
> >  
> > > On Wed, Mar 29, 2017 at 04:22:16PM +0800, Eli Qiao wrote:
> > > >  
> > > >  
> > > > --
> > > > Best regards
> > > > Eli
> > > >  
> > > > 天涯无处不重逢
> > > > a leaf duckweed belongs to the sea, where not to meet in life
> > > >  
> > > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > > >  
> > > >  
> > > > On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:
> > > >  
> > > > > On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:
> > > > > > hi Martin
> > > > > >  
> > > > > > (cc libvir-list)
> > > > > >  
> > > > > > I am a little confused about cat support.
> > > > > >  
> > > > > > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
> > > > >  
> > > > > So we can work together on that. Since the rework of the sysfs
> > > > > functions, some patches are easier to write from scratch then rewrite,
> > > > > but I'm now just trying to setup the test suite, so that we have
> > > > > something to test on, at least some of the code. So where are you in
> > > > > the rebase right now? Do you think anything from the virsysfs.c code
> > > > > could be enhanced?
> > > > >  
> > > >  
> > > >  
> > > >  
> > > >  
> > > >  
> > > > Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(
> > > >  
> > > > I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c
> > > >  
> > > > thought ?
> > >  
> > > Yeah, we should wrap around /sys/fs/resctrl as we do with
> > > /sys/devices/system so that it can be easily tested.
> > >  
> >  
> > Sure, working on it, and done, will push it for review.
> >  
> > Also I will push some fake data for resctrl testing..
> >  
> >  
> > >  
> > > Also I got another idea about keeping the resource info. There is no
> > > need for any global data to be stored as you are re-reading almost all
> > > of it. The only info that stays the same is caches (that is already
> > > saved in capabilities) and what caches are available for resource
> > > control (that will be there as well). So I don't think we need yet
> > > another global data storage.
> > >  
> >  
> >  
> > Do you mean, we re-create all struct (reading them from /sys/fs/resctrl) when we create/destroy instance?
> > also, for get free cache ?
> >  
>  
>  
> You have to update that for every request anyway, so what's the point of
> keeping the data when they immediately become old?
>  
I was thought that may reduce the time costing, not all of the content be refreshed, anyway, I will try to avoid global files in my later version.  

LoL lots of rebasing  :(  

Thanks for your suggestion.
>  
> > This is what I did in my early PoC, that will much easier… but please keep in mind that only one thread can read/write to /sys/fs/resctrl at one time.
>  
>  
> Yeah, that's what we have locks for.
>  
> > the neck bottle is /sys/fs/resctrl
>  
> Sure you mean bottleneck, right? :)
yes, bottleneck,  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

Posted by Martin Kletzander 8 years, 3 months ago

On Wed, Mar 29, 2017 at 05:59:47PM +0800, Eli Qiao wrote:
>
>
>On Wednesday, 29 March 2017 at 5:47 PM, Martin Kletzander wrote:
>
>> On Wed, Mar 29, 2017 at 05:31:42PM +0800, Eli Qiao wrote:
>> >
>> >
>> > --
>> > Best regards
>> > Eli
>> >
>> > 天涯无处不重逢
>> > a leaf duckweed belongs to the sea, where not to meet in life
>> >
>> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>>
>> OK, please do something with your client. Having the footer here on top
>> for every reply is *sooooo* bothersome when you are replying inline
>> (that part is fine).
>>
>Sorry, I removed footer, better now?
>

Way better, thank you very much.  I always didn't know whether to look
for the rest of the message or not.  And as you can see, after some
replies, it's hard to read what was discussed:

>>
>> > On Wednesday, 29 March 2017 at 5:19 PM, Martin Kletzander wrote:
>> >
>> > > On Wed, Mar 29, 2017 at 04:22:16PM +0800, Eli Qiao wrote:
>> > > >
>> > > >
>> > > > --
>> > > > Best regards
>> > > > Eli
>> > > >
>> > > > 天涯无处不重逢
>> > > > a leaf duckweed belongs to the sea, where not to meet in life
>> > > >
>> > > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
>> > > >
>> > > >
>> > > > On Wednesday, 29 March 2017 at 3:45 PM, Martin Kletzander wrote:
>> > > >
>> > > > > On Tue, Mar 28, 2017 at 03:22:34PM +0800, Eli Qiao wrote:


Look hoe much space it took ^^, and that's only once in there ;)

>> > > > > > hi Martin
>> > > > > >
>> > > > > > (cc libvir-list)
>> > > > > >
>> > > > > > I am a little confused about cat support.
>> > > > > >
>> > > > > > I am currently rebasing my code on top of pre-cat branch from your private github repo, today when I check it you have removed it and create a cat branch and there are some related code pushed[1], can I know what ’s your plan for my patch set for CAT support ? should I continue my rebasing work? your though?
>> > > > >
>> > > > > So we can work together on that. Since the rework of the sysfs
>> > > > > functions, some patches are easier to write from scratch then rewrite,
>> > > > > but I'm now just trying to setup the test suite, so that we have
>> > > > > something to test on, at least some of the code. So where are you in
>> > > > > the rebase right now? Do you think anything from the virsysfs.c code
>> > > > > could be enhanced?
>> > > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > > Not so fast, only the first patch [1], I found that nodeinfo.c is removed :(
>> > > >
>> > > > I think we need to extend virResCtrlGetInfoStr and virResCtrlGetInfoUint to virsysfs.c
>> > > >
>> > > > thought ?
>> > >
>> > > Yeah, we should wrap around /sys/fs/resctrl as we do with
>> > > /sys/devices/system so that it can be easily tested.
>> > >
>> >
>> > Sure, working on it, and done, will push it for review.
>> >
>> > Also I will push some fake data for resctrl testing..
>> >
>> >
>> > >
>> > > Also I got another idea about keeping the resource info. There is no
>> > > need for any global data to be stored as you are re-reading almost all
>> > > of it. The only info that stays the same is caches (that is already
>> > > saved in capabilities) and what caches are available for resource
>> > > control (that will be there as well). So I don't think we need yet
>> > > another global data storage.
>> > >
>> >
>> >
>> > Do you mean, we re-create all struct (reading them from /sys/fs/resctrl) when we create/destroy instance?
>> > also, for get free cache ?
>> >
>>
>>
>> You have to update that for every request anyway, so what's the point of
>> keeping the data when they immediately become old?
>>
>I was thought that may reduce the time costing, not all of the content be refreshed, anyway, I will try to avoid global files in my later version.
>

Well, if there is something that does not need refreshing, then you
might consider adding it to capabilities (if it is helpful to the user
in any way).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list