-----Original Message----- From: Jiri Denemark [mailto:jdenemar@redhat.com] Sent: Monday, November 23, 2015 11:26 PM To: Ren, Qiaowei Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/8] perf: add new public APIs for perf event

On Tue, Nov 17, 2015 at 16:00:41 +0800, Qiaowei Ren wrote:

...

API agreed on in https://www.redhat.com/archives/libvir-list/2015-October/msg00872.html

* include/libvirt/libvirt-domain.h (virDomainGetPerfEvents, virDomainSetPerfEvents): New declarations. * src/libvirt_public.syms: Export new symbols.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- include/libvirt/libvirt-domain.h | 18 ++++++++++++++++++ src/libvirt_public.syms | 6 ++++++ 2 files changed, 24 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index a1ea6a5..69965e6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1802,6 +1802,24 @@ int virDomainListGetStats(virDomainPtr *doms, void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);

/* + * Perf Event API + */ + +/** + * VIR_DOMAIN_PERF_CMT: + * + * Macro for typed parameter name that represents CMT perf event. + */ +# define VIR_DOMAIN_PERF_CMT "cmt" + +int virDomainGetPerfEvents(virDomainPtr dom, + virTypedParameterPtr params, + int * nparams);

We don't put a space between * and the name.

...

+int virDomainSetPerfEvents(virDomainPtr dom, + virTypedParameterPtr params, + int nparams); + +/* * BlockJob API */

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index dd94191..03206e7 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -725,4 +725,10 @@ LIBVIRT_1.2.19 { virDomainRename; } LIBVIRT_1.2.17;

+LIBVIRT_1.2.23 { + global: + virDomainGetPerfEvents; + virDomainSetPerfEvents; +} LIBVIRT_1.2.19; + # .... define new API here using predicted next version number ....

This patch does not compile, you should squash patches 1, 2, and 3 together, since 'make all check syntax-check' should pass after each patch in the series.

Ok. This is just from the sample about "Implementing a new API in Libvirt" (https://libvirt.org/api_extension.html ). ^-^ I will make these 3 patches together in next version. Thanks, Qiaowei

-----Original Message----- From: Jiri Denemark [mailto:jdenemar@redhat.com] Sent: Tuesday, November 24, 2015 12:25 AM To: Ren, Qiaowei Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 3/8] perf: implement the public APIs for perf event

On Tue, Nov 17, 2015 at 16:00:43 +0800, Qiaowei Ren wrote:

...

* src/libvirt-domain.c: Implement virDomainGetPerfEvents and virDomainSetPerfEvents.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- src/libvirt-domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index de7eb04..e767606 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -9572,6 +9572,112 @@ virDomainOpenChannel(virDomainPtr dom,

/** + * virDomainGetPerfEvents: + * @domain: pointer to domain object + * @params: pointer to perf events parameter object + * (return value, allocated by the caller) + * @nparams: pointer to number of perf event parameters + * + * Get all perf events setting. On input, @nparams gives the size of + the + * @params array; on output, @nparams gives how many slots were + filled + * with parameter information, which might be less but will not + exceed + * the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 + on + * input will cause @nparams on output to contain the number of + parameters + * supported by the hypervisor. The caller should then allocate + @params + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call + the + * API again. + * + * See virDomainGetMemoryParameters() for an equivalent usage example.

You took a wrong API to copy. Requiring the caller to run the API with @params == NULL to get the number of parameters and then pass the allocated @params to get the values is the original design which we do not use for new APIs. The API should just allocate @params, fill it with all the values it knows about, and return the count in @nparams. In other words, no caller allocated arrays, please. You can look at, e.g., virDomainGetJobStats to see how it works.

Thanks, Jirka. I will update this patch according to new API design. Thanks, Qiaowei

On Tue, Nov 17, 2015 at 16:00:45 +0800, Qiaowei Ren wrote:

This patch implement a set of interfaces for perf event. Based on these interfaces, we can implement internal driver API for perf, and get the results of perf conuter you care about.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- include/libvirt/virterror.h | 2 + src/Makefile.am | 1 + src/libvirt_private.syms | 8 ++ src/util/virerror.c | 2 + src/util/virperf.c | 222 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virperf.h | 60 ++++++++++++ 6 files changed, 295 insertions(+) create mode 100644 src/util/virperf.c create mode 100644 src/util/virperf.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index f716cb9..59dd8e7 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -128,6 +128,8 @@ typedef enum { VIR_FROM_THREAD = 61, /* Error from thread utils */ VIR_FROM_ADMIN = 62, /* Error from admin backend */

Extra empty line.

+ VIR_FROM_PERF = 63, /* Error from perf */ + # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST # endif diff --git a/src/Makefile.am b/src/Makefile.am index 99b4993..afc6cf0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -98,6 +98,7 @@ UTIL_SOURCES = \ util/virauthconfig.c util/virauthconfig.h \ util/virbitmap.c util/virbitmap.h \ util/virbuffer.c util/virbuffer.h \ + util/virperf.c util/virperf.h \ util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h \ util/virclosecallbacks.c util/virclosecallbacks.h \ util/vircommand.c util/vircommand.h util/vircommandpriv.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a835f18..f0e2bd5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1988,6 +1988,14 @@ virPCIGetVirtualFunctions; virPCIIsVirtualFunction;

+# util/vircgroup.h

s/vircgroup/virperf/

+virPerfInitialize; +virPerfFree; +virPerfEventOfType; +virPerfCmtEnable; +virPerfEventDisable; + + # util/virpidfile.h virPidFileAcquire; virPidFileAcquirePath; diff --git a/src/util/virerror.c b/src/util/virerror.c index 6dc05f4..12a5218 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -134,6 +134,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Polkit", /* 60 */ "Thread jobs", "Admin Interface", +

Extra empty line.

+ "Perf", )

diff --git a/src/util/virperf.c b/src/util/virperf.c new file mode 100644 index 0000000..9c71858 --- /dev/null +++ b/src/util/virperf.c @@ -0,0 +1,222 @@ +/* + * virperf.c: methods for managing perf events + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Ren Qiaowei <qiaowei.ren@intel.com> + */ +#include <config.h> + +#include <sys/ioctl.h> +#if defined HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif + +#include "virperf.h" +#include "viralloc.h" +#include "virerror.h" +#include "virlog.h" +#include "virfile.h" +#include "virstring.h" +#include "virtypedparam.h" + +VIR_LOG_INIT("util.perf"); + +#define VIR_FROM_THIS VIR_FROM_PERF + +#if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H) +# define VIR_PERF_SUPPORTED +#endif + +VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, + "cmt"); + + +#ifdef VIR_PERF_SUPPORTED

Why don't you use defined(__linux__) && defined(HAVE_SYS_SYSCALL_H) directly here?

+ +#include <linux/perf_event.h>

This should be "# include <linux/perf_event.h>".

+ +int +virPerfInitialize(virPerfPtr *perf)

We usually create such functions with virPerfPtr virPerfNew(void) prototype. You can then use it if (!(perf = virPerfNew())) goto error;

+{ + size_t i; + + if (VIR_ALLOC((*perf)) < 0) + return -1; + + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + (*perf)->events[i].type = i; + (*perf)->events[i].fd = -1; + (*perf)->events[i].enabled = false; + } + + return 0; +}

+ +void +virPerfFree(virPerfPtr *perf)

void virPerFree(virPerfPtr perf) is the prototype we usually use for this kind of functions.

+{ + size_t i; + + if (*perf == NULL) + return; + + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + if ((*perf)->events[i].enabled) + virPerfEventDisable(i, *perf); + } + + VIR_FREE(*perf); +} + +virPerfEventPtr +virPerfEventOfType(virPerfPtr perf, + int type)

This is a private API, using enum type [1] is OK. I think virPerfGetEvent would be a better name for this API.

+{ + size_t i; + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + if (perf->events[i].type == type) + return &perf->events[i]; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Event '%d' is not supported"), + type); + + return NULL; +}

The perf->events array is indexed by event type so why do we walk through all of them instead of just: if (type >= VIR_PERF_EVENT_LAST) { virReportError(VIR_ERR_INTERNAL_ERROR, ...); return NULL; } return perf->events + type;

+ +static int +sys_perf_event_open(struct perf_event_attr *hw_event, + pid_t pid, + int cpu, + int group_fd, + unsigned long flags) +{ + return syscall(__NR_perf_event_open, hw_event, pid, cpu, + group_fd, flags); +}

The name of this wrapper does not really match the name convention used for all other functions in this file. But, I don't see a real reason for this one-line wrapper since it is only used once.

+ +int +virPerfCmtEnable(pid_t pid, + virPerfPtr perf)

I think 'perf' should always be the first parameter of all virPerf functions, since it is the object all the functions operate on.

From a caller perspective, I think it would be a bit better to create a generic int virPerfEventEnable(perf, type, pid) entry point, and make virPerfCmtEnable static. Unless each type requires different input data.

+{ + struct perf_event_attr cmt_attr; + int event_type; + FILE *fp = NULL; + virPerfEventPtr event = NULL; + + fp = fopen("/sys/devices/intel_cqm/type", "r"); + if (!fp) { + virReportSystemError(errno, "%s", + _("CMT is not available on this host")); + return -1; + } + if (fscanf(fp, "%d", &event_type) != 1) { + virReportSystemError(errno, "%s", + _("Unable to read event type file.")); + VIR_FORCE_FCLOSE(fp); + return -1; + } + VIR_FORCE_FCLOSE(fp); + + event = virPerfEventOfType(perf, VIR_PERF_EVENT_CMT); + + memset(&cmt_attr, 0, sizeof(struct perf_event_attr)); + cmt_attr.size = sizeof(struct perf_event_attr);

Using sizeof(cmt_attr) instead of sizeof(struct perf_event_attr) is a bit more robust and it is the preferred way in libvirt.

+ cmt_attr.type = event_type; + cmt_attr.config = 1; + cmt_attr.inherit = 1; + cmt_attr.disabled = 1; + cmt_attr.enable_on_exec = 0; + + event->fd = sys_perf_event_open(&cmt_attr, pid, -1, -1, 0); + if (event->fd < 0) { + virReportSystemError(errno, + _("Unable to open perf type=%d for pid=%d"), + event_type, pid); + return -1; + } + + if (ioctl(event->fd, PERF_EVENT_IOC_ENABLE) < 0) { + virReportSystemError(errno, "%s", + _("Unable to enable perf event for CMT"));

You should close and reset event->fd here.

+ return -1; + } + + event->enabled = true; + return 0;

Overall, this function would benefit from introducing error and cleanup labels to group all cleanups at one place. And you should avoid touching event structure until you know everything succeeded to make the cleanups cleaner.

+} + +int +virPerfEventDisable(int type, + virPerfPtr perf) +{ + virPerfEventPtr event = virPerfEventOfType(perf, type);

You should return -1 if event is NULL...

+ + if (event && ioctl(event->fd, PERF_EVENT_IOC_DISABLE) < 0) { + virReportSystemError(errno, "%s", + _("Unable to disable perf event for CMT")); + return -1; + } + + event->enabled = false;

otherwise libvirt will crash here.

+ VIR_FORCE_CLOSE(event->fd); + return 0; +} + +#else /* !VIR_PERF_SUPPORTED */ +int +virPerfInitialize(virPerfPtr perf ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENXIO, "%s", + _("Perf not supported on this platform")); + return 0; +} + +void +virPerfFree(virPerfPtr perf ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENXIO, "%s", + _("Perf not supported on this platform")); +}

At least virPerfNew and virPerfFree should work even if perf is not supported so that the caller can initialize and free its structures without thinking about this.

+ +virPerfEventPtr +virPerfEventOfType(virPerfPtr perf ATTRIBUTE_UNUSED, + int type ATTRIBUTE_UNUSED) +{

No error?

+ return NULL; +} + +int +virPerfCmtEnable(pid_t pid ATTRIBUTE_UNUSED, + virPerfPtr perf ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENXIO, "%s", + _("Perf not supported on this platform")); + return -1; +} + +int +virPerfEventDisable(int event ATTRIBUTE_UNUSED, + virPerfPtr perf ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENXIO, "%s", + _("Perf not supported on this platform")); + return -1; +} + +#endif /* !VIR_PERF_SUPPORTED */ diff --git a/src/util/virperf.h b/src/util/virperf.h new file mode 100644 index 0000000..bab6597 --- /dev/null +++ b/src/util/virperf.h @@ -0,0 +1,60 @@ +/* + * virperf.h: methods for managing perf events + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Ren Qiaowei <qiaowei.ren@intel.com> + */ + +#ifndef __VIR_PERF_H__ +# define __VIR_PERF_H__ + +# include "virutil.h" + +enum { + VIR_PERF_EVENT_CMT, + + VIR_PERF_EVENT_LAST +};

This enum should be a typedef so that it can be used for declaring variables [1].

+ +VIR_ENUM_DECL(virPerfEvent); + +struct virPerfEvent { + int type; + int fd; + bool enabled; +}; +typedef struct virPerfEvent *virPerfEventPtr; + +struct virPerf { + struct virPerfEvent events[VIR_PERF_EVENT_LAST]; +}; +typedef struct virPerf *virPerfPtr;

We usually try to hide the internals of such structures in .c and provide accessors.

+ +int virPerfInitialize(virPerfPtr *perf); + +void virPerfFree(virPerfPtr *perf); + +virPerfEventPtr virPerfEventOfType(virPerfPtr perf, + int type); + +int virPerfCmtEnable(pid_t pid, + virPerfPtr perf); + +int virPerfEventDisable(int type, + virPerfPtr perf); + +#endif /* __VIR_PERF_H__ */ -- 1.9.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Nov 17, 2015 at 16:00:46 +0800, Qiaowei Ren wrote:

This patch implement the internal driver API for perf event into qemu driver.

In addition, this patch extend virDomainListGetStats API to get the statistics for perf event. To do so, we add a 'VIR_DOMAIN_STATS_PERF' enum to causes reporting of all previously enabled perf events.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 181 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 6 ++ 4 files changed, 191 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 69965e6..5e74b29 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1771,6 +1771,7 @@ typedef enum { VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */ VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */ VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */ + VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */ } virDomainStatsTypes;

typedef enum { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4be998c..0348d4a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -26,6 +26,7 @@

# include "virthread.h" # include "vircgroup.h" +# include "virperf.h" # include "domain_addr.h" # include "domain_conf.h" # include "snapshot_conf.h" @@ -190,6 +191,8 @@ struct _qemuDomainObjPrivate {

virCgroupPtr cgroup;

+ virPerfPtr perf; + virCond unplugFinished; /* signals that unpluggingDevice was unplugged */ const char *unpluggingDevice; /* alias of the device that is being unplugged */ char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 92a9961..5ad4d79 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -98,6 +98,7 @@ #include "virhostdev.h" #include "domain_capabilities.h" #include "vircgroup.h" +#include "virperf.h" #include "virnuma.h" #include "dirname.h" #include "network/bridge_driver.h" @@ -113,6 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver");

#define QEMU_NB_NUMA_PARAM 2

+#define QEMU_NB_PERF_PARAM VIR_PERF_EVENT_LAST +

VIR_PERF_EVENT_LAST is good enough, no need to define another symbol for it.

#define QEMU_SCHED_MIN_PERIOD 1000LL #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL @@ -10244,6 +10247,106 @@ qemuDomainGetNumaParameters(virDomainPtr dom, }

static int +qemuDomainSetPerfEvents(virDomainPtr dom, + virTypedParameterPtr params, + int nparams) +{ + size_t i; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + int ret = -1; + bool enabled; + + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_PERF_CMT, + VIR_TYPED_PARAM_BOOLEAN, + NULL) < 0) + return -1;

Use virTypedParamsCheck and define the data for it in virperf.h so that you don't need to change the code here if new event type is added.

+ + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; + + priv = vm->privateData; + + if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + enabled = params[i].value.b; + + if (STREQ(param->field, VIR_DOMAIN_PERF_CMT)) {

Just use the appropriate FromString to convert the param field into the perf type enum and call virPerfEvent{Disable,Enable} depending on the value. Thus no change will be needed here if you add more perf events.

+ if (!enabled && virPerfEventDisable(VIR_PERF_EVENT_CMT, priv->perf)) { + virReportError(VIR_ERR_NO_SUPPORT, + _("can't disable perf event: %s"), + VIR_DOMAIN_PERF_CMT); + goto cleanup; + } + if (enabled && virPerfCmtEnable(vm->pid, priv->perf)) { + virReportError(VIR_ERR_NO_SUPPORT, + _("can't enable perf event: %s"), + VIR_DOMAIN_PERF_CMT); + goto cleanup; + }

Both virPerfEventDisable and virPerfEventEnable should have reported the error, no need to mask it with another one.

+ } + } + + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +qemuDomainGetPerfEvents(virDomainPtr dom, + virTypedParameterPtr params, + int * nparams) +{ + size_t i; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + int ret = -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + priv = vm->privateData; + + if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if ((*nparams) == 0) { + *nparams = QEMU_NB_PERF_PARAM; + ret = 0; + goto cleanup; + } + + for (i = 0; i < QEMU_NB_PERF_PARAM && i < *nparams; i++) { + virMemoryParameterPtr param = ¶ms[i];

Comments similar to SetPerfEvents apply here as well.

+ + switch (i) { + case VIR_PERF_EVENT_CMT: + if (virTypedParameterAssign(param, VIR_DOMAIN_PERF_CMT, + VIR_TYPED_PARAM_BOOLEAN, + priv->perf->events[VIR_PERF_EVENT_CMT].enabled) < 0)

This should rather you an access function.

+ goto cleanup; + break; + default: + break; + } + } + + if (*nparams > QEMU_NB_PERF_PARAM) + *nparams = QEMU_NB_PERF_PARAM; + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, unsigned long long period, long long quota) { @@ -19419,6 +19522,81 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

#undef QEMU_ADD_COUNT_PARAM

+static int +qemuDomainGetStatsPerfCmt(virPerfEventPtr eventPtr, + virDomainStatsRecordPtr record, + int *maxparams) +{ + FILE *fd; + unsigned long long cache = 0; + int scaling_factor = 0; + + if (eventPtr->fd <= 0 || !eventPtr->enabled) + return -1; + + if (read(eventPtr->fd, &cache, sizeof(uint64_t)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to read cache data")); + return -1; + } + + fd = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");

Will this value change in time? If not, we could just read it and store within virPerfEvent when enabling it. It may be necessary to turn virPerfEvent into union to do that.

+ if (!fd) { + virReportSystemError(errno, "%s", + _("Unable to open CMT scale file")); + return -1; + } + if (fscanf(fd, "%d", &scaling_factor) != 1) { + virReportSystemError(errno, "%s", + _("Unable to read CMT scale file")); + VIR_FORCE_FCLOSE(fd); + return -1; + } + VIR_FORCE_FCLOSE(fd);

Anyway, it looks like this would deserve to be moved to virperf.c (and use cleanup lables).

+ cache *= scaling_factor; + + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "perf.cache", + cache) < 0) + return -1; + + return 0; +} + +static int +qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + qemuDomainObjPrivatePtr priv = dom->privateData; + int ret = -1; + + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + virPerfEventPtr event = virPerfEventOfType(priv->perf, i); + if (!event->enabled) + continue; + + switch (i) {

Typecast 'i' to the perf event type...

+ case VIR_PERF_EVENT_CMT: + if (qemuDomainGetStatsPerfCmt(event, record, maxparams)) + goto cleanup; + break; + default: + break;

and remove this default branch so that the compiler reminds you to change this code in case new perf event is added.

+ } + } + + ret = 0; + cleanup: + return ret; +} + typedef int (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, virDomainObjPtr dom, @@ -19439,6 +19617,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false }, { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true }, + { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false }, { NULL, 0, false } };

@@ -20227,6 +20406,8 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainMigrateFinish3 = qemuDomainMigrateFinish3, /* 0.9.2 */ .domainMigrateConfirm3 = qemuDomainMigrateConfirm3, /* 0.9.2 */ .domainSendKey = qemuDomainSendKey, /* 0.9.4 */ + .domainGetPerfEvents = qemuDomainGetPerfEvents, /* 1.2.23 */ + .domainSetPerfEvents = qemuDomainSetPerfEvents, /* 1.2.23 */ .domainBlockJobAbort = qemuDomainBlockJobAbort, /* 0.9.4 */ .domainGetBlockJobInfo = qemuDomainGetBlockJobInfo, /* 0.9.4 */ .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4545f77..2d81069 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4680,6 +4680,10 @@ int qemuProcessStart(virConnectPtr conn, if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) goto error;

+ VIR_DEBUG("Initializing perf event"); + if (virPerfInitialize(&(priv->perf)) < 0) + goto cleanup;

This is why virPerfNew should not return a failure in case perf events are not supported.

+ /* This must be done after cgroup placement to avoid resetting CPU * affinity */ if (!vm->def->cputune.emulatorpin && @@ -5187,6 +5191,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, } virCgroupFree(&priv->cgroup);

+ virPerfFree(&(priv->perf)); + qemuProcessRemoveDomainStatus(driver, vm);

/* Remove VNC and Spice ports from port reservation bitmap, but only if

Jirka

On Mon, Nov 30, 2015 at 08:17:05 +0000, Ren, Qiaowei wrote:

...

-----Original Message----- From: Jiri Denemark [mailto:jdenemar@redhat.com] Sent: Tuesday, November 24, 2015 9:01 PM To: Ren, Qiaowei Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 6/8] qemu_driver: add support to perf event

On Tue, Nov 17, 2015 at 16:00:46 +0800, Qiaowei Ren wrote:

...

This patch implement the internal driver API for perf event into qemu driver.

In addition, this patch extend virDomainListGetStats API to get the statistics for perf event. To do so, we add a 'VIR_DOMAIN_STATS_PERF' enum to causes reporting of all previously enabled perf events.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 181 +++++++++++++++++++++++++++++++++++++++ #define QEMU_SCHED_MIN_PERIOD 1000LL #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL @@ -10244,6 +10247,106 @@ qemuDomainGetNumaParameters(virDomainPtr dom, }

static int +qemuDomainSetPerfEvents(virDomainPtr dom, + virTypedParameterPtr params, + int nparams) +{ + size_t i; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + int ret = -1; + bool enabled; + + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_PERF_CMT, + VIR_TYPED_PARAM_BOOLEAN, + NULL) < 0) + return -1;

Use virTypedParamsCheck and define the data for it in virperf.h so that you don't need to change the code here if new event type is added.

Jirka, I checked the difference between virTypedParamsValidate and virTypedParamsCheck, and found that virTypedParamsCheck just check whether the param is valid through param name, but virTypedParamsValidate will check whether the param type is right.

Oh, sorry for the confusion then. I actually wanted the code to be something like if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMS) < 0) return -1; and VIR_PERF_PARAMS would be a macro defined in virparams.h: # define VIR_PERF_PARAMS \ VIR_DOMAIN_PERF_CMT, VIR_TYPED_PARAM_BOOLEAN, \ NULL Similar to QEMU_MIGRATION_PARAMETERS and few others. Jirka