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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3985 bytes --]

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.
> > 
> > > --- 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. 
>
Mmm... I understand your concerns. So, sticking to the future L2 CAT
support example, what would 'target' mean in that case, a pCPU? I'll
have to be something that makes it possible to 'identify' an L2, as much
as socket identifies an L3... Is this the case?

I'm not sure. My own concerns are that, if I look at the prototypes of
this functions, it's not that evident what values should I use for the
type and target parameters, whether there are constraints/relationships
among them, etc. That applies to both the xc_* functions above and the
libxl_* functions below, IMO.

Libxc is not a stable interface, so we could even just forget about
this, design this very interface _only_ for what we have now and deal
with different CBM types when we'll be introducing them. However, libxl
*does* have a stable API, so we still need to solve the issue at that
level.

> 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'?
> 
No, IMO, that is one "good inconsistency", as it allows, at least for
that function, to easily figure out what one should pass to the function
by means of that parameter! :-)

I really am not sure, and probably would have to know in what way(s)
'target' would change its meaning, depending on the value of 'type' (as
asked above)... Probably what I'd do is leave parameters names as they
are, but write a few doc-comments to explain how to use them, especially
at the libxl level (libxc is a lot less critical, from this respect, I
think).

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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