Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Chao Peng <chao.p.peng@linux.intel.com>]

On Wed, Mar 04, 2015 at 10:07:56AM +0000, Ian Campbell wrote:
> On Wed, 2015-03-04 at 09:04 +0800, Chao Peng wrote:
> > On Tue, Mar 03, 2015 at 10:09:39AM +0000, Ian Campbell wrote:
> > > On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > > > On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> > > > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > > > CMT should be enabled in hypervisor.
> > > > > > 
> > > > > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > > > 
> > > > > This looks good. I have one question and one small comment/idea:
> > > > > 
> > > > > [...]
> > > > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > > > > index 09d819f..54043ee 100644
> > > > > > --- a/tools/libxc/include/xenctrl.h
> > > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> > > > > >  #if defined(__i386__) || defined(__x86_64__)
> > > > > >  enum xc_psr_cmt_type {
> > > > > >      XC_PSR_CMT_L3_OCCUPANCY,
> > > > > > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > > > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > > > 
> > > > > Is "bandwidth" still the correct term here (and more importantly in the
> > > > > libxl interface e.g. enum), given that we now do the sampling at the
> > > > > application level and just expose the current count from Xen via libxl?
> > > > 
> > > > I feel comfortable either changing it or not. The reason to change it is
> > > > what you said here that we do return the counter value to the caller, so
> > > > a consistent name would be nice. While the reason to keep it is: the
> > > > names are listed as the "monitoring event type" from spec, so the caller
> > > > perhaps knows that the returned data is the sample value from event
> > > > counter register related to that type.
> > > > 
> > > > Anyway, if you feel it's better to change, then I will do.
> > > 
> > > What names does Intel's documentation use for these registers?
> > 
> > The register is IA32_QM_CTR(Monitoring Counter Register), to read the
> > count in this register,
> 
> I'm thinking then that COUNT is the right name, .e.g.
> XC_PSR_CMT_TOTAL_MEM_COUNT (and for libxl names too). Does that sound
> alright?

Yes. See my changes sent in another thread.
Chao