[libvirt] [resend v2 1/7] Resctrl: Add some utils functions
Daniel P. Berrange
berrange at redhat.com
Mon Feb 6 16:11:03 UTC 2017
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>
> ---
> include/libvirt/virterror.h | 1 +
> src/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>
> + */
> +#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/ :|
More information about the libvir-list
mailing list