--
Best regards
Eli
天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life
Sent with Sparrow
On Friday, 17 February 2017 at 1:03 AM, Martin Kletzander wrote:
On Thu, Feb 16, 2017 at 05:35:12PM +0800, Eli Qiao wrote:This patch adds some utils struct and functions to expose resctrlinformation.One tiny nitpick, but it might actually help you as well. You can use-v7 parameter to git send-email or git format-patch to automatically add'v7' to the subject-prefix keeping the "PATCH" in there. Not only couldthis be easier for you, but people like me, who filter patches and othermails on the list to different folders, would have these properlycategorized.Anyway, for the review now.
Thx
virResCtrlAvailable: If resctrl interface exist on hostvirResCtrlGet: get specify type resource contral information"get specific resource control information" ???
Yep
virResCtrlInit: initialize resctrl struct from the host's sys fs.resctrlall[]: an array to maintain resource control information.Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx>---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/virresctrl.c | 343 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virresctrl.h | 80 +++++++++++7 files changed, 431 insertions(+)create mode 100644 src/util/virresctrl.ccreate mode 100644 src/util/virresctrl.hdiff --git a/src/util/virresctrl.c b/src/util/virresctrl.cnew file mode 100644index 0000000..80481bc--- /dev/null+++ b/src/util/virresctrl.c@@ -0,0 +1,343 @@+/*+ * virresctrl.c: methods for managing resource contral+ *+ * 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@xxxxxxxxx>+ */+#include <config.h>++#include <sys/ioctl.h>+#if defined HAVE_SYS_SYSCALL_H+# include <sys/syscall.h>What do you need syscall.h for?
Removed
+#endif+#include <sys/types.h>+#include <sys/stat.h>+#include <fcntl.h>++#include "virresctrl.h"+#include "viralloc.h"+#include "virerror.h"+#include "virfile.h"+#include "virhostcpu.h"+#include "virlog.h"+#include "virstring.h"+#include "nodeinfo.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"+If you need SYSFS_..._PATH for anything, it probably could be split intoother src/util/ files. Example below.+#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 virResCtrlGetCPUValue(const char *path, char **value)It would be more consistent if you reused parts of virHostCPUGetValue(),put them in a function, and use that one in both this one an theoriginal one. It chould be also put in the virhostcpu.c since that'sabout the host cpu.
Done
+static int virResctrlGetCPUSocketID(const size_t cpu, int *socket_id)No need for this function, just use virHostCPUParseSocket()
virHostCPUParseSocket is a static function, I created a new one.
+static int virResCtrlGetCPUCache(const size_t cpu, int type, int *cache)So we have some places in the code that get info from sysfs. Iunderstand that cache is controlled in the resctrl, but one doesn't haveto have resctrl to get some cache info, so I would move this functioninto virhostcpu.c and keep here only the stuff strictly related toresource control.
Done
+{+ int ret = -1;+ char *cache_dir = NULL;+ char *cache_str = NULL;+ char *tmp;+ int carry = -1;++ if (virAsprintf(&cache_dir,+ "%s/cpu/cpu%zu/cache/index%d/size", + SYSFS_SYSTEM_PATH, cpu, type) < 0)+ return -1;++ if (virResCtrlGetCPUValue(cache_dir, &cache_str) < 0) + goto cleanup;++ tmp = cache_str;++ while (*tmp != '\0') tmp++;++ if (*(tmp - 1) == 'K') {+ *(tmp - 1) = '\0';+ carry = 1;+ } else if (*(tmp - 1) == 'M') {+ *(tmp - 1) = '\0';+ carry = 1024;+ }++ if (virStrToLong_i(cache_str, NULL, 0, cache) < 0)+ goto cleanup;++ *cache = (*cache) * carry;++ if (*cache < 0)+ goto cleanup;++ ret = 0;+ cleanup:+ VIR_FREE(cache_dir);+ VIR_FREE(cache_str);+ return ret;+}+Why all this fuzz? You should instead pass pointer to virStrToLong_i toget where the number ends and then use virScaleInteger() to multiply itproperly.
Good to know, thx. done.
+/* Fill all cache bank informations */+static virResCacheBankPtr virResCtrlGetCacheBanks(int type, int *n_sockets)+{Still could be in virhostcpu.c
okay, will move it to there.
+ int npresent_cpus;+ int idx = -1;+ size_t i;+ virResCacheBankPtr bank;++ *n_sockets = 1;+ if ((npresent_cpus = virHostCPUGetCount()) < 0)+ return NULL;++ if (type == VIR_RDT_RESOURCE_L3+ || type == VIR_RDT_RESOURCE_L3DATA+ || type == VIR_RDT_RESOURCE_L3CODE)+ idx = 3;+ else if (type == VIR_RDT_RESOURCE_L2)+ idx = 2;++ if (idx == -1)+ return NULL;+Indentation, "||" should be on the previous line but, most importantly,why not switch? That'd make sure you won't miss any enum value and ifsomeone adds a new one, compilator will see it's missing here.
Done
+ if (VIR_ALLOC_N(bank, *n_sockets) < 0) {+ *n_sockets = 0;set this before the first return so that this function guaranteesn_sockets to be 0 on fail. Moreover, n_sockets is always set to 1here. Due to the way the rest of the function is designed, this doesn'thave to be here at all.
Done.
+ return NULL;+ }++ for (i = 0; i < npresent_cpus; i ++) {+ int s_id;+ int cache_size;++ if (virResctrlGetCPUSocketID(i, &s_id) < 0)+ goto error;++ if (s_id > (*n_sockets - 1)) {+ size_t cur = *n_sockets;+ size_t exp = s_id - (*n_sockets) + 1;+ if (VIR_EXPAND_N(bank, cur, exp) < 0)+ goto error;+ *n_sockets = 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 (virResCtrlGetCPUCache(i, idx, &cache_size) < 0)+ goto error;++ bank[s_id].cache_size = cache_size;+ bank[s_id].cache_min = cache_size / resctrlall[type].cbm_len;+ }+ }+ return bank;+Wouldn't it be easier to have list of virResCacheBankPtr *banks; andsize_t nbanks; and then just populate each pointer when that socket ison the system, so that you that NULL means the socket info was notfilled yet. Or just a list that isn't sparse (like yours is now). Thelogic here seems hard to read.
:( sorry, I don’t get you
Are you saying pre-allocate memory for virResCacheBankPtr? here nbanks are equal to sockets.
I can not know how many nbanks(sockets) on the host before virResCtrlGetCacheBanks.
can you show me some code examples?
I'll continue the review tomorrow.Martin+ error:+ *n_sockets = 0;+ VIR_FREE(bank);+ return NULL;+}++static int virResCtrlGetConfig(int type)+{+ int ret;+ size_t i;+ char *str;++ /* Read min_cbm_bits from resctrl.+ eg: /sys/fs/resctrl/info/L3/num_closids + */+ if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)+ return ret;++ if (virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid) < 0)+ return -1;++ 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)+ return ret;++ if (virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_bits) < 0) + return -1;++ 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)+ return ret;++ /* Calculate cbm length from the default cbm_mask. */+ resctrlall[type].cbm_len = strlen(str) * 4;+ VIR_FREE(str);++ /* Get all cache bank informations */+ resctrlall[type].cache_banks = virResCtrlGetCacheBanks(type,+ &(resctrlall[type].num_banks)); ++ if (resctrlall[type].cache_banks == NULL)+ return -1;++ 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;++ return ret;+}++int+virResCtrlInit(void)+{+ size_t i = 0;+ char *tmp;+ int rc = 0;++ for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {+ if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {+ VIR_ERROR(_("Failed to initialize resource control config"));+ return -1;+ }+ if (virFileExists(tmp)) {+ if ((rc = virResCtrlGetConfig(i)) < 0)+ VIR_ERROR(_("Failed to get resource control config"));+ return -1;+ }++ 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.hnew file mode 100644index 0000000..aa113f4--- /dev/null+++ b/src/util/virresctrl.h@@ -0,0 +1,80 @@+/*+ * * virresctrl.h: methods for managing rscctrl+ * *+ * * 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@xxxxxxxxx>+ */++#ifndef __VIR_RESCTRL_H__+# define __VIR_RESCTRL_H__++# include "virutil.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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list