perf: Ignore non-sampling overflows

perf: Ignore non-sampling overflows

Some arch implementations call perf_event_overflow() by 'accident', ignore this.

Reported-by Francis Moreau <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <new-submission>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/perf_event.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

kernel/perf_event.c +0 -0

index 98c5549..af1e63f 100644
@@ -4240,6 +4240,13 @@ static int __perf_event_overflow(struct perf_event *event, int nmi,
 	struct hw_perf_event *hwc = &event->hw;
 	int ret = 0;
 

Commit-ID:  963988262c3c8f4234f64a0dde59446a295e07bb
Gitweb:     http://git.kernel.org/tip/963988262c3c8f4234f64a0dde59446a295e07bb
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 24 Nov 2010 18:55:29 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 26 Nov 2010 15:14:55 +0100

perf: Ignore non-sampling overflows

Some arch implementations call perf_event_overflow() by 'accident', ignore this.

Reported-by Francis Moreau <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <new-submission>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/perf_event.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

kernel/perf_event.c +0 -0

index 98c5549..af1e63f 100644

kernel/perf_event.c +0 -1

 	if (!throttle) {
 		hwc->interrupts++;
 	} else {
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines

kernel/perf_event.c +0 -0

@@ -4240,6 +4240,13 @@ static int __perf_event_overflow(struct perf_event *event, int nmi,
 	struct hw_perf_event *hwc = &event->hw;
 	int ret = 0;
 

kernel/perf_event.c static int __perf_event_overflow(struct perf_event *event, int nmi, · line 4249

Quoted 7 lines

kernel/perf_event.c +7 -0

+	/*
+	 * Non-sampling counters might still use the PMI to fold short
+	 * hardware counters, ignore those.
+	 */
+	if (unlikely(!is_sampling_event(event)))
+		return 0;
+

do you remember the background of this change. This check silently drops data of non-sampling events. I want to use perf_event_overflow() to write to the buffer and want to modify the check, but don't see which 'accidentally' interrupts may occur that must be ignored.

Quoted 6 lines · Robert Richter

Quoted 6 lines

kernel/perf_event.c +0 -0

@@ -4240,6 +4240,13 @@ static int __perf_event_overflow(struct perf_event *event, int nmi,
     struct hw_perf_event *hwc = &event->hw;
     int ret = 0;
 

Quoted 12 lines

kernel/perf_event.c static int __perf_event_overflow(struct perf_event *event, int nmi, · line 4249

Quoted 7 lines

kernel/perf_event.c +7 -0

+   /*
+    * Non-sampling counters might still use the PMI to fold short
+    * hardware counters, ignore those.
+    */
+   if (unlikely(!is_sampling_event(event)))
+           return 0;
+

do you remember the background of this change. This check silently drops data of non-sampling events. I want to use perf_event_overflow() to write to the buffer and want to modify the check, but don't see which 'accidentally' interrupts may occur that must be ignored.

IIRC this is because we always program the interrupt bit, such that when the counter overflows we can account and reprogram the thing. This is needed because no hardware counter is in fact 64 bits wide. Therefore we have to program the counter to its max width and properly account the state and reprogram on overflow.

Imagine a 32bit cycle counter (@1GHz), if we were not to program that as taking interrupts and nobody would read that counter for about 4.2 seconds, we'd have overflowed and lost the actual count value for the thing.

So what we do is program is at 31bits (so that the msb can toggle and trigger the interrupt), and on interrupt add to event->count, and reset the hardware to start counting again.

Now some arch/*/perf_event.c implementations unconditionally called perf_event_overflow() from their IRQ handler, even for such non-sampling counters.

Quoted 31 lines · Peter Zijlstra

Quoted 6 lines · Robert Richter

Quoted 6 lines

kernel/perf_event.c +0 -0

@@ -4240,6 +4240,13 @@ static int __perf_event_overflow(struct perf_event *event, int nmi,
     struct hw_perf_event *hwc = &event->hw;
     int ret = 0;
 

Quoted 12 lines

kernel/perf_event.c static int __perf_event_overflow(struct perf_event *event, int nmi, · line 4249

Quoted 7 lines

kernel/perf_event.c +7 -0

+   /*
+    * Non-sampling counters might still use the PMI to fold short
+    * hardware counters, ignore those.
+    */
+   if (unlikely(!is_sampling_event(event)))
+           return 0;
+

do you remember the background of this change. This check silently drops data of non-sampling events. I want to use perf_event_overflow() to write to the buffer and want to modify the check, but don't see which 'accidentally' interrupts may occur that must be ignored.

IIRC this is because we always program the interrupt bit, such that when the counter overflows we can account and reprogram the thing. This is needed because no hardware counter is in fact 64 bits wide. Therefore we have to program the counter to its max width and properly account the state and reprogram on overflow.

Imagine a 32bit cycle counter (@1GHz), if we were not to program that as taking interrupts and nobody would read that counter for about 4.2 seconds, we'd have overflowed and lost the actual count value for the thing.

Quoted 7 lines

So what we do is program is at 31bits (so that the msb can toggle and trigger the interrupt), and on interrupt add to event->count, and reset the hardware to start counting again.

Now some arch/*/perf_event.c implementations unconditionally called perf_event_overflow() from their IRQ handler, even for such non-sampling counters.

I looked at the interrupt handlers. The events are always determined from a per-cpu array:

	cpuc = &__get_cpu_var(cpu_hw_events);
	...
	event = cpuc->events[idx];

In case of interrupts the event should then always be a hw event (or uninitialized). Even if the interrupt was triggered by a different source, it would always be mapped to the same event and the check is_sampling_event() would be meaningless.

There are other occurrences of perf_event_overflow() in kernel/events/core.c for events of type PERF_TYPE_SOFTWARE. These events are initialized with sample_period set and a check would always be true too.

For both cases I stil don't see a reason for the check.

Anyway, would the following extentension of the check above ok?

	if (unlikely(!is_sampling_event(event) && !event->attr.sample_type))
		...

With no bits set in attr.sample_type the sample would be empty and nothing to report. Now, with this change, samples that have data to report wouldn't be dropped anymore.

Quoted 7 lines · Robert Richter

I looked at the interrupt handlers. The events are always determined from a per-cpu array:

	cpuc = &__get_cpu_var(cpu_hw_events);
	...
	event = cpuc->events[idx];

Quoted 4 lines

In case of interrupts the event should then always be a hw event (or uninitialized). Even if the interrupt was triggered by a different source, it would always be mapped to the same event and the check is_sampling_event() would be meaningless.

I'm probably not quite getting what you mean, but how is is_sampling_event() meaningless? the INT bit is enabled for _all_ events, whether they were configured as a sampling event or not.

Its just that for !sampling events we shouldn't attempt to generate any output.

Quoted 5 lines

There are other occurrences of perf_event_overflow() in kernel/events/core.c for events of type PERF_TYPE_SOFTWARE. These events are initialized with sample_period set and a check would always be true too.

I'm failing to see what you mean, where do we always set event->attr.sample_period for software events?

Quoted 2 lines

For both cases I stil don't see a reason for the check.

You're going to have to spell things out for me, I'm really not getting your argument.

Quoted 6 lines

Anyway, would the following extentension of the check above ok?

	if (unlikely(!is_sampling_event(event) && !event->attr.sample_type))
		...

Quoted 3 lines

With no bits set in attr.sample_type the sample would be empty and nothing to report. Now, with this change, samples that have data to report wouldn't be dropped anymore.

Also, could you explain in what way data is dropped? Where do non-sampling events need to write sample data?

Quoted 3 lines

I'm probably not quite getting what you mean, but how is is_sampling_event() meaningless? the INT bit is enabled for _all_ events, whether they were configured as a sampling event or not.

Aren't all events that are mapped to counters via cpu_hw_events always sampling events?

Then, when calling perf_event_overflow() from an interrupt handler there are no other events than sampling events.

Quoted 2 lines

Its just that for !sampling events we shouldn't attempt to generate any output.

If attr.sample_type is null, there is no output to generate.

Better
use this instead of attr.sample_type in is_sampling_event()? perf_event_overflow() could be used then to generate output also for samples where no period is specified.

Quoted 2 lines

You're going to have to spell things out for me, I'm really not getting your argument.

I was thinking about to change this check and haven't seen cases for that the check is for. What would happen if the check isn't there and perf_event_overflow() is called from the interrupt handler?

Quoted 9 lines

Quoted 8 lines

Anyway, would the following extentension of the check above ok?

	if (unlikely(!is_sampling_event(event) && !event->attr.sample_type))
		...

With no bits set in attr.sample_type the sample would be empty and nothing to report. Now, with this change, samples that have data to report wouldn't be dropped anymore.

Quoted 2 lines

Also, could you explain in what way data is dropped? Where do non-sampling events need to write sample data?

I stumbled over this while rebasing my perf ibs patches:

http://git.kernel.org/?p=linux/kernel/git/rric/oprofile.git;a=shortlog;h=refs/heads/perf-ibs

Hope I could explain this to you better now.

Quoted 4 lines · Robert Richter

Quoted 3 lines

I'm probably not quite getting what you mean, but how is is_sampling_event() meaningless? the INT bit is enabled for _all_ events, whether they were configured as a sampling event or not.

Quoted 2 lines

Aren't all events that are mapped to counters via cpu_hw_events always sampling events?

No. perf stat which only counts (has period==0) uses hardware counters just fine but doesn't sample anything, yet has the INT bit set (as explained a few emails back).

Quoted 3 lines

Then, when calling perf_event_overflow() from an interrupt handler there are no other events than sampling events.

Thus false. (Also, even if we didn't always set the INT bit, it might see the overflow of a non-sampling event while dealing with the PMI triggered by another event).

Quoted 4 lines

Quoted 2 lines

Its just that for !sampling events we shouldn't attempt to generate any output.

If attr.sample_type is null, there is no output to generate.

Arguably true, currently we would still write a rudimentary sample, consisting of just the header with a 0-sized payload. This is, I'd rather we do that than add yet another conditional on the sample fast path. If the user doesn't want samples he should've set period==0, if the does he had better set a non-zero sample_type.

Quoted 4 lines

Better
use this instead of attr.sample_type in is_sampling_event()? perf_event_overflow() could be used then to generate output also for samples where no period is specified.

But what for? period==0 is defined as: does not generate samples.

Quoted 6 lines

Quoted 2 lines

You're going to have to spell things out for me, I'm really not getting your argument.

I was thinking about to change this check and haven't seen cases for that the check is for. What would happen if the check isn't there and perf_event_overflow() is called from the interrupt handler?

It might generate spurious samples, nothing too bad, just unexpected.

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

Anyway, would the following extentension of the check above ok?

	if (unlikely(!is_sampling_event(event) && !event->attr.sample_type))
		...

Quoted 6 lines

Quoted 3 lines

With no bits set in attr.sample_type the sample would be empty and nothing to report. Now, with this change, samples that have data to report wouldn't be dropped anymore.

Also, could you explain in what way data is dropped? Where do non-sampling events need to write sample data?

I stumbled over this while rebasing my perf ibs patches:

Quoted 3 lines

http://git.kernel.org/?p=linux/kernel/git/rric/oprofile.git;a=shortlog;h=refs/heads/perf-ibs

Hope I could explain this to you better now.

Could you point where exactly in the IBS code this happens? Even for IBS, if period==0 it should not generate samples. Arguably IBS with period==0 is pretty pointless, but that's another story.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

It covers the case there the sampling period may be specified in the raw config register (line 286). I know, this is not the preferred way, but this way you can directly set the config value in perf without bypassing some bits through event->attr. As IBS events are sampling events I will change the code so that it writes back the specified period to event->attr.sample_period/event->hw.sample_period. Something like:

        if (event->hw.sample_period) {
                ...
        } else {
                max_cnt = event->attr.config & map->cnt_mask;
                event->attr.sample_period = max_cnt << 4;
                event->hw.sample_period = event->attr.sample_period;
        }

This will then proper identify IBS samples as samling event.

Quoted 17 lines · Robert Richter

It covers the case there the sampling period may be specified in the raw config register (line 286). I know, this is not the preferred way, but this way you can directly set the config value in perf without bypassing some bits through event->attr. As IBS events are sampling events I will change the code so that it writes back the specified period to event->attr.sample_period/event->hw.sample_period. Something like:

        if (event->hw.sample_period) {
                ...
        } else {
                max_cnt = event->attr.config & map->cnt_mask;
                event->attr.sample_period = max_cnt << 4;
                event->hw.sample_period = event->attr.sample_period;
        }

Quoted 1 line

This will then proper identify IBS samples as samling event.

Ah, being a naught boy then ;-), yeah passing stuff through like that will confuse bits and the fixup you propose will indeed work around that.

But why complicate things like this? You just add extra code to the init path since you need to support attr.sample_period input as well.