[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]