-- 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, 17 February 2017 at 8:47 PM, Martin Kletzander wrote: > 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.q...@intel.com > > > (mailto:liyong.q...@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.q...@intel.com (mailto:liyong.q...@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. Done > > > > +{ > > > + 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 ^^ Done > > > > + > > > + 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 Done > > > > + > > > + 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? Not really, here ’s tricky. only host id for VIR_RDT_RESOURCE_L3CODE set to VIR_RDT_RESOURCE_L3DATA > > > > + } > > > + > > > + 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. > Done. > > > + return -1; > > > > > tmp leaks here > Done. > > > + } > > > + > > > + 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. > I tried to move accessors to .c file, but failed, I can not even reference it outside. > > > +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. Done. > > > > + * 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.q...@intel.com (mailto:liyong.q...@intel.com)> > > > + */ > > > + > > > +#ifndef __VIR_RESCTRL_H__ > > > +# define __VIR_RESCTRL_H__ > > > + > > > +# include "virutil.h" > > > > > > > > What do you need virutil in the header file for? > Removed > > > +# 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