Re: [libvirt] [v7 01/10] Resctrl: Add some utils functions

On Thu, Feb 16, 2017 at 06:03:50PM +0100, 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 resctrl > > information. > > > > > 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 could > this be easier for you, but people like me, who filter patches and other > mails on the list to different folders, would have these properly > categorized. > > Anyway, for the review now. > > > virResCtrlAvailable: If resctrl interface exist on host > > virResCtrlGet: get specify type resource contral information > > > > > "get specific resource control information" ??? > > > 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(a)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/virresctrl.c | 343 ++++++++++++++++++++++++++++++++++++++++++++ > > src/util/virresctrl.h | 80 +++++++++++ > > 7 files changed, 431 insertions(+) > > create mode 100644 src/util/virresctrl.c > > create mode 100644 src/util/virresctrl.h > > > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c > > new file mode 100644 > > index 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(a)intel.com (mailto:liyong.qiao@intel.com)> > > + */ > > +#include <config.h> > > + > > +#include <sys/ioctl.h> > > +#if defined HAVE_SYS_SYSCALL_H > > +# include <sys/syscall.h> > > > > > What do you need syscall.h for? > > > +#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 into > other 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 the > original one. It chould be also put in the virhostcpu.c since that's > about the host cpu. > > > +static int virResctrlGetCPUSocketID(const size_t cpu, int *socket_id) > > No need for this function, just use virHostCPUParseSocket() > > > +static int virResCtrlGetCPUCache(const size_t cpu, int type, int *cache) > > So we have some places in the code that get info from sysfs. I > understand that cache is controlled in the resctrl, but one doesn't have > to have resctrl to get some cache info, so I would move this function > into virhostcpu.c and keep here only the stuff strictly related to > resource control. > > > +{ > > + 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 to > get where the number ends and then use virScaleInteger() to multiply it > properly. > > > +/* Fill all cache bank informations */ > > +static virResCacheBankPtr virResCtrlGetCacheBanks(int type, int *n_sockets) > > +{ > > > > > Still could be in virhostcpu.c > > > + 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 if > someone adds a new one, compilator will see it's missing here. > > > + if (VIR_ALLOC_N(bank, *n_sockets) < 0) { > > + *n_sockets = 0; > > > > > set this before the first return so that this function guarantees > n_sockets to be 0 on fail. Moreover, n_sockets is always set to 1 > here. Due to the way the rest of the function is designed, this doesn't > have to be here at all. > > > + 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; and > size_t nbanks; and then just populate each pointer when that socket is > on the system, so that you that NULL means the socket info was not > filled yet. Or just a list that isn't sparse (like yours is now). The > logic here seems hard to read. > > I'll continue the review tomorrow. > > Martin > > > + error: > > + *n_sockets = 0; > > + VIR_FREE(bank); > > + return NULL; > > +} > > + > > +static int virResCtrlGetConfig(int type) > > > > I feel like 'Get' implies you're returning it, I'd go with 'Read' instead.

> > +{ > > + 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; > > > You leak str here ^^

> > + > > + 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; > > > Same here

> > + > > + 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. */ > > > The comment could say the mask is in hex and that's why strlen(s) * 4 is OK. Question: It can never be, for example, 38 bits or just 2? Just asking to be sure. > > + 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++; > > > Shouldn't this be done only if CDP is not supported?

> > + } > > + > > + 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")); > > > virReportError should be used here, VIR_ERROR has a specific use case that's not applicable here.

> > + return -1; > tmp leaks here

> > + } > > + > > + 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 > > + */ > > > That's easy to achieve. If you only put the typedef into the header file and accessors into this file, then no other code will be able to modify the data because the pointer will be opaque. Even if it is modifiable, that should be the way to go.

> > +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..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. > > + * > > > Too many asterisks.

> > + * 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(a)intel.com (mailto:liyong.qiao@intel.com)> > > + */ > > + > > +#ifndef __VIR_RESCTRL_H__ > > +# define __VIR_RESCTRL_H__ > > + > > +# include "virutil.h" > > > What do you need virutil in the header file for?

> > +# 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(a)redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list >