[libvirt] [v7 01/10] Resctrl: Add some utils functions

[Eli Qiao]

--  
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 1:03 AM, 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.

Thx  
>  
> > virResCtrlAvailable: If resctrl interface exist on host
> > virResCtrlGet: get specify type resource contral information
> >  
>  
>  
> "get specific resource control information" ???

Yep  
>  
> > 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 +
> > 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.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>
> >  
>  
>  
> What do you need syscall.h for?
Removed
  
>  
> > +#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.
>  
Done  
> > +static int virResctrlGetCPUSocketID(const size_t cpu, int *socket_id)
>  
>  
> No need for this function, just use virHostCPUParseSocket()
>  
virHostCPUParseSocket is a static function, I created a new one.  
> > +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.
>  
>  
>  

Done  
>  
> > +{
> > + 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.
>  
Good to know, thx. done.  
> > +/* Fill all cache bank informations */
> > +static virResCacheBankPtr virResCtrlGetCacheBanks(int type, int *n_sockets)
> > +{
> >  
>  
>  
> Still could be in virhostcpu.c

okay, will move it to there.  
>  
> > + 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.
>  
Done  
> > + 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.
>  
Done.  
> > + 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.
>  
:( sorry, I don’t get you  
  
 Are you saying pre-allocate memory for virResCacheBankPtr?  here nbanks are equal to sockets.

I can not know how many nbanks(sockets) on the host before virResCtrlGetCacheBanks.

can you show me some code examples?

> I'll continue the review tomorrow.
>  
> Martin
>  
> > + error:
> > + *n_sockets = 0;
> > + VIR_FREE(bank);
> > + return NULL;
> > +}
> > +
> > +static int virResCtrlGetConfig(int type)
> > +{
> > + 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;
> > +
> > + 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;
> > +
> > + 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. */
> > + 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++;
> > + }
> > +
> > + 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"));
> > + return -1;
> > + }
> > +
> > + 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
> > + */
> > +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.
> > + *
> > + * 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)>
> > + */
> > +
> > +#ifndef __VIR_RESCTRL_H__
> > +# define __VIR_RESCTRL_H__
> > +
> > +# include "virutil.h"
> > +# 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
> >  
>  
>  
>  
>  


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