Re: [PATCH v9 3/3] tools, docs: add total/local memory bandwith monitoring

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

On Wed, 2015-02-25 at 18:21 +0800, Chao Peng wrote:
> On Mon, Feb 23, 2015 at 04:24:32PM +0000, Ian Campbell wrote:
> > On Fri, 2015-02-13 at 13:09 +0000, Wei Liu wrote:
> > > On Mon, Feb 02, 2015 at 04:06:09PM +0800, Chao Peng wrote: 
> > > > +#ifdef LIBXL_HAVE_PSR_MBM
> > > > +int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
> > > > +int libxl_psr_cmt_get_mem_bandwidth_sample(libxl_ctx *ctx,
> > > > +                                           uint32_t domid,
> > > > +                                           uint32_t socketid,
> > > > +                                           bool     total,
> > > > +                                           uint64_t *sample_r,
> > > > +                                           uint64_t *tsc_r);
> > > 
> > > Should the interface be only limited to socket level? Do you (Intel) has
> > > plan to provide bandwidth sampling on finer grain level (say, core
> > > level)?
> > 
> > If we assume not then obviously some h/w designer will implement it ;-)
> > 
> > The existing libxl_psr_cmt_get_l3_cache_size has uint32_t socketid
> > though, so we may as well do the same here and fixup everything at once
> > if it ever happens.
> 
> I didn't see any implementation for core-level in the near future from
> Intel side but perhaps it's good idea to consider it, at least for long
> time. Define the following types achieve this purpose?
> 
> libxl_psr_scope_type = Enumeration("psr_scope_type", [
>     (1, "SOCKET"),
>     ])
> 
> libxl_psr_scope = Struct("psr_scope", [
>     ("u", KeyedUnion(None, libxl_psr_scope_type, "type",
>             ("socket",  integer),
>            ])),
> ], dir=DIR_OUT)

That's probably overkill. Simply expanding the socketid parameter to
encode socket/cores (perhaps widening the type) would suffice, wouldn't
it?

All that can wait until there is an actual sub-socket feature to
actually support.
> 
> > 
> > > I also assume that you will never have third type of memory bandwidth?
> > > I.e. do we need to change boolean total to a enum?
> > 
> > There is already an enum, but some of the values it has don't really fit
> > this interface (the "l3_cache_size" isn't really a stat which needs
> > sampling), cf my wonder in response to v8 "if wedging all of these
> > logically unrelated statistics into an umbrella psr interface at the
> > libxl level makes sense."
> > 
> > Perhaps just call this interface psr_cmt_get_sample, pass it the enum
> > and always return a tsc? Then other types of stats sampling would fit in
> > the future, hopefully.
> 
> For l3_cache_size the tsc will not actually be returned if tsc is NULL.

But it will be if tsc != NULL, right?

> For the time being, the interface sounds enough for both cache occupancy
> and memory bandwidth.
> 
> The question is that if I adopt this solution will I remove the original
> dedicated l3_cache_size interface? Most concern is api compatibility but
> I think it has never been used outside of xl.

We can leave the old interface in place (perhaps as a wrapper around the
new one) as a deprecated one for compatibility, that's fine.

If we are really sure it is unused (i.e. libvirt never used it) then we
could consider removing it, there's an extent to which ABI compatibility
is a bit like a tree falling in the woods...

Ian.