[libvirt] [PATCH 1/1] perf: add support to perf event for MBM

[Peter Krempa]

On Wed, May 11, 2016 at 17:09:01 +0800, Qiaowei Ren wrote:
> MBM (Memory Bandwidth Monitoring) is a new feature introduced in some
> Intel processor families. MBM is build on the CMT (Cache Monitoring
> Technology) infrastructure to allow monitoring of bandwidth from one
> level of the cache hierarchy to the next. With current perf framework,
> this patch adds support to perf event for MBM.
> 
> Signed-off-by: Qiaowei Ren <qiaowei.ren at intel.com>
> ---
>  include/libvirt/libvirt-domain.h | 14 +++++++++++
>  src/qemu/qemu_driver.c           | 50 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virperf.c               | 40 +++++++++++++++++++++-----------
>  src/util/virperf.h               |  2 ++
>  4 files changed, 92 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 160f20f..9c3795c 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1904,6 +1904,20 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
>   */
>  # define VIR_PERF_PARAM_CMT "cmt"
> 
> +/**
> + * VIR_PERF_PARAM_MBMT:
> + *
> + * Macro for typed parameter name that represents MBMT perf event.
> + */
> +# define VIR_PERF_PARAM_MBMT "mbmt"
> +
> +/**
> + * VIR_PERF_PARAM_MBML:
> + *
> + * Macro for typed parameter name that represents MBML perf event.
> + */
> +# define VIR_PERF_PARAM_MBML "mbml"

Neither of those documents the unit or meaning of the value.


[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c4c4968..8c79e49 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

> @@ -19515,6 +19517,46 @@ qemuDomainGetStatsPerfCmt(virPerfPtr perf,
>  }
>  
>  static int
> +qemuDomainGetStatsPerfMbmt(virPerfPtr perf,
> +                           virDomainStatsRecordPtr record,
> +                           int *maxparams)
> +{
> +    uint64_t total_bytes = 0;
> +
> +    if (virPerfReadEvent(perf, VIR_PERF_EVENT_MBMT, &total_bytes) < 0)
> +        return -1;
> +
> +    if (virTypedParamsAddDouble(&record->params,
> +                                &record->nparams,
> +                                maxparams,
> +                                "perf.total_bytes",
> +                                total_bytes*1e-6) < 0)

Memory bandwidth seems like it's in MiB/s thus dividing by 1 000 000
is not the right conversion. Additionally we should report it in
bytes/s. The users can always convert it to any value they like.

> +        return -1;
> +
> +    return 0;
> +}
> +
> +static int
> +qemuDomainGetStatsPerfMbml(virPerfPtr perf,
> +                           virDomainStatsRecordPtr record,
> +                           int *maxparams)
> +{
> +    uint64_t local_bytes = 0;
> +
> +    if (virPerfReadEvent(perf, VIR_PERF_EVENT_MBML, &local_bytes) < 0)
> +        return -1;
> +
> +    if (virTypedParamsAddDouble(&record->params,
> +                                &record->nparams,
> +                                maxparams,
> +                                "perf.local_bytes",
> +                                local_bytes*1e-6) < 0)
> +        return -1;
> +
> +    return 0;
> +}

Both above functions are almost identical. I think  you could merge them
and pass the difference as an argument.

> +
> +static int
>  qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                         virDomainObjPtr dom,
>                         virDomainStatsRecordPtr record,

[...]


> diff --git a/src/util/virperf.c b/src/util/virperf.c
> index bd65587..7cfdd77 100644
> --- a/src/util/virperf.c
> +++ b/src/util/virperf.c

[...]

> @@ -132,26 +132,36 @@ virPerfCmtEnable(virPerfEventPtr event,

This does not seem to be the correct function to put this. All the
variables and name hint that it's CMT specific ...

>      }
>      VIR_FREE(buf);
>  
> -    if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale",
> -                       10, &buf) < 0)
> -        goto error;
> -
> -    if (virStrToLong_ui(buf, NULL, 10, &scale) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("failed to get cmt scaling factor"));
> -        goto error;
> -    }
> -
> -    event->efields.cmt.scale = scale;
> -
>      memset(&cmt_attr, 0, sizeof(cmt_attr));
>      cmt_attr.size = sizeof(cmt_attr);
>      cmt_attr.type = event_type;
> -    cmt_attr.config = 1;
>      cmt_attr.inherit = 1;
>      cmt_attr.disabled = 1;
>      cmt_attr.enable_on_exec = 0;
>  
> +    switch (event->type) {
> +    case VIR_PERF_EVENT_CMT:
> +        if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale",
> +                           10, &buf) < 0)
> +            goto error;
> +
> +        if (virStrToLong_ui(buf, NULL, 10, &scale) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("failed to get cmt scaling factor"));
> +            goto error;
> +        }
> +        event->efields.cmt.scale = scale;
> +
> +        cmt_attr.config = 1;
> +        break;
> +    case VIR_PERF_EVENT_MBMT:
> +        cmt_attr.config = 2;
> +        break;
> +    case VIR_PERF_EVENT_MBML:
> +        cmt_attr.config = 3;
> +        break;
> +    }
> +
>      event->fd = syscall(__NR_perf_event_open, &cmt_attr, pid, -1, -1, 0);
>      if (event->fd < 0) {
>          virReportSystemError(errno,
> @@ -186,6 +196,8 @@ virPerfEventEnable(virPerfPtr perf,
>  
>      switch (type) {
>      case VIR_PERF_EVENT_CMT:
> +    case VIR_PERF_EVENT_MBMT:
> +    case VIR_PERF_EVENT_MBML:
>          if (virPerfCmtEnable(event, pid) < 0)

... If you are going to reuse this function for the other events you
need to refactor it before and make it universal.

>              return -1;
>          break;

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160511/87f2a956/attachment-0001.sig>