Re: [Xen-devel] [PATCH v4 11/12] tools: add tools support for Intel CAT

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-04-09 at 17:18 +0800, Chao Peng wrote:

> +=item B<psr-cat-cbm-set> [I<OPTIONS>] [I<domain-id>] [I<cbm>]
> +
> +Set cache capacity bitmasks(CBM) for a domain.

I can see from the example in the commit log that I<cbm> is a number,
but a) I can't tell that from these docs and b) I have no idea what that
number actually does based on these docs.

Also, are I<domain-id> and I<cmb> really optional to this command as
implied by the surrounding []'s

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5eec092..3800738 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -718,6 +718,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   * If this is defined, the Memory Bandwidth Monitoring feature is supported.
>   */
>  #define LIBXL_HAVE_PSR_MBM 1
> +
> +/*
> + * LIBXL_HAVE_PSR_CAT
> + *
> + * If this is defined, the Cache Allocation Technology feature is supported.
> + */
> +#define LIBXL_HAVE_PSR_CAT 1
>  #endif
>  
>  typedef char **libxl_string_list;
> @@ -1513,6 +1520,25 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
>                               uint64_t *tsc_r);
>  #endif
>  
> +#ifdef LIBXL_HAVE_PSR_CAT
> +
> +#define LIBXL_PSR_TARGET_ALL (~0U)
> +int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
> +                          libxl_psr_cbm_type type, uint32_t target,
> +                          uint64_t cbm);
> +int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
> +                          libxl_psr_cbm_type type, uint32_t target,
> +                          uint64_t *cbm_r);
> +
> +/*
> + * On success, the function returns an array of elements in 'info',
> + * and the length in 'nr'. 

> 'info' is from malloc so it must be freed
> + * by the caller.

This is true of all libxl interfaces and is documented generically near
the top, so no need to repeat it here.

It's also strictly speaking required to call libxl_psr_cat_info_dispose
on each list element. We usually provide a libxl_psr_cat_info_list_free
helper which does all of the disposes and then frees the containing
array. See libxl_vcpuinfo_list_free (and a bunch of others near it in
libxl_utils.c) for example.

> + */
> +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
> +                              uint32_t *nr);


> @@ -247,6 +308,99 @@ out:
>      return rc;
>  }
>  
> +int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
> +                          libxl_psr_cbm_type type, uint32_t target,
> +                          uint64_t cbm)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +    uint32_t i, nr_sockets;
> +
> +    if (target != LIBXL_PSR_TARGET_ALL) {
> +        rc = xc_psr_cat_set_domain_data(ctx->xch, domid, type, target, cbm);

Per CODING_STYLE a variable called rc must only ever contain a libxl
error code, which xc_* doesn't return. You should use a second variable
called r or some such.

> +        if (rc < 0) {
> +            libxl__psr_cat_log_err_msg(gc, errno);
> +            rc = ERROR_FAIL;
> +        }
> +    } else {
> +        rc = libxl__count_physical_sockets(gc, &nr_sockets);
> +        if (rc) {
> +            LOGE(ERROR, "failed to get system socket count");
> +            rc = ERROR_FAIL;

In this case rc should already (correctly) contain a libxl error code
from the call to libxl__count_physical_sockets, which should be
propagated.

> +            goto out;
> +        }
> +        for (i = 0; i < nr_sockets; i++) {
> +            rc = xc_psr_cat_set_domain_data(ctx->xch, domid, type, i, cbm);

But here again you should use a variable other than rc.

> +            if (rc < 0) {
> +                libxl__psr_cat_log_err_msg(gc, errno);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
> +                          libxl_psr_cbm_type type, uint32_t target,
> +                          uint64_t *cbm_r)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +
> +    rc = xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r);

Don't use rc here.

> +    if (rc < 0) {
> +        libxl__psr_cat_log_err_msg(gc, errno);
> +        rc = ERROR_FAIL;
> +    }
> +
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
> +                              uint32_t *nr)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +    uint32_t i, nr_sockets;
> +    libxl_psr_cat_info *ptr;
> +
> +    rc = libxl__count_physical_sockets(gc, &nr_sockets);
> +    if (rc) {
> +        LOGE(ERROR, "failed to get system socket count");
> +        rc = ERROR_FAIL;

Don't overwrite rc here.

> +        goto out;
> +    }
> +
> +    ptr = malloc(nr_sockets * sizeof(libxl_psr_cat_info));
> +    if (!ptr) {

As Wei says use libxl__malloc with NOGC and then you don't need to check
for failure.

> +        LOGE(ERROR, "failed to allocate cat info");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < nr_sockets; i++) {
> +        rc = xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
> +                                                 &ptr[i].cbm_len);

Don't use rc here.

> +        if (rc) {
> +            libxl__psr_cat_log_err_msg(gc, errno);
> +            rc = ERROR_FAIL;
> +            free(ptr);
> +            goto out;
> +        }
> +    }
> +
> +    *info = ptr;
> +    *nr = nr_sockets;
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 04faf98..0a5f436 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> +
> +    printf("\n");
> +}
> +
> +static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socket)
> +{
> +    int i, nr_domains;
> +    libxl_dominfo *list;
> +
> +    if (domid != INVALID_DOMID) {
> +        psr_cat_print_one_domain_cbm(domid, socket);
> +        return 0;
> +    }
> +
> +    if (!(list = libxl_list_domain(ctx, &nr_domains))) {
> +        fprintf(stderr, "Failed to get domain list for cbm display\n");
> +        return -1;
> +    }
> +
> +    for (i = 0; i < nr_domains; i++)
> +        psr_cat_print_one_domain_cbm(list[i].domid, socket);
> +    libxl_dominfo_list_free(list, nr_domains);
> +
> +    printf("\n");

Since psr_cat_print_one_domain_cbm ends with this too do you not end up
with an extra blank line?

> +static int psr_cat_show(uint32_t domid)
> +{
> +    uint32_t socket, nr_sockets;
> +    int rc;
> +    libxl_psr_cat_info *info;
> +
> +    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
> +    if (rc) {
> +        fprintf(stderr, "Failed to get cat info\n");
> +        return rc;
> +    }
> +
> +    for (socket = 0; socket < nr_sockets; socket++) {
> +        rc = psr_cat_print_socket(domid, socket, info + socket);
> +        if (rc)
> +            goto out;
> +    }
> +
> +out:
> +    free(info);

Should use (to be added)  libxl_psr_cat_info_list_free.


> +    if (strlen(ptr) > 2 && ptr[0] == '0' && ptr[1] == 'x')
> +        cbm = strtoll(ptr, NULL , 16);
> +    else
> +        cbm = strtoll(ptr, NULL , 10);

Passing 0 as the base (third) parameter to strtoll does the right thing
wrt 0x prefixes etc. No need to replicate manually.

A bunch of pretty minor comments but it is mostly looking good, thanks.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel