Re: [Xen-devel] [PATCH v3 5/5] tools: add total/local memory bandwith monitoring

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jan 13, 2015 at 04:02:13PM +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>
> ---
>  docs/man/xl.pod.1             |    9 +++++
>  tools/libxc/include/xenctrl.h |    2 +
>  tools/libxc/xc_psr.c          |    8 ++++
>  tools/libxl/libxl.h           |    8 ++++
>  tools/libxl/libxl_psr.c       |   84 
> +++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl   |    2 +
>  tools/libxl/xl_cmdimpl.c      |   21 ++++++++++-
>  tools/libxl/xl_cmdtable.c     |    4 +-
>  8 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 6b89ba8..0370625 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1461,6 +1461,13 @@ is domain level. To monitor a specific domain, just 
> attach the domain id with
>  the monitoring service. When the domain doesn't need to be monitored any 
> more,
>  detach the domain id from the monitoring service.
>  
> +Intel Broadwell and later server platforms also offer total/local memory
> +bandwidth monitoring. Xen supports per-domain monitoring for these two
> +additional monitoring types. Both memory bandwidth monitoring and L3 cache
> +occupancy monitoring share the same set of underground monitoring service. 
> Once
                                              ^^^^^^^^^^^
                                              underlying?

I'm not native speaker though. I will defer reviewing this paragraph to
a native English speaker.

> +a domain is attached to the monitoring service, monitoring data can be showed
> +for any of these monitoring types.
> +
>  =over 4
>  
[...]
> +static int libxl__psr_cmt_get_mem_bandwidth(libxl__gc *gc,
> +                                            uint32_t domid,
> +                                            xc_psr_cmt_type type,
> +                                            uint32_t socketid,
> +                                            uint32_t *bandwidth)
> +{
> +    uint64_t sample1, sample2;
> +    uint32_t upscaling_factor;
> +    int retry_attempts = 0;
> +    int rc;
> +
> +    do {
> +        rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
> +                                                   &sample1);
> +        if (rc < 0) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        usleep(10000);
> +
> +        rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
> +                                                   &sample2);
> +        if (rc < 0) {
> +           rc = ERROR_FAIL;
> +           goto out;
> +        }
> +
> +        if (sample2 >= sample1)

If sample2 == sample1 then bandwidth is zero. Is this expected?

> +            break;
> +
> +        if (retry_attempts < MBM_SAMPLE_RETRY_MAX) {
> +            retry_attempts++;
> +        } else {
> +            LOGE(ERROR, "event counter overflowed");
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +    } while(1);

Minor nit, should be "while (1)".

The rest of this patch looks OK to me.

Wei.

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