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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2015-04-17 at 22:33 +0800, Chao Peng wrote:
> This is the xc/xl changes to support Intel Cache Allocation
> Technology(CAT). Two commands are introduced:
> - xl psr-cat-hwinfo
>   Show CAT hardware information.

> Examples:
> [root@vmm-psr vmm]# xl psr-cat-hwinfo
> Cache Allocation Technology (CAT):
> Socket ID       : 0
> L3 Cache        : 12288KB
> Maximum COS     : 15
> CBM length      : 12
> Default CBM     : 0xfff
> 
Or, you can rename the psr-cmt-hwinfo command, added in the previous
patch, to 'psr-hwinfo' and make it accept options, e.g.,

 "-m, --cmt   show Cache Monitoring Technology (CMT) hardware info"
 "-c, --cat   show Cache Allocation Technology (CAT) hardware info"

By default (i.e., no options provided), it can just print all the hw
info.

Not a big deal, but I think that would make a better command line
interface. Tools' maintainers' call, I guess.

> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h

> +
> +int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
> +                               xc_psr_cat_type type, uint32_t target,
> +                               uint64_t data);
> +int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> +                               xc_psr_cat_type type, uint32_t target,
> +                               uint64_t *data);
>
So, for this twos, 'target' is the socket you want to act on.

> +int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
> +                           uint32_t *cos_max, uint32_t *cbm_len);
>
While here you use 'socket', to mean the same thing.

That looks rather inconsistent. Since it's a socket we are talking
about, why not 'socket' everywhere?

> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
 
> +#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);
> +
The same applies here: I'd rename taget to socket.

Then there's that LIBXL_PSR_TARGET_ALL. So, target (or socket, if you'll
rename it) is an integer representing _which_one_ socket to act on.
However, there is a special value to mean "all sockets".

Another possibility would be to offer an API that "natively" allows for
operating on multiple sockets, by using libxl_bitmap-s, as it happens in
many other places, in libxl itself (e.g., setting and getting vcpu
affinity).

That means target would become a libxl_bitmap, and, in the
implementation, you'll apply the operation on all the sockets
corresponding to a set bit in there. Only one bit set means just that
socket, all bits means all sockets.

This looks like a better interface to me (no need for special ~0U
values), it'd make the implementation more linear, and is more
consistent with how other similar situations are handled in libxl.
However, I appreciate that one may find it overkill... I guess it
depends whether we expect the prevalent usage pattern to be almost
always about single sockets --and maybe on all sockets, from time to
time-- or if we see value in being able to specify more than one and
less than all sockets.

For instance, now that we have vNUMA, if a domain has 4 vNUMA nodes,
each one mapped to a physical NUMA node, it looks to me like it would
make sense to set CAT to, say, 0x0F, on the sockets corresponding to the
physical NODEs. With the interface in this patch, that would require
calling libxl_psr_cat_set_cbm() 4 times, with the libxl_bitmap approach,
just once, after setting up the bitmap properly.

Thoughts?

> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c

> +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
> +                              uint32_t *nr)
> +{
> +    GC_INIT(ctx);
> +    int rc, r;
> +    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");
> +        goto out;
> +    }
> +
> +    ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_cat_info));
> +
> +    for (i = 0; i < nr_sockets; i++) {
> +        r = xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
> +                                                &ptr[i].cbm_len);
> +        if (r < 0) {
>
why not just:

           if (xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
                                      &ptr[i].cbm_len)) {

> +            libxl__psr_cat_log_err_msg(gc, errno);
> +            rc = ERROR_FAIL;
> +            free(ptr);
> +            goto out;
> +        }
> +    }
> +
I mean, you don't need to save the actual return value from libxc, do
you? The same applies to other places down in the patch.

Regards,
Dario

Attachment: signature.asc
Description: This is a digitally signed message part

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