From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v10 4/4] tools,
 docs: add total/local memory bandwith monitoring
Date: Mon, 2 Mar 2015 13:48:43 +0000
Message-ID: <1425304123.21151.26.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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1424940339-11455-5-git-send-email-chao.p.peng@linux.intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Chao Peng <chao.p.peng@linux.intel.com>
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 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'm not sure what I better term would be though. "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++)

Ian.