All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: Robert Richter <robert.richter@amd.com>
Cc: Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@elte.hu>,
	Arnaldo Carvalho de Melo <acme@ghostprotocols.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] [PATCH] perf: Attaching an event to a specific PMU
Date: Tue, 05 Jul 2011 10:51:45 +0200	[thread overview]
Message-ID: <1309855905.3282.42.camel@twins> (raw)
In-Reply-To: <20110704175927.GZ4590@erda.amd.com>

On Mon, 2011-07-04 at 19:59 +0200, Robert Richter wrote:

> 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.

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

> 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.

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

> 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?

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

> 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?

It adds a redundant interface.

> 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.

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()

> 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.

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

  reply	other threads:[~2011-07-05  8:52 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-07-03 15:04 [RFC] [PATCH] perf: Attaching an event to a specific PMU Robert Richter
2011-07-03 18:04 ` Peter Zijlstra
2011-07-04 17:59   ` Robert Richter
2011-07-05  8:51     ` Peter Zijlstra [this message]
2011-07-05  9:12       ` Ingo Molnar
2011-07-06 16:53         ` Robert Richter
2011-07-06 17:10           ` Ingo Molnar
2011-07-06 17:14             ` Peter Zijlstra
2011-07-06 17:15               ` Ingo Molnar
2011-07-07 10:22             ` Robert Richter
2011-07-06 17:12           ` Peter Zijlstra
2011-07-07  9:21             ` Robert Richter
2011-07-07  9:39               ` Robert Richter
2011-07-07 19:38               ` Peter Zijlstra
2011-07-05  9:47     ` [PATCH] perf: Extend attr check to allow also dynamically generated Robert Richter
2011-07-05 10:51       ` Peter Zijlstra
2011-07-05 10:56         ` Robert Richter
2011-07-05 10:53     ` [PATCH] perf: Extend attr check to allow also dynamically generated types Robert Richter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1309855905.3282.42.camel@twins \
    --to=a.p.zijlstra@chello.nl \
    --cc=acme@ghostprotocols.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=paulus@samba.org \
    --cc=robert.richter@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.