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

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

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.

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

> 
> > @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
> >      entries[nr].rsvd = 0;
> >      nr++;
> >  
> > +    if ( tsc != NULL )
> > +    {
> > +        tsc_entry = &entries[nr];
> > +        entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > +        entries[nr].idx = MSR_IA32_TSC;
> > +        entries[nr].val = 0;
> > +        entries[nr].rsvd = 0;
> > +        nr++;
> > +    }
> 
> Perhaps consider an assertion here that nr <= the number of elements in
> the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
> which switched to using nr++)

Agreed. Given that patch (switched to using nr++) is aleady in, so will
add it here.

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