Re: [PATCH v2 3/8] perf: implement a set of util functions for perf event

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Mon, Dec 07, 2015 at 03:53:54PM +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 |   1 +
>  src/Makefile.am             |   1 +
>  src/libvirt_private.syms    |  12 ++
>  src/util/virerror.c         |   1 +
>  src/util/virperf.c          | 298 ++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virperf.h          |  61 +++++++++
>  6 files changed, 374 insertions(+)
>  create mode 100644 src/util/virperf.c
>  create mode 100644 src/util/virperf.h
> 

> +VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST,
> +              "cmt");
> +
> +struct virPerfEvent {
> +    int type;
> +    int fd;
> +    bool enabled;
> +    union {
> +        /* cmt */
> +        struct {
> +            int scale;
> +        } cmt;
> +    } efields;
> +};
> +typedef struct virPerfEvent *virPerfEventPtr;
> +
> +struct virPerf {
> +    struct virPerfEvent events[VIR_PERF_EVENT_LAST];
> +};
> +
> +virPerfPtr
> +virPerfNew(void)
> +{
> +    size_t i;
> +    virPerfPtr perf;
> +
> +    if (VIR_ALLOC(perf) < 0)
> +        return NULL;
> +
> +    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 perf;
> +}
> +
> +void
> +virPerfFree(virPerfPtr perf)
> +{
> +    size_t i;
> +
> +    if (perf == NULL)
> +        return;
> +
> +    for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
> +        if (perf->events[i].enabled)
> +            virPerfEventDisable(perf, i);
> +    }
> +
> +    VIR_FREE(perf);
> +}
> +
> +#if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)
> +
> +# include <linux/perf_event.h>
> +
> +static virPerfEventPtr
> +virPerfGetEvent(virPerfPtr perf,
> +                virPerfEventType type)
> +{
> +    if (type >= VIR_PERF_EVENT_LAST) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Event '%d' is not supported"),
> +                       type);
> +        return NULL;
> +    }
> +
> +    return perf->events + type;
> +}
> +
> +static int
> +virPerfCmtEnable(virPerfEventPtr event,
> +                 pid_t pid)
> +{
> +    struct perf_event_attr cmt_attr;
> +    int event_type;
> +    FILE *fp = NULL;
> +    FILE *fp_scale = NULL;
> +    int scale = 0;
> +
> +    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."));
> +        goto cleanup;
> +    }

Scanf has pretty poor error reporting, so we'd usually do

   char *buf;
   if (virFileReadAll("/path", 100, &buf) < 0)
        return -1;

   if (virStrToLong_ui(buf, NULL, 10, &scale) < 0) {
       VIR_FREE(buf);
        ....report error...
	return -1;
   }

   VIR_FREE(buf);


> +
> +    fp_scale = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");
> +    if (!fp_scale) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to open CMT scale file"));
> +        goto cleanup;
> +    }
> +    if (fscanf(fp_scale, "%d", &scale) != 1) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to read CMT scale file"));
> +        goto cleanup;
> +    }

Same comment here.

> +    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;
> +
> +    event->fd = syscall(__NR_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);
> +        goto cleanup;
> +    }
> +
> +    if (ioctl(event->fd, PERF_EVENT_IOC_ENABLE) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to enable perf event for CMT"));
> +        goto cleanup;
> +    }
> +
> +    event->enabled = true;
> +    return 0;
> +
> + cleanup:
> +    if (fp)
> +        VIR_FORCE_FCLOSE(fp);
> +    if (fp_scale)
> +        VIR_FORCE_FCLOSE(fp_scale);
> +    if (event->fd >= 0)
> +        VIR_FORCE_CLOSE(event->fd);
> +    return -1;


> +int
> +virPerfEventDisable(virPerfPtr perf,
> +                    virPerfEventType type)
> +{
> +    virPerfEventPtr event = virPerfGetEvent(perf, type);
> +    if (event == NULL)
> +        return -1;
> +
> +    if (ioctl(event->fd, PERF_EVENT_IOC_DISABLE) < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to disable perf event type=%d"),
> +                             event->type);
> +        return -1;
> +    }

Since we're closing the file handle next, is there any benefit
in doing this ioctl() ?

> +
> +    event->enabled = false;
> +    VIR_FORCE_CLOSE(event->fd);
> +    return 0;
> +}

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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