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@xxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list