> -----Original Message----- > From: Peter Krempa [mailto:pkrempa@xxxxxxxxxx] > Sent: Thursday, May 12, 2016 4:31 PM > To: Ren, Qiaowei <qiaowei.ren@xxxxxxxxx> > Cc: libvir-list@xxxxxxxxxx; berrange@xxxxxxxxxx > Subject: Re: [PATCH 1/1] perf: add support to perf event for MBM > > On Thu, May 12, 2016 at 14:55:16 +0800, Qiaowei Ren wrote: > > Some Intel processor families (e.g. the Intel Xeon processor E5 v3 > > family) introduced some RDT (Resource Director Technology) features to > > monitor or control shared resource. Among these features, MBM (Memory > > Bandwidth Monitoring), which is build on the CMT (Cache Monitoring > > Technology) infrastructure, provides OS/VMM a way to monitor bandwidth > > from one level of cache to another. > > > > 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 | 18 ++++++++++++ > > src/qemu/qemu_driver.c | 35 +++++++++++++++++++---- > > src/util/virperf.c | 60 ++++++++++++++++++++++++---------------- > > src/util/virperf.h | 2 ++ > > 4 files changed, 85 insertions(+), 30 deletions(-) > > > > diff --git a/include/libvirt/libvirt-domain.h > > b/include/libvirt/libvirt-domain.h > > index 160f20f..e4594f0 100644 > > --- a/include/libvirt/libvirt-domain.h > > +++ b/include/libvirt/libvirt-domain.h > > @@ -1904,6 +1904,24 @@ void > virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); > > */ > > # define VIR_PERF_PARAM_CMT "cmt" > > > > +/** > > + * VIR_PERF_PARAM_MBMT: > > + * > > + * Macro for typed parameter name that represents MBMT perf event > > + * which can be used to monitor total system bandwidth (bytes/s) > > + * from one level of cache to another. > > + */ > > +# define VIR_PERF_PARAM_MBMT "mbmt" > > + > > +/** > > + * VIR_PERF_PARAM_MBML: > > + * > > + * Macro for typed parameter name that represents MBML perf event > > + * which can be used to monitor the amount of data (bytes/s) sent > > + * through the memory controller on the socket. > > + */ > > +# define VIR_PERF_PARAM_MBML "mbml" > > > So I was a little bit mistaken what this is documenting. So these document the > arguments for virDomainSetPerfEvents where the perf stuff is enabled. > > I was actually thinking this is documenting the values that are returned via the > virConnectGetAllDomainStats/virDomainListGetStats APIs. I've just noticed that > the perf events are not documented at all there. > > This should be fixed also for the CMT event. This part of the documentation > should then state which fields in the *Stats APIs the perf events correspond to. > > In the documentation for the fields returned bulk Stats API the individual fields > should be marked by which event they are produced. > Sure. The documentation will be added. > > + > > int virDomainGetPerfEvents(virDomainPtr dom, > > virTypedParameterPtr *params, > > int *nparams, diff --git > > a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index > > c4c4968..a3bd7ec 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -10051,6 +10051,8 @@ qemuDomainSetPerfEvents(virDomainPtr dom, > > > > if (virTypedParamsValidate(params, nparams, > > VIR_PERF_PARAM_CMT, > > VIR_TYPED_PARAM_BOOLEAN, > > + VIR_PERF_PARAM_MBMT, VIR_TYPED_PARAM_BOOLEAN, > > + VIR_PERF_PARAM_MBML, > > + VIR_TYPED_PARAM_BOOLEAN, > > NULL) < 0) > > return -1; > > > > @@ -19495,20 +19497,38 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr > > driver, #undef QEMU_ADD_COUNT_PARAM > > > > static int > > -qemuDomainGetStatsPerfCmt(virPerfPtr perf, > > +qemuDomainGetStatsPerfRdt(virPerfPtr perf, > > + virPerfEventType type, > > virDomainStatsRecordPtr record, > > int *maxparams) { > > - uint64_t cache = 0; > > + uint64_t value = 0; > > > > - if (virPerfReadEvent(perf, VIR_PERF_EVENT_CMT, &cache) < 0) > > + if (virPerfReadEvent(perf, type, &value) < 0) > > return -1; > > > > - if (virTypedParamsAddULLong(&record->params, > > + if (type == VIR_PERF_EVENT_CMT && > > + virTypedParamsAddULLong(&record->params, > > &record->nparams, > > maxparams, > > "perf.cache", > > - cache) < 0) > > + value) < 0) > > + return -1; > > + > > + if (type == VIR_PERF_EVENT_MBMT && > > + virTypedParamsAddULLong(&record->params, > > + &record->nparams, > > + maxparams, > > + "perf.total_bytes", > > If you pass this string into this function you won't need to special case all the > different types. > > Also by naming the fields similarly to the name of the feature (virPerfEvent enum) > you could entirely avoid the need to pass the name of the value as it could be > generated by the virPerfEventTypeToString. > (concatenate it with 'perf.' to get 'perf.mbmt', 'perf.cmt' and 'perf.mbml'). > > Unfortunately the implementation of the CMT event already used a different > name for the stats entry. > > Since this actually was never documented I would be in favor of changing > perf.cache to perf.cmt, so that the macros can be used. I cc'd danpb who > reviewed and pushed the first impl for another opinion. > Yes. I guess also that the new perf feature should be not used by customers and maybe we can change perf.cache to perf.cmt. Otherwise we can only use the macros for mbmt & mbml Thanks, Qiaowei -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list