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

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

On Tue, 2015-04-21 at 17:49 +0800, Chao Peng wrote:
> 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.

FWIW I think this is a good idea.


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

I don't think you need libxl__count_physical_sockets for this, the
existing xl code for similar things just uses libxl_bitmap_set_any to
handle the all case.

With that in mind Dario's suggestion does seem like a good improvement
to the interface.

Ian.