perf: Attaching an event to a specific PMU

Peter,

this is a prototype implementation for attaching an event to a specific PMU. If there is a general acceptance for this approach I will create patches for upstream integration and base my current IBS patches on it.

-Robert

This patch creates device nodes for each pmu using udev:

 # ls -l /dev/pmu/
 total 0
 crw-rw---- 1 root root 254, 5 Jul  8  2011 breakpoint
 crw-rw---- 1 root root 254, 4 Jul  8  2011 cpu
 crw-rw---- 1 root root 254, 6 Jul  8  2011 proto
 crw-rw---- 1 root root 254, 1 Jul  8  2011 software
 crw-rw---- 1 root root 254, 2 Jul  8  2011 tracepoint

After opening a device the pmu's file descriptor can be used to attach an event to it. This works same as attaching an event to a specific group:

        pmu = open("/dev/pmu/proto", O_RDONLY);
        ...
        event = sys_perf_event_open(&attr, 0, -1, pmu, 0);

This patch includes a working example that attaches an event to the PMU registered with the name 'proto':

 # ls -l /dev/pmu/proto
 crw-rw---- 1 root root 254, 6 Jul  8  2011 /dev/pmu/proto
 # dmesg -c > /dev/null
 # ./proto
 # dmesg -c
 Found event ffff88041de71c00 (config=0000000000f00ba2) for pmu proto (type=6) on cpu -1
 Adding event ffff88041de71c00 (config=0000000000f00ba2) to pmu proto (type=6) on cpu 1
 Removing event ffff88041de71c00 (config=0000000000f00ba2) to pmu proto (type=6) on cpu 1
 Adding event ffff88041de71c00 (config=0000000000f00ba2) to pmu proto (type=6) on cpu 1
 Removing event ffff88041de71c00 (config=0000000000f00ba2) to pmu proto (type=6) on cpu 1

Building the example:

 $ cd linux         # Linux kernel source dir
 $ make -C tools/perf/Documentation/examples CFLAGS=-I../.. proto

This approach works for fixed pmu types and also for dynamically allocated pmus.

I intend to use this event allocation method to implement AMD IBS. Other pmus can be implemented similar, such as northbridge and/or uncore events for x86. The implementation is generic and not limited to a single architecture, it is useful in every system with multiple pmus.

---
 include/linux/perf_event.h                |    1 +
 kernel/events/core.c                      |  179 ++++++++++++++++++++++++++---
 tools/perf/Documentation/examples/proto.c |   51 ++++++++
 3 files changed, 213 insertions(+), 18 deletions(-)
 create mode 100644 tools/perf/Documentation/examples/proto.c

index e76a410..3c5452e 100644
@@ -602,6 +602,7 @@ struct pmu {
 	struct list_head		entry;
 
 	struct device			*dev;
+	struct device			*cldev;
 	char				*name;
 	int				type;
 

1.7.5.3


-- 
Advanced Micro Devices, Inc.
Operating System Research Center

But there's already an infrastructure to do this, simply stick the contents of /sys/bus/event_source/devices/*/type in perf_event_attr::type and you're done.

I don't see the need for /dev crap.

Using sysfs by reading from /sys/bus/event_source/devices/*/type would work too. But there are several reasons why I like my approach.

First of all, it follows the idea of grouping events. Attaching events to a specific pmu is not different from attaching them to a specific event group. It is actually the same if we think of one group for events that may be scheduled on only one pmu. Thus, treating a pmu like a group event is the logical step for intuitive usage of the perf_open syscall. This way we have symmetrical implementations for binding events to groups or pmus.

The syscall interface has already everything needed for this. Following the concept of attaching events to a group with a file descriptor, we simply must create a file descriptor pointing to a pmu device. This works without extending the perf_open syscall interface, no changes to the interface are needed.

Device nodes are the general approach for controlling devices from user-space, they are integral part of the Linux device driver model. With a device file descriptor opened from a device node we can explicitly point to a pmu device.

Representing a device with a device node is common programming practice. Usage of device nodes is not deprecated. There are existing frameworks to easily create such devices. With dynamically device node allocation and udev there are solutions for drawbacks of /dev. Why not having a device node for pmus? What are your concerns using /dev?

The patch introduces a pmu device class and devices are grouped by class now. They can be found in /dev/pmu/* instead of searching for device attributes somewhere in the system hirarchie at /sys/bus/event_source/devices/*/type. A device node is easier to handle.

The implementation only needs about 150 lines of kernel code. It is straight and separated. There is nothing special what makes it hard to read or maintain. The code is using typical kernel device allocation methods. Do you think this patch makes kernel code too complex?

There is much easier user-space code:

       pmu = open("/dev/pmu/proto", O_RDONLY);
       if (pmu == -1)
               err(1, "pmu not found");

       attr.config = 0xf00ba2;

       event = sys_perf_event_open(&attr, 0, -1, pmu, 0);

vs.

        pmu = open("/sys/bus/event_source/devices/proto/type", O_RDONLY);
        if (pmu == -1)
                err(1, "pmu not found");
        size = read(pmu, buf, BUFSIZ - 1);
        if (size < 0) {
                close(pmu);
                err(1, "failed to read pmu type");
        }
        buf[size] = '0';

        attr.type = atoi(buf);
        attr.config = 0xf00ba2;

       event = sys_perf_event_open(&attr, 0, -1, -1, 0);

There are also additional header includes and variable declarations needed.

(Btw, current kernel code does not support dynamically allocated pmu types due to a check in perf_copy_attr():

        if (attr->type >= PERF_TYPE_MAX)
                return -EINVAL;

)

Beside of that, using /sys/ is racy. There is no protection against unregistering the pmu. Probably this might not cause big problems in practice, but it can be done better. With open/close we can protect the pmu from being removed.

Overall, my approach improves the perf design. It adds a better and more intuitve access to perf from user space with clear and common methods and interfaces. Please let me know the concerns you have.

Thanks,

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

That's not a good analogy. Grouping is about events being together, not about events being on a particular pmu.

Yeah, but we already have a /sys interface, so this ship has sailed.

Its impossible to represent events using that /dev interface, furthermore we already have a /sys interface, so this is pure duplication of a

It adds a redundant interface.

Why can't the open/close of the sysfs file provide the same? It just means you have to close after sys_perf_event_open()

Its redundant, this interface ship has sailed, its not going to happen.

Even if we had the choice, i don't see how a /dev based enumeration of PMUs is in any way better than a topologically attached set of PMUs in /sys.

This kind of structure is nice in principle:

 # ls -l /dev/pmu/
 total 0
 crw-rw---- 1 root root 254, 5 Jul  8  2011 breakpoint
 crw-rw---- 1 root root 254, 4 Jul  8  2011 cpu
 crw-rw---- 1 root root 254, 6 Jul  8  2011 proto
 crw-rw---- 1 root root 254, 1 Jul  8  2011 software
 crw-rw---- 1 root root 254, 2 Jul  8  2011 tracepoint

But it should be done in /sys/. That way for example the graphics card:

00:02.0 VGA compatible controller

should have its events under:

/sys/devices/pci0000\:00/0000\:00\:02.0/events/

breakpoints could be listed in:

/sys/devices/system/cpu/cpu0/events/

core kernel software events could be listed in:

/sys/devices/system/core/events/

tracepoints could be listed in their respective subsystem directories:

/sys/devices/system/timekeeping/events/ # timer tracepoints
/sys/devices/system/machinecheck/events/ # the MCE tracepoint

/sys/kernel/slab/events/ # SLAB tracepoints

/sys/kernel/debug/tracing/events/ # the old, legacy location of

                                             # tracepoint events

there should also be a /sys/class/events/ populated with symlinks to all their locations.

Then we could transition all events to their respective sysfs locations, where they would be easily discoverable and enumerable.

The events would also automatically move (and with time, be automatically added) when a new device is added to sysfs.

Peter, there was a patch (from you?) that started doing this kind of sysfs enumeration with a class added as well - do you still have it or do you have a link to it?

Thanks,

Ingo

I have to learn yet why /dev is bad and /sys is good...

The system topology is always in /sys, also for device nodes. But we can't get a device file descriptor from /sys. I doubt /sys is capable to handle a device use count (need to be checked). We actually must grab the pmu while attaching events to it. And, user space implementation is must easier with /dev (see code in my previous mail).

My patch also includes code that creates a device class. It is also visible in /sys/class/pmu/*.

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

Because /sys is already there and already carries rather rich classification of various hardware components, devices and kernel subsystems.

/dev/ is mostly a flat registry of classical, unstructured devices.

I think Peter suggested it that an open() done in /sys should give us a handle to a given event?

But PMU is a very limited term: what we want is a higher level organization of 'event sources' and 'events'. Some may come from PMUs, many wont.

Thanks,

Ingo

No, more like the open of a sysfs file could also grab a refcount on the module. All it would take is a custom open/close method for sysfs files. Now I have no clue how to do that, but that's another issue.

Also, I really don't see the point. With modular PMUs there's always races against unload, just don't do that or live with it.

I don't think it's a big issue either - module load/unloads are rare.

Thanks,

Ingo

As already said, /sys provides system information. Of course it is richer then. But I also don't want/need to specify a pmu that deep in the system hierarchy. And /dev is flat and easy. What of the following would you prefer?

/sys/devices/pci0000:00/0000:00:03.0/usb2/2-2/2-2:1.1/input/input4/mouse0 /dev/input/mouse0

Beside of this, you can't control devices with /sys.

Using /dev solves the problems in a simple way, and simple things often work very well. With open() we control the refcount and we then get a device reference which is unique and immediately usable without parsing strings. This makes user code small and intuitive and also avoids complex config checks on the kernel side to catch wrongly assigned events.

While we probably can live with such races now on static pmus, thinking of future use cases like hotplugable graphic cards with pmus in it, we actually want a clean design for this. I don't see how to do this with sysfs. As there are no implementations yet for such an interface, we now have the chance to design it properly.

You can see it as 'virtual' pmu, like the virtual file system. Whatever the name for this is, 'event sources' is not very handy. But don't want to change this.

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

Its not so much /dev is bad, as not quite suited for this.

Well you must not per-se, from a user's perspective there isn't much of a difference between if the sys_perf_event_open() fails or if the initial file open fails, in both cases he's not getting an event.

perf stat -e IBS:fetches will always have a fail against rmmod, rmmod could complete before we try to open the file (assuming IBS is a module).

So not only are you providing a duplicate of existing interfaces, you're actually duplicating information inside sysfs as well.

It is not that I want to create event handles with open. For this there is the syscall. I use open() to create a unique reference to a pmu in userspace. No more, and /dev is the right thing to do this.

This is not duplication. /sys provides information about system hierarchy, /dev is for controlling devices. Thus, every device node is visible in /sys as it is part of the system.

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

One more note:

If the module is busy, it wont be unloaded.

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

Never said you wanted to do that.

Yeah, and its pointless.

Which ever way around you turn this problem it always ends up looking like:

  fd = open("/sys/foo");
  event = sys_perf_event_open();
  close(fd);
    /* do stuff */
  close(event);

The pmu not going away between the open() and sys_perf_event_open() is irrelevant. What is required is the success of both calls, up until that happens its a fail.

It is duplication, we already have a /sys representation of every pmu, by adding /sys/class/pmu/ you add another, that's pure duplication.

Below a fix for this.

-Robert

From: Robert Richter <[email protected]>
Date: Tue, 5 Jul 2011 11:04:39 +0200
Subject: [PATCH] perf: Extend attr check to allow also dynamically generated
types

When attaching events to a pmu with generated type, the initialization fails. Extending the check to allow such types.

---
 kernel/events/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index cca3588..5900729 100644
@@ -5958,7 +5958,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	 * If the type exists, the corresponding creation will verify
 	 * the attr->config.
 	 */
-	if (attr->type >= PERF_TYPE_MAX)
+	if (attr->type >= PERF_TYPE_MAX && !idr_find(&pmu_idr, attr->type))
 		return -EINVAL;
 
 	if (attr->__reserved_1)

1.7.5.3




-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Thanks for the pointer, ignore my resent patch.

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

(resent with fixed subject)

Below a fix for this.

-Robert

From: Robert Richter <[email protected]>
Date: Tue, 5 Jul 2011 11:04:39 +0200
Subject: [PATCH] perf: Extend attr check to allow also dynamically generated
types

When attaching events to a pmu with generated type, the initialization fails. Extending the check to allow such types.

---
 kernel/events/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index cca3588..5900729 100644
@@ -5958,7 +5958,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	 * If the type exists, the corresponding creation will verify
 	 * the attr->config.
 	 */
-	if (attr->type >= PERF_TYPE_MAX)
+	if (attr->type >= PERF_TYPE_MAX && !idr_find(&pmu_idr, attr->type))
 		return -EINVAL;
 
 	if (attr->__reserved_1)

1.7.5.3




-- 
Advanced Micro Devices, Inc.
Operating System Research Center