-- Eli Qiao Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
On Tuesday, 7 February 2017 at 12:11 AM, Daniel P. Berrange wrote: > 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 <[email protected] > > (mailto:[email protected])> > > --- > > 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 <[email protected] (mailto:[email protected])> > > + */ > > +#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. > > > okay, thanks Daniel. > > > + 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 > okay > > + > > + 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. > okay. > > + } > > + 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. > sure > > + } > > + > > + 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 okay, will move to c 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. > okay > > +}; > > + > > + > > +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. > > > Only cache_banks will be change at runtime, such as cache_left on each socket will be updated if libvirt allocates cache to domains. > > > > +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. > > > Okay, I will fill all API docs for the public APIs > > > 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/ :| > > >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
