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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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?

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

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

Thanks.



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