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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> + */
> +#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@xxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list