From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring Date: Wed, 4 Mar 2015 10:07:56 +0000 Message-ID: <1425463676.25940.108.camel@citrix.com> References: <1424940339-11455-1-git-send-email-chao.p.peng@linux.intel.com> <1424940339-11455-5-git-send-email-chao.p.peng@linux.intel.com> <1425304123.21151.26.camel@citrix.com> <20150303080050.GD15262@pengc-linux.bj.intel.com> <1425377379.24959.59.camel@citrix.com> <20150304010407.GE15262@pengc-linux.bj.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150304010407.GE15262@pengc-linux.bj.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chao Peng Cc: Ian.Jackson@eu.citrix.com, will.auld@intel.com, xen-devel@lists.xen.org, wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 > > > > > > > > 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? Ian.