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

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

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@xxxxxxxxxxxxxxx>
> > > 
> > > 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, IA32_QM_EVTSEL(Monitoring Event Select Register)
must be set first. This enum corresponds to the event type in IA32_QM_EVTSEL.

> 
> > > I'm not sure what I better term would be though. "count"?
> > 
> > The returned value is actually read from monitor event counter, so
> > "count" you suggested or "sample" both sound OK to me, e.g.:
> > XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
> > XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT
> 
> I was meaning XC_PSR_CMT_TOTAL_MEM_COUNT etc. not BANDWIDTH_COUNT (which
> I think doesn't make sense).

OK, I have no problem. Thanks for clarification.

Chao

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