Re: [libvirt] [resend v2 1/7] Resctrl: Add some utils functions

On Mon, Feb 06, 2017 at 10:23:36AM +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 specify type resource contral 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 + > src/Makefile.am (http://Makefile.am) | 1 + > src/libvirt_private.syms | 4 + > src/util/virerror.c | 1 + > src/util/virresctrl.c | 330 ++++++++++++++++++++++++++++++++++++++++++++ > src/util/virresctrl.h | 89 ++++++++++++ > 6 files changed, 426 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..63bc808 > --- /dev/null > +++ b/src/util/virresctrl.c > @@ -0,0 +1,330 @@ > +/* > + * 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> > +#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 > + > +static unsigned int host_id = 0; > + > +static virResCtrl ResCtrlAll[] = { > Lowercase for global variable names please.

> + { > + .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 (asprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[type].name, item) < 0) > + return -1; > Use of asprintf is forbidden in libvirt - use virAsprintf. Please make sure to run 'make check' and 'make syntax-check' on each patch to catch this kind of policy error. There's quite a few other style rules missed in these patches - i won't list them all since 'make syntax-check' can tell you.

> + 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) > +{ > + int ret = -1; > + char* tmp; > The '*' should be next to the variable name, not the type name. Multiple other cases follow

> + > + if(virFileReadAll(path, 10, value) < 0) { > + goto cleanup; > + } > + if ((tmp = strchr(*value, '\n'))) { > + *tmp = '\0'; > + } > + ret = 0; > +cleanup: > + return ret; > +} > + > > +int virResCtrlInit(void) { > + int i = 0; > + char *tmp; > + int rc = 0; > + > + for(i = 0; i < RDT_NUM_RESOURCES; i++) { > + if ((rc = asprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[i].name)) < 0) { > + continue; > Silently ignoring OOM is not good - please return a proper error - using virAsprintf would do so.

> + } > + if (virFileExists(tmp)) { > + if ((rc = virResCtrlGetConfig(i)) < 0 ) > + VIR_WARN("Ignor error while get config for %d", i); > Again don't ignore errors like this - this should be fatal.

> + } > + > + VIR_FREE(tmp); > + } > + return rc; > +} > + > +bool virResCtrlAvailable(void) { > + if (!virFileExists(RESCTRL_INFO_DIR)) > + return false; > + return true; > +} > + > +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..f713e66 > --- /dev/null > +++ b/src/util/virresctrl.h > @@ -0,0 +1,89 @@ > > + > +#ifndef __VIR_RESCTRL_H__ > +# define __VIR_RESCTRL_H__ > + > +# include "virutil.h" > +# include "virbitmap.h" > + > +#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) > Doesn't seem like any of this needs to be in the header file

> + > +enum { > + RDT_RESOURCE_L3, > + RDT_RESOURCE_L3DATA, > + RDT_RESOURCE_L3CODE, > + RDT_RESOURCE_L2, > + /* Must be the last */ > + RDT_NUM_RESOURCES, > Use a VIR_ prefix on these constants. Also the libvirt naming convention is to use RDT_RESOURCE_LAST as the last element.

> +}; > + > + > +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; > +}; > > +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; > +}; > Can any of these fields change at runtime, or are they all immutable once populated.

> +bool virResCtrlAvailable(void); > +int virResCtrlInit(void); > +virResCtrlPtr virResCtrlGet(int); > API docs for these would be nice, especially describing whether virResCtrlPtr returned from this method is immutable or not. Since libvirt is multi-threaded this is important to know.

Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|