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

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

On Tue, Apr 21, 2015 at 03:24:37AM +0200, Dario Faggioli wrote:
> 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.

Thanks for suggestion.
> 
> > --- 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?


The idea behind here is: All the places that appear as 'target' imply 
there are possible values other than just socket (e.g. considering L2
Cache Allocation in the future). So 'target' is always paired with a
'psr_cat_type' parameter. For routines that only work for L3 (e.g.
xc_psr_cat_get_l3_info) then 'socket' is used. I admit that it looks
inconsistent, perhaps rename all 'socket' to 'target'?

> 
> > --- 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?

I do like this suggestion and I have ever considered it actually. The
only thing prevents me is that we need an extra _get_socket_count in xl
for TARGET_ALL case. So libxl__count_physical_sockets is needed to be
public. If Ian/Wei have no concerns for this, then I'm glad to do this.

> 
> > --- 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.

Agreed, thanks.

Chao

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