Hello
it was pointed out to me that in the 3.7 kernel (more specifically,
3581fe0ef37ce12ac7a4f74831168352ae848edc ) a change was made in the
ARM architecture to change how PERF_EVENT_IOC_PERIOD is handled.
Unlike other architectures, post-3.7 ARM updates the period right away
rather than waiting until the next overflow.
I can't find any discussion as to why this change was made.
Should other architectures be updated to? I just wanted to find out the
rationale for this before I update the manpage to reflect the difference
in behaviors between architectures.
Vince
On Mon, Oct 28, 2013 at 02:37:46AM +0000, Vince Weaver wrote:
> Hello
Hi Vince,
> it was pointed out to me that in the 3.7 kernel (more specifically,
> 3581fe0ef37ce12ac7a4f74831168352ae848edc ) a change was made in the
> ARM architecture to change how PERF_EVENT_IOC_PERIOD is handled.
>
> Unlike other architectures, post-3.7 ARM updates the period right away
> rather than waiting until the next overflow.
>
> I can't find any discussion as to why this change was made.
This was in response to complaints from both internal users and people on
public lists:
http://www.mail-archive.com/[email protected]/msg02657.html
I believe the scenario was something like:
(1) An instruction counter is set up to overflow after 200 instructions,
with a SIGIO handler to print some information. It is initially
disabled.
(2) At some point, the counter is enabled for 1 overflow (IOC_REFRESH)
(3) The counter eventually overflows and the SIGIO handler is triggered.
At this pointer the counter is disabled.
(4) The signal handler changes the period to 200k instructions using
IOC_PERIOD and enables the counter for a further overflow.
(5) SIGIO is taken after 200 instructions, rather than 200k.
> Should other architectures be updated to? I just wanted to find out the
> rationale for this before I update the manpage to reflect the difference
> in behaviors between architectures.
I don't want to be the `oddball' architecture (at least, not more than I am
already :), but I do find it tricky to follow the required semantics of perf
as opposed to `it happens to work this way', especially when so much of it
is buried in the various arch backends. So if somebody using the thing on
ARM has (what looks to me like) a valid issue, then I usually try and fix
it.
Cheers,
Will
On Mon, Oct 28, 2013 at 08:57:00AM +0000, Will Deacon wrote:
> > Should other architectures be updated to? I just wanted to find out the
> > rationale for this before I update the manpage to reflect the difference
> > in behaviors between architectures.
>
> I don't want to be the `oddball' architecture (at least, not more than I am
> already :), but I do find it tricky to follow the required semantics of perf
> as opposed to `it happens to work this way', especially when so much of it
> is buried in the various arch backends. So if somebody using the thing on
> ARM has (what looks to me like) a valid issue, then I usually try and fix
> it.
Hurmph.. at least raise the issue for the other archs.
On Mon, Oct 28, 2013 at 10:00:49AM +0000, Peter Zijlstra wrote:
> On Mon, Oct 28, 2013 at 08:57:00AM +0000, Will Deacon wrote:
> > > Should other architectures be updated to? I just wanted to find out the
> > > rationale for this before I update the manpage to reflect the difference
> > > in behaviors between architectures.
> >
> > I don't want to be the `oddball' architecture (at least, not more than I am
> > already :), but I do find it tricky to follow the required semantics of perf
> > as opposed to `it happens to work this way', especially when so much of it
> > is buried in the various arch backends. So if somebody using the thing on
> > ARM has (what looks to me like) a valid issue, then I usually try and fix
> > it.
>
> Hurmph.. at least raise the issue for the other archs.
Yeah, sorry about that. I thought Stephane might take it forward (since he
was on the perfmon thread I linked to) and I did CC you on the patch.
While we're at it, there was another patch that I CC'd others on since it
might be wanted by other architectures too: cb2d8b342aa0 ("ARM: 7698/1:
perf: fix group validation when using enable_on_exec"). Basically, the
patch ensures that events that are set to enable on exec are included in
group validation, despite being in the OFF state at validation time.
Any thoughts on that?
Will
On Mon, 28 Oct 2013, Will Deacon wrote:
> This was in response to complaints from both internal users and people on
> public lists:
>
> http://www.mail-archive.com/[email protected]/msg02657.html
>
> I believe the scenario was something like:
>
> (1) An instruction counter is set up to overflow after 200 instructions,
> with a SIGIO handler to print some information. It is initially
> disabled.
>
> (2) At some point, the counter is enabled for 1 overflow (IOC_REFRESH)
>
> (3) The counter eventually overflows and the SIGIO handler is triggered.
> At this pointer the counter is disabled.
>
> (4) The signal handler changes the period to 200k instructions using
> IOC_PERIOD and enables the counter for a further overflow.
>
> (5) SIGIO is taken after 200 instructions, rather than 200k.
It would be nice if changelogs for patches had this level of detail.
It's also a shame this change apprently didn't hit the linux-kernel list
as far as I can tell. I do my best to try to note all of the perf
ABI-related changes there, but if things like this are going to start
getting merged in architecture trees then things get that much harder
to keep track of.
> I don't want to be the `oddball' architecture (at least, not more than I am
> already :), but I do find it tricky to follow the required semantics of perf
> as opposed to `it happens to work this way', especially when so much of it
> is buried in the various arch backends. So if somebody using the thing on
> ARM has (what looks to me like) a valid issue, then I usually try and fix
> it.
But it was global behavior that was common on all architectures.
Now any cross-platform tool like PAPI is going to have to have a mess of
#ifdefs around every use of this ioctl, and it will only get worse if
other architectures decide to "fix" the problem too.
Vince
On Mon, Oct 28, 2013 at 02:07:48PM +0000, Vince Weaver wrote:
> It's also a shame this change apprently didn't hit the linux-kernel list
> as far as I can tell. I do my best to try to note all of the perf
> ABI-related changes there, but if things like this are going to start
> getting merged in architecture trees then things get that much harder
> to keep track of.
I can CC LKML on ARM perf patches if you think it will help, but all PMU
backend patches go via their respective arch trees afaict.
> > I don't want to be the `oddball' architecture (at least, not more than I am
> > already :), but I do find it tricky to follow the required semantics of perf
> > as opposed to `it happens to work this way', especially when so much of it
> > is buried in the various arch backends. So if somebody using the thing on
> > ARM has (what looks to me like) a valid issue, then I usually try and fix
> > it.
>
> But it was global behavior that was common on all architectures.
>
> Now any cross-platform tool like PAPI is going to have to have a mess of
> #ifdefs around every use of this ioctl, and it will only get worse if
> other architectures decide to "fix" the problem too.
What would you like me to do to fix this for you? Moving more code out of
the backends and into the core will help maintain consistency between
architectures, but that's a huge job.
Will
On Tue, Oct 29, 2013 at 04:28:10AM +0000, Will Deacon wrote:
> On Mon, Oct 28, 2013 at 02:07:48PM +0000, Vince Weaver wrote:
> > It's also a shame this change apprently didn't hit the linux-kernel list
> > as far as I can tell. I do my best to try to note all of the perf
> > ABI-related changes there, but if things like this are going to start
> > getting merged in architecture trees then things get that much harder
> > to keep track of.
>
> I can CC LKML on ARM perf patches if you think it will help, but all PMU
> backend patches go via their respective arch trees afaict.
Just those that change user visible semantics that are shared between
archs I suppose :-)
> > > I don't want to be the `oddball' architecture (at least, not more than I am
> > > already :), but I do find it tricky to follow the required semantics of perf
> > > as opposed to `it happens to work this way', especially when so much of it
> > > is buried in the various arch backends. So if somebody using the thing on
> > > ARM has (what looks to me like) a valid issue, then I usually try and fix
> > > it.
> >
> > But it was global behavior that was common on all architectures.
> >
> > Now any cross-platform tool like PAPI is going to have to have a mess of
> > #ifdefs around every use of this ioctl, and it will only get worse if
> > other architectures decide to "fix" the problem too.
>
> What would you like me to do to fix this for you? Moving more code out of
> the backends and into the core will help maintain consistency between
> architectures, but that's a huge job.
We could start by making all archs do the same thing again; but yes
ideally we'd move some of it into generic code. Not entirely sure how
that will work out though, there's a reason its in per-arch code :/
Vince, what would you prefer to do here?
On Tue, 29 Oct 2013, Peter Zijlstra wrote:
> On Tue, Oct 29, 2013 at 04:28:10AM +0000, Will Deacon wrote:
> >
> > I can CC LKML on ARM perf patches if you think it will help, but all PMU
> > backend patches go via their respective arch trees afaict.
>
> Just those that change user visible semantics that are shared between
> archs I suppose :-)
I suppose it is hard to know what's commonly shared. I hadn't realized
that the IOC_PERIOD stuff was arch specific code, I would have thought
it was common code.
Since there isn't a perf-specific list CCing LKML might be the answer even
though it sometimes adds to the noise. I think the Power people CC all
their PMU related patches to LKML and it has made them easier to find and
review.
> We could start by making all archs do the same thing again; but yes
> ideally we'd move some of it into generic code. Not entirely sure how
> that will work out though, there's a reason its in per-arch code :/
>
>
> Vince, what would you prefer to do here?
as with most of thes things there isn't really a good answer.
It turns out in the end that PAPI isn't bit by this one, because instead
of using PERF_EVENT_IOC_PERIOD when the period is changed, PAPI just tears
down all the perf_events and re-sets them up from scratch with the new
period. This is probably because PERF_EVENT_IOC_PERIOD was broken until
2.6.36.
It is true the current behavior is unexpected. What was the logic behind
deferring to the next overflow for the update? Was it a code simplicity
thing? Or were there hardware reasons behind it?
Definitely when an event is stopped, it makes more sense for
PERF_EVENT_IOC_PERIOD to take place immediately.
I'm not sure what happens if we try to use it on a running event,
especially if we've already passed the new period value.
Vince
On Tue, Oct 29, 2013 at 11:36:52AM -0400, Vince Weaver wrote:
> On Tue, 29 Oct 2013, Peter Zijlstra wrote:
> > On Tue, Oct 29, 2013 at 04:28:10AM +0000, Will Deacon wrote:
> > >
> > > I can CC LKML on ARM perf patches if you think it will help, but all PMU
> > > backend patches go via their respective arch trees afaict.
> >
> > Just those that change user visible semantics that are shared between
> > archs I suppose :-)
>
> I suppose it is hard to know what's commonly shared. I hadn't realized
> that the IOC_PERIOD stuff was arch specific code, I would have thought
> it was common code.
OK, so I've gone over this and this isn't in fact arch specific at all.
The arch code should simply use ->period to reset the counters, and
stuff the last period into ->last_period.
Aside from that it shouldn't do anything. So what ARM did was actively
wrong.
Esp. since it added code to the common path instead of the uncommon
(ioctl) path.
> > We could start by making all archs do the same thing again; but yes
> > ideally we'd move some of it into generic code. Not entirely sure how
> > that will work out though, there's a reason its in per-arch code :/
> >
> >
> > Vince, what would you prefer to do here?
>
> as with most of thes things there isn't really a good answer.
Yeah, I was afraid of that :/
> It turns out in the end that PAPI isn't bit by this one, because instead
> of using PERF_EVENT_IOC_PERIOD when the period is changed, PAPI just tears
> down all the perf_events and re-sets them up from scratch with the new
> period. This is probably because PERF_EVENT_IOC_PERIOD was broken until
> 2.6.36.
Right, it was one of those interfaces that people claimed were
absolutely required so I implemented them but then nobody actually tried
using them for a long while :-(
This is a prime example of why Ingo now insists the perf tools supports
every new interface, we had too many of these incidents.
> It is true the current behavior is unexpected. What was the logic behind
> deferring to the next overflow for the update? Was it a code simplicity
> thing? Or were there hardware reasons behind it?
Mostly an oversight I think. The delay is simply how it worked out in
that the arch code has to reload the period once an event fires in order
to reprogram. Since nobody actually used the thing, nobody had
experience with it.
Now it turns out someone had a complaint but hid it somewhere on some
obscure list :-(
There is actually generic code that force resets the period; see
perf_event_period().
> Definitely when an event is stopped, it makes more sense for
> PERF_EVENT_IOC_PERIOD to take place immediately.
>
> I'm not sure what happens if we try to use it on a running event,
> especially if we've already passed the new period value.
The below code should deal with both cases I think -- completely
untested.
---
arch/arm/kernel/perf_event.c | 4 ----
kernel/events/core.c | 16 +++++++++++++++-
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index e186ee1e63f6..4eb288f7ba69 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -99,10 +99,6 @@ int armpmu_event_set_period(struct perf_event *event)
s64 period = hwc->sample_period;
int ret = 0;
- /* The period may have been changed by PERF_EVENT_IOC_PERIOD */
- if (unlikely(period != hwc->last_period))
- left = period - (hwc->last_period - left);
-
if (unlikely(left <= -period)) {
left = period;
local64_set(&hwc->period_left, left);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 17b3c6cf1606..c45d53e561da 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3530,7 +3530,7 @@ static void perf_event_for_each(struct perf_event *event,
static int perf_event_period(struct perf_event *event, u64 __user *arg)
{
struct perf_event_context *ctx = event->ctx;
- int ret = 0;
+ int ret = 0, active;
u64 value;
if (!is_sampling_event(event))
@@ -3554,6 +3554,20 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
event->attr.sample_period = value;
event->hw.sample_period = value;
}
+
+ active = (event->state == PERF_EVENT_STATE_ACTIVE);
+ if (active) {
+ perf_pmu_disable(ctx->pmu);
+ event->pmu->stop(event, PERF_EF_UPDATE);
+ }
+
+ local64_set(event->hw.period_left, 0);
+
+ if (active) {
+ event->pmu->start(event, PERF_EF_RELOAD);
+ perf_pmu_enable(ctx->pmu);
+ }
+
unlock:
raw_spin_unlock_irq(&ctx->lock);
On Wed, Oct 30, 2013 at 10:56 AM, Peter Zijlstra <[email protected]> wrote:
> On Tue, Oct 29, 2013 at 11:36:52AM -0400, Vince Weaver wrote:
>> On Tue, 29 Oct 2013, Peter Zijlstra wrote:
>> > On Tue, Oct 29, 2013 at 04:28:10AM +0000, Will Deacon wrote:
>> > >
>> > > I can CC LKML on ARM perf patches if you think it will help, but all PMU
>> > > backend patches go via their respective arch trees afaict.
>> >
>> > Just those that change user visible semantics that are shared between
>> > archs I suppose :-)
>>
>> I suppose it is hard to know what's commonly shared. I hadn't realized
>> that the IOC_PERIOD stuff was arch specific code, I would have thought
>> it was common code.
>
> OK, so I've gone over this and this isn't in fact arch specific at all.
> The arch code should simply use ->period to reset the counters, and
> stuff the last period into ->last_period.
>
> Aside from that it shouldn't do anything. So what ARM did was actively
> wrong.
>
> Esp. since it added code to the common path instead of the uncommon
> (ioctl) path.
>
>> > We could start by making all archs do the same thing again; but yes
>> > ideally we'd move some of it into generic code. Not entirely sure how
>> > that will work out though, there's a reason its in per-arch code :/
>> >
>> >
>> > Vince, what would you prefer to do here?
>>
>> as with most of thes things there isn't really a good answer.
>
> Yeah, I was afraid of that :/
>
>> It turns out in the end that PAPI isn't bit by this one, because instead
>> of using PERF_EVENT_IOC_PERIOD when the period is changed, PAPI just tears
>> down all the perf_events and re-sets them up from scratch with the new
>> period. This is probably because PERF_EVENT_IOC_PERIOD was broken until
>> 2.6.36.
>
> Right, it was one of those interfaces that people claimed were
> absolutely required so I implemented them but then nobody actually tried
> using them for a long while :-(
>
Yes, I now remember about this problem. As Vince said, this is an old
issue which never got solved. I remember getting questions about it.
I would expect this ioctl to be used by PAPI because they are doing
user level sampling, i.e., get a user notification for each event.
> This is a prime example of why Ingo now insists the perf tools supports
> every new interface, we had too many of these incidents.
>
>> It is true the current behavior is unexpected. What was the logic behind
>> deferring to the next overflow for the update? Was it a code simplicity
>> thing? Or were there hardware reasons behind it?
>
> Mostly an oversight I think. The delay is simply how it worked out in
> that the arch code has to reload the period once an event fires in order
> to reprogram. Since nobody actually used the thing, nobody had
> experience with it.
>
> Now it turns out someone had a complaint but hid it somewhere on some
> obscure list :-(
>
> There is actually generic code that force resets the period; see
> perf_event_period().
>
>> Definitely when an event is stopped, it makes more sense for
>> PERF_EVENT_IOC_PERIOD to take place immediately.
>>
>> I'm not sure what happens if we try to use it on a running event,
>> especially if we've already passed the new period value.
>
> The below code should deal with both cases I think -- completely
> untested.
>
I can test this easily with libpfm4.
> ---
> arch/arm/kernel/perf_event.c | 4 ----
> kernel/events/core.c | 16 +++++++++++++++-
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> index e186ee1e63f6..4eb288f7ba69 100644
> --- a/arch/arm/kernel/perf_event.c
> +++ b/arch/arm/kernel/perf_event.c
> @@ -99,10 +99,6 @@ int armpmu_event_set_period(struct perf_event *event)
> s64 period = hwc->sample_period;
> int ret = 0;
>
> - /* The period may have been changed by PERF_EVENT_IOC_PERIOD */
> - if (unlikely(period != hwc->last_period))
> - left = period - (hwc->last_period - left);
> -
> if (unlikely(left <= -period)) {
> left = period;
> local64_set(&hwc->period_left, left);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 17b3c6cf1606..c45d53e561da 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3530,7 +3530,7 @@ static void perf_event_for_each(struct perf_event *event,
> static int perf_event_period(struct perf_event *event, u64 __user *arg)
> {
> struct perf_event_context *ctx = event->ctx;
> - int ret = 0;
> + int ret = 0, active;
> u64 value;
>
> if (!is_sampling_event(event))
> @@ -3554,6 +3554,20 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
> event->attr.sample_period = value;
> event->hw.sample_period = value;
> }
> +
> + active = (event->state == PERF_EVENT_STATE_ACTIVE);
> + if (active) {
> + perf_pmu_disable(ctx->pmu);
> + event->pmu->stop(event, PERF_EF_UPDATE);
> + }
> +
> + local64_set(event->hw.period_left, 0);
> +
> + if (active) {
> + event->pmu->start(event, PERF_EF_RELOAD);
> + perf_pmu_enable(ctx->pmu);
> + }
> +
> unlock:
> raw_spin_unlock_irq(&ctx->lock);
>
On Wed, 30 Oct 2013, Peter Zijlstra wrote:
> Right, it was one of those interfaces that people claimed were
> absolutely required so I implemented them but then nobody actually tried
> using them for a long while :-(
Well, the initial perf_event code drop was a big huge new interface
without much documentation and it took a while for people to come up to
speed on it.
People have tried to use PERF_EVENT_IOC_PERIOD but stopped because it was
buggy. It is still used, as someone complaining to me about the manpage
description not matching reality is what started this whole thread.
There was a push for getting this issue fixed back in 2010 but it never
went anywhere.
The userspace users will report bugs, but they are usually intimidated by
the linux-kernel list and won't push along changes themselves. Usually
it's up to someone like me or Stephane to do that, and we miss things
sometimes.
The users will just find a workaround and drop the issue, especially if
it's going to take years before the fix shows up in RHEL or whatever
ancient vendor kernel they are using.
> The below code should deal with both cases I think -- completely
> untested.
Uncompiled too I guess?
kernel/events/core.c: In function ?perf_event_period?:
kernel/events/core.c:3531: error: invalid type argument of ?->? (have ?local64_t?)
make[3]: *** [kernel/events/core.o] Error 1
I also won't be able to test the ARM change, as my pandaboard won't boot
with recent kernels (can't find the MMC root filesystem) and I haven't had
time to track down why. Also even on a simple period changing test it
often fails due to lost interrupts (the Cortex-A9 lost interrupt errata?).
Vince
On Wed, Oct 30, 2013 at 02:13:11PM +0000, Vince Weaver wrote:
> On Wed, 30 Oct 2013, Peter Zijlstra wrote:
> > The below code should deal with both cases I think -- completely
> > untested.
>
> Uncompiled too I guess?
>
> kernel/events/core.c: In function ‘perf_event_period’:
> kernel/events/core.c:3531: error: invalid type argument of ‘->’ (have ‘local64_t’)
> make[3]: *** [kernel/events/core.o] Error 1
That's trivial to fix.
> I also won't be able to test the ARM change, as my pandaboard won't boot
> with recent kernels (can't find the MMC root filesystem) and I haven't had
> time to track down why. Also even on a simple period changing test it
> often fails due to lost interrupts (the Cortex-A9 lost interrupt errata?).
The omap guys like trying to blame the A9 erratum for that (which doesn't
even affect the cycle counter), but the reality is that the CTI never
worked reliably with mainline, and now there are no developers working
on that after the TI layoffs. I don't see the state of Pandaboard support
improving over time.
If you have a canned testcase for this stuff, I'm happy to run it on my
Chromebook.
Will
On Wed, 30 Oct 2013, Will Deacon wrote:
> On Wed, Oct 30, 2013 at 02:13:11PM +0000, Vince Weaver wrote:
> > Uncompiled too I guess?
> >
> > kernel/events/core.c: In function ‘perf_event_period’:
> > kernel/events/core.c:3531: error: invalid type argument of ‘->’ (have ‘local64_t’)
> > make[3]: *** [kernel/events/core.o] Error 1
>
> That's trivial to fix.
Probably, although Peter's been sending some "interesting" mostly untested
perf patches to linux-kernel the past few days so before testing I'd
rather have a patch that at least compiles cleanly.
> > I also won't be able to test the ARM change, as my pandaboard won't boot
> > with recent kernels (can't find the MMC root filesystem) and I haven't had
> > time to track down why. Also even on a simple period changing test it
> > often fails due to lost interrupts (the Cortex-A9 lost interrupt errata?).
>
> The omap guys like trying to blame the A9 erratum for that (which doesn't
> even affect the cycle counter), but the reality is that the CTI never
> worked reliably with mainline, and now there are no developers working
> on that after the TI layoffs. I don't see the state of Pandaboard support
> improving over time.
So it might be OMAP4460/CTI specific and not related to the errata?
That might be nice especially if things might work on other cortex-a9
boards.
A shame about the TI people, they were often semi-responsive to my
OMAP4460 questions even if it was usually just to say "ignore the
documentation, that's broken, don't do that"
> If you have a canned testcase for this stuff, I'm happy to run it on my
> Chromebook.
There's the tests/ioctl/ioctl_period test in my perf_event_tests.git tree
but it's very simple. Currently it will likely just return success on
current x86 Linux but fail on newish ARM and patched x86. I need to
fix it to print old_behavior/new_behavior/fail I guess.
I have a cortex-a15 chromebook too, but I haven't managed to get a
recent kernel booting on it yet.
Vince