Re: [PATCH v2 7/7] tools: add tools support for Intel CAT

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> This is the xc/xl changes to support Intel Cache Allocation
> Technology(CAT). Two commands are introduced:
> - xl psr-cat-cbm-set [-s socket] <domain> <cbm>
>   Set cache capacity bitmasks(CBM) for a domain.
> - xl psr-cat-show <domain>
>   Show Cache Allocation Technology information.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxc/include/xenctrl.h |  15 +++
>  tools/libxc/xc_psr.c          |  74 ++++++++++++++

At the libxc level this looks like a reasonable wrapping of the
hypercall interface, so if the hypervisor folks are happy with that then
I'm happy with this.

>  tools/libxl/libxl.h           |  18 ++++

At the libxl level things seem to be rather opaque to me, i.e. what is
domain_data? what does it contain? does it have any more semantics than
a 64-bit number?

What future new values might there be for libxl_psr_cat_type?

>  tools/libxl/libxl_psr.c       | 107 ++++++++++++++++++--
>  tools/libxl/libxl_types.idl   |   4 +
>  tools/libxl/xl.h              |   4 +
>  tools/libxl/xl_cmdimpl.c      | 225 ++++++++++++++++++++++++++++++++++++++++--
>  tools/libxl/xl_cmdtable.c     |  13 +++

You've missed updating the manpage.

>  8 files changed, 445 insertions(+), 15 deletions(-)
> 

> +#ifdef LIBXL_HAVE_PSR_CAT
> +int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid,
> +                                  libxl_psr_cat_type type, uint32_t target,
> +                                  uint64_t data);
> +int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid,
> +                                  libxl_psr_cat_type type, uint32_t target,
> +                                  uint64_t *data_r);
> +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket,
> +                              uint32_t *cos_max_r, uint32_t *cbm_len_r);
> +#endif
> +


> [...]
> +static uint32_t get_phy_socket_num(void)

I think this would be better named "count_physical_sockets" or
something, otherwise I might think it takes a lock.

> +{
> +    int rc;
> +    uint32_t nr_sockets;

       uint32_t nr_sockets = 0;

> +    libxl_physinfo info;
> +    libxl_physinfo_init(&info);
> +    rc = libxl_get_physinfo(ctx, &info);

Then:
       if (!rc)
           nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
      
       libxl_physinfo_dispose(&info);
       return nr_sockets;

means you only have one return path to do clean up on.

Also, does this function need to be in an ifdef, since it isn't called
unless LIBXL_HAVE_... is set? (IOW: does this compile for ARM)


> @@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv)
>  }
>  #endif
>  
> +#ifdef LIBXL_HAVE_PSR_CAT
> +static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm)
> +{
> +    int rc;
> +    uint32_t i, nr_sockets;
> +
> +    if (socket != ALL_SOCKETS) {
> +        return libxl_psr_cat_set_domain_data(ctx, domid,
> +                                             LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                             socket, cbm);
> +    } else {
> +        nr_sockets = get_phy_socket_num();

I wonder if the libxl API ought to allow for an "all sockets" argument
and then do all this internally instead of making the callers do it?

> +        if (nr_sockets == 0) {
> +            fprintf(stderr, "Failed getting physinfo\n");
> +            return -1;
> +        }
> +        for (i = 0; i < nr_sockets; i++) {
> +            rc = libxl_psr_cat_set_domain_data(ctx, domid,
> +                                               LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                               i, cbm);
> +            if (rc < 0) {
> +                fprintf(stderr, "Failed to set l3 cbm for socket:%d\n", i);
> +            return -1;

Indentation.

> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +struct psr_cat_l3_info
> +{
> +    uint32_t cos_max;
> +    uint32_t cbm_len;
> +};

Is it every useful to retrieve these independently? Perhaps this struct
should be in the libxl API and be what is passed to
libxl_psr_cat_get_l3_info?

> +
> +static void psr_cat_print_domain_info(libxl_dominfo *dominfo,
> +                                      struct psr_cat_l3_info *l3_info,
> +                                      uint32_t nr_sockets)
> +{
> +    char *domain_name;
> +    uint32_t socketid;
> +    uint64_t cbm;
> +
> +    domain_name = libxl_domid_to_name(ctx, dominfo->domid);
> +    printf("%-40s %5d", domain_name, dominfo->domid);
> +    free(domain_name);
> +
> +    for (socketid = 0; socketid < nr_sockets; socketid++) {
> +        if (l3_info[socketid].cbm_len > 0 &&
> +            !libxl_psr_cat_get_domain_data(ctx, dominfo->domid,
> +                                           LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                           socketid, &cbm) )
> +            printf("%#16"PRIx64, cbm);

Print socket id too?

> +
> +    /* Header */
> +    printf("%-40s %5s", "Name", "ID");
> +    for (socketid = 0; socketid < nr_sockets; socketid++)
> +        printf("%14s %d", "Socket", socketid);
> +    printf("\n");
> +
> +    /* Total L3 cache size */
> +    printf("%-46s", "Total L3 Cache Size");

How wide are these lines going to be? Can we try and keep it to <80
columns? Perhaps you could include an example of the output of each of
the show functions in the commit message?

> +    for (socketid = 0; socketid < nr_sockets; socketid++) {
> +        rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
> +        if (rc < 0) {
> +            fprintf(stderr, "Failed to get system l3 cache size for socket:%d\n",
> +                            socketid);
> +            return -1;
> +        }
> +        printf("%13u KB", l3_cache_size);
> +    }
> +    printf("\n");
> +
> +    /* Max COS and CBM length */
> +    l3_info = malloc(sizeof(l3_info) * nr_sockets);
> +    //if (!l3_info)

Leftover.

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 22ab63b..ffaf4ed 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -542,6 +542,19 @@ struct cmd_spec cmd_table[] = {
>        "\"total_mem_bandwidth\":     Show total memory bandwidth(KB/s)\n"
>        "\"local_mem_bandwidth\":     Show local memory bandwidth(KB/s)\n",
>      },
> +    { "psr-cat-cbm-set",
> +      &main_psr_cat_cbm_set, 0, 1,
> +      "Set cache capacity bitmasks(CBM) for a domain",
> +      "-s <socket>       Specify the socket to process, all sockets when not"

"Specify the socket to process, otherwise all sockets are processed"

Ian.