On Thu, Jun 11, 2015 at 03:02:55PM +0200, Hendrik Brueckner wrote:
> On Thu, Jun 11, 2015 at 12:25:01PM +0200, Peter Zijlstra wrote:
> > On Thu, 2015-06-11 at 11:59 +0200, Hendrik Brueckner wrote:
> > > The ENOTSUPP (which actually should be EOPNOTSUPP for user space) does not
> > > trigger a fallback event selection, for example, by perf record.
> > > If hardware support for the cycles perf event is available, but the hardware
> > > does not provide interrupts, returning ENOTSUPP causes perf to end. Returning
> > > ENOENT causes the perf tool to fallback to a software-based cycle PMU that
> > > supports interrupts.
> > >
> > > The commit 53b25335dd ("perf: Disable sampled events if no PMU interrupt")
> > > introduced that incompatible change.
> >
> > That's 3.16
>
> Correct... I recently encountered the problem.
>
> >
> > > if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
> > > - err = -ENOTSUPP;
> > > + err = -ENOENT;
> > > goto err_alloc;
> > > }
> > > }
> >
> > And now you would be changing an API that's been around for at least 4
> > releases.
>
> Well... the behavior before 53b25335dd was differently in this regard. Of
> course, the API changed 4 releases ago. The question here is rather was
> this desired or not. In my mind I considered this problem as a regression.
Ah, I see what you mean:
97b1198fece0 ("s390, perf: Use common PMU interrupt disabled code")
> > Also, I really think -ENOENT is the wrong return here, you're asking for
> > things that's not supported, not for something that's not there.
>
> So looks like -ENOTSUPP is the desired API now. So the problem I'd like
> to solve is that there are two different hardware PMUs that support the
> "cycles" event. Just one of them supports sampling of cycles, the other not.
perf_event_attr::type will uniquely identify the pmu.
if (event->attr.type != pmu->type)
return -ENOENT;
/* event is for this pmu, any fail hereafter should be fatal */
if (is_sampling_event(event))
return -EOPNOTSUPP;
> In the past (prior to 3.16), the perf tool tried several PMUs if -ENOENT
> was returned. With 3.16, -ENOTSUPP is returned (which actually should be
> -EOPNOTSUPP but different story) and the perf tool exits.
So cpumf_pmu_event_init() should not have returned -ENOENT to start
with.
It should have first ascertained that this event was indeed for that
pmu, if not, -ENOENT would indeed be the correct return. However once it
finds its an event for this pmu, which requests sampling, which this pmu
cannot deliver, it should have returned a fatal error (!-ENOENT).
-EOPNOTSUPP would have been a good one there.
> So the question is: what is the desired behavior?
I think desired would be EOPNOTSUPP, but it would mean yet another
change to the API.
Then again, seeing how this isn't actually working no, that might not be
too bad.
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
