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

Eli Qiao qiaoliyong at gmail.com
Tue Feb 7 06:43:17 UTC 2017



-- 
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 <liyong.qiao at intel.com (mailto:liyong.qiao at 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 at intel.com (mailto:liyong.qiao at 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.
> 
> 
> 


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/ :|
> 
> 
> 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170207/4ec5bb77/attachment-0001.htm>


More information about the libvir-list mailing list