Re: [PATCH 3/5] perf: Ability to enable in a paused mode

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Jun 12, 2010 at 11:44:26AM +0200, Peter Zijlstra wrote:
> On Sat, 2010-06-12 at 09:34 +0200, Frederic Weisbecker wrote:
> >  struct pmu {
> >         int (*enable)                   (struct perf_event *event);
> > +       /*
> > +        * Reserve acts like enable, except the event must go in a "pause"
> > +        * state. Ie: it is scheduled but waiting to be started
> > +        * with the ->start() callback.
> > +        */
> > +       int (*reserve)                  (struct perf_event *event);
> >         void (*disable)                 (struct perf_event *event); 
> 
> Urgh, so then we have, enable(), reserve() and start(), that's just too
> much. Also, you need to visit all pmu implementations if you touch
> struct pmu like that.


No, in fact that's convenient way not to need to check every pmus.
Only those I know and could test got this reserve implemented. Those that
haven't a reserve will fallback to enable and won't enter this PAUSED
state, task exclusion will then miss the first slice between schedule
and the next interrupt, but it's not dangerous. It's just that if
one wants to support a given pmu, he needs to check this pmu to provide
a relevant implementation.

It was the most convenient way as I don't need to check every pmus,
and I suspect I'm going to be quite useless on sparc, powerpc or arm
as I can't event test these archs (I have no more access to a sparc box).

But yeah I understand your worries about adding a new callback for that.

If you prefer I can provide a flag on enable() and return -ENOSYS for
those I can't test or audit. In fact this is probably more sane, and
we could even not fallback to a normal enable() in case of no support
for the paused mode, and put the event in an error state in this case.
The user requested task exclusion anyway, it's better to tell him we
can't rather than half doing it.


On software events I can just return immediately, on x86 I can maintain
an internal state that gets checked on hw_perf_*able() but the generic
state is convenient enough for that, so I will likely reuse it unless
there are oppositions about this, in which case I can maintain a
duplicate internal paused state for x86 events.


Please tell me what you think about this.

Thanks.