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

Eli Qiao qiaoliyong at gmail.com
Wed Feb 22 02:02:03 UTC 2017



--  
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 8:47 PM, Martin Kletzander wrote:

> On Thu, Feb 16, 2017 at 06:03:50PM +0100, 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.
> >  
> > > virResCtrlAvailable: If resctrl interface exist on host
> > > virResCtrlGet: get specify type resource contral information
> > >  
> >  
> >  
> > "get specific resource control 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 +
> > > 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?
> >  
> > > +#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.
> >  
> > > +static int virResctrlGetCPUSocketID(const size_t cpu, int *socket_id)
> >  
> > No need for this function, just use virHostCPUParseSocket()
> >  
> > > +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.
> >  
> > > +{
> > > + 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.
> >  
> > > +/* Fill all cache bank informations */
> > > +static virResCacheBankPtr virResCtrlGetCacheBanks(int type, int *n_sockets)
> > > +{
> > >  
> >  
> >  
> > Still could be in virhostcpu.c
> >  
> > > + 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.
> >  
> > > + 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.
> >  
> > > + 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.
> >  
> > I'll continue the review tomorrow.
> >  
> > Martin
> >  
> > > + error:
> > > + *n_sockets = 0;
> > > + VIR_FREE(bank);
> > > + return NULL;
> > > +}
> > > +
> > > +static int virResCtrlGetConfig(int type)
> > >  
> >  
> >  
>  
>  
> I feel like 'Get' implies you're returning it, I'd go with 'Read' instead.

Done
>  
> > > +{
> > > + 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;
> > >  
> >  
>  
>  
> You leak str here ^^
Done  
>  
> > > +
> > > + 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;
> > >  
> >  
>  
>  
> Same here
Done  
>  
> > > +
> > > + 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. */
> > >  
> >  
>  
>  
> The comment could say the mask is in hex and that's why strlen(s) * 4 is
> OK.
>  
> Question: It can never be, for example, 38 bits or just 2? Just asking
> to be sure.
>  
> > > + 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++;
> > >  
> >  
>  
>  
> Shouldn't this be done only if CDP is not supported?

Not really, here ’s tricky.

only host id for VIR_RDT_RESOURCE_L3CODE set to VIR_RDT_RESOURCE_L3DATA


  
>  
> > > + }
> > > +
> > > + 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"));
> > >  
> >  
>  
>  
> virReportError should be used here, VIR_ERROR has a specific use case
> that's not applicable here.
>  
Done.  
> > > + return -1;
> >  
>  
>  
> tmp leaks here
>  
Done.  
> > > + }
> > > +
> > > + 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
> > > + */
> > >  
> >  
>  
>  
> That's easy to achieve. If you only put the typedef into the header
> file and accessors into this file, then no other code will be able to
> modify the data because the pointer will be opaque. Even if it is
> modifiable, that should be the way to go.
>  
I tried to move accessors to .c file, but failed, I can not even reference it outside.  
> > > +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.
> > > + *
> > >  
> >  
>  
>  
> Too many asterisks.
Done.  
>  
> > > + * 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"
> > >  
> >  
>  
>  
> What do you need virutil in the header file for?
>  
Removed  
> > > +# 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
> > >  
> >  
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com (mailto:libvir-list at redhat.com)
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >  
>  
>  
>  
>  


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


More information about the libvir-list mailing list