From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring Date: Thu, 5 Mar 2015 08:47:19 +0800 Message-ID: <20150305004719.GF15262@pengc-linux.bj.intel.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> <1425463676.25940.108.camel@citrix.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1425463676.25940.108.camel@citrix.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: Ian Campbell Cc: wei.liu2@citrix.com, will.auld@intel.com, stefano.stabellini@eu.citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > > > > > > > > > > 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