* [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode
@ 2017-09-06 16:08 Alexander Shishkin
2017-09-12 22:37 ` Will Deacon
2017-09-29 9:28 ` [tip:perf/urgent] perf/aux: Only update ->aux_wakeup " tip-bot for Alexander Shishkin
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Shishkin @ 2017-09-06 16:08 UTC (permalink / raw)
To: Peter Zijlstra, will.deacon; +Cc: Ingo Molnar, linux-kernel, Alexander Shishkin
Commit d9a50b0256 ("perf/aux: Ensure aux_wakeup represents most recent
wakeup index") changed aux wakeup position calculation to rounddown(),
which causes a division-by-zero in AUX overwrite mode (aka "snapshot
mode").
The zero denominator results from the fact that perf record doesn't set
aux_watermark to anything, in which case the kernel will set it to half
the AUX buffer size, but only for non-overwrite mode. In the overwrite
mode aux_watermark stays zero.
The good news is that, AUX overwrite mode, wakeups don't happen and
related bookkeeping is not relevant, so we can simply forego the whole
wakeup updates.
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
kernel/events/ring_buffer.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index f4ebe42879..4dae1da4c1 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -412,6 +412,19 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
return NULL;
}
+static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
+{
+ if (rb->aux_overwrite)
+ return false;
+
+ if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
+ return true;
+ }
+
+ return false;
+}
+
/*
* Commit the data written by hardware into the ring buffer by adjusting
* aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
@@ -451,10 +464,8 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
}
rb->user_page->aux_head = rb->aux_head;
- if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ if (rb_need_aux_wakeup(rb))
wakeup = true;
- rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
- }
if (wakeup) {
if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
@@ -484,9 +495,8 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
rb->aux_head += size;
rb->user_page->aux_head = rb->aux_head;
- if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ if (rb_need_aux_wakeup(rb)) {
perf_output_wakeup(handle);
- rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode
2017-09-06 16:08 [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode Alexander Shishkin
@ 2017-09-12 22:37 ` Will Deacon
2017-09-13 10:34 ` Alexander Shishkin
2017-09-29 9:28 ` [tip:perf/urgent] perf/aux: Only update ->aux_wakeup " tip-bot for Alexander Shishkin
1 sibling, 1 reply; 6+ messages in thread
From: Will Deacon @ 2017-09-12 22:37 UTC (permalink / raw)
To: Alexander Shishkin; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel
Hi Alexander,
On Wed, Sep 06, 2017 at 07:08:11PM +0300, Alexander Shishkin wrote:
> Commit d9a50b0256 ("perf/aux: Ensure aux_wakeup represents most recent
> wakeup index") changed aux wakeup position calculation to rounddown(),
> which causes a division-by-zero in AUX overwrite mode (aka "snapshot
> mode").
>
> The zero denominator results from the fact that perf record doesn't set
> aux_watermark to anything, in which case the kernel will set it to half
> the AUX buffer size, but only for non-overwrite mode. In the overwrite
> mode aux_watermark stays zero.
>
> The good news is that, AUX overwrite mode, wakeups don't happen and
> related bookkeeping is not relevant, so we can simply forego the whole
> wakeup updates.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
> kernel/events/ring_buffer.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
Damn, sorry about that. How did you spot the problem?
Anyway, I think the code is much better with this factored out:
Acked-by: Will Deacon <will.deacon@arm.com>
Thanks,
Will
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index f4ebe42879..4dae1da4c1 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -412,6 +412,19 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
> return NULL;
> }
>
> +static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
> +{
> + if (rb->aux_overwrite)
> + return false;
> +
> + if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> + rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
> + return true;
> + }
> +
> + return false;
> +}
> +
> /*
> * Commit the data written by hardware into the ring buffer by adjusting
> * aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
> @@ -451,10 +464,8 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> }
>
> rb->user_page->aux_head = rb->aux_head;
> - if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> + if (rb_need_aux_wakeup(rb))
> wakeup = true;
> - rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
> - }
>
> if (wakeup) {
> if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> @@ -484,9 +495,8 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
> rb->aux_head += size;
>
> rb->user_page->aux_head = rb->aux_head;
> - if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> + if (rb_need_aux_wakeup(rb)) {
> perf_output_wakeup(handle);
> - rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
> handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
> }
>
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode
2017-09-12 22:37 ` Will Deacon
@ 2017-09-13 10:34 ` Alexander Shishkin
2017-09-28 11:29 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Shishkin @ 2017-09-13 10:34 UTC (permalink / raw)
To: Will Deacon; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel
Will Deacon <will.deacon@arm.com> writes:
> Hi Alexander,
>
> On Wed, Sep 06, 2017 at 07:08:11PM +0300, Alexander Shishkin wrote:
>> Commit d9a50b0256 ("perf/aux: Ensure aux_wakeup represents most recent
>> wakeup index") changed aux wakeup position calculation to rounddown(),
>> which causes a division-by-zero in AUX overwrite mode (aka "snapshot
>> mode").
>>
>> The zero denominator results from the fact that perf record doesn't set
>> aux_watermark to anything, in which case the kernel will set it to half
>> the AUX buffer size, but only for non-overwrite mode. In the overwrite
>> mode aux_watermark stays zero.
>>
>> The good news is that, AUX overwrite mode, wakeups don't happen and
>> related bookkeeping is not relevant, so we can simply forego the whole
>> wakeup updates.
>>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> ---
>> kernel/events/ring_buffer.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> Damn, sorry about that. How did you spot the problem?
A normal perf record with -S should trigger it, I don't remember what
exactly I was doing at that moment. But no worries, we all missed it the
first time around. :)
> Anyway, I think the code is much better with this factored out:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode
2017-09-13 10:34 ` Alexander Shishkin
@ 2017-09-28 11:29 ` Will Deacon
2017-09-28 11:40 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2017-09-28 11:29 UTC (permalink / raw)
To: Alexander Shishkin; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel
Hi Alexander,
On Wed, Sep 13, 2017 at 01:34:03PM +0300, Alexander Shishkin wrote:
> Will Deacon <will.deacon@arm.com> writes:
> > On Wed, Sep 06, 2017 at 07:08:11PM +0300, Alexander Shishkin wrote:
> >> Commit d9a50b0256 ("perf/aux: Ensure aux_wakeup represents most recent
> >> wakeup index") changed aux wakeup position calculation to rounddown(),
> >> which causes a division-by-zero in AUX overwrite mode (aka "snapshot
> >> mode").
> >>
> >> The zero denominator results from the fact that perf record doesn't set
> >> aux_watermark to anything, in which case the kernel will set it to half
> >> the AUX buffer size, but only for non-overwrite mode. In the overwrite
> >> mode aux_watermark stays zero.
> >>
> >> The good news is that, AUX overwrite mode, wakeups don't happen and
> >> related bookkeeping is not relevant, so we can simply forego the whole
> >> wakeup updates.
> >>
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> ---
> >> kernel/events/ring_buffer.c | 20 +++++++++++++++-----
> >> 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > Damn, sorry about that. How did you spot the problem?
>
> A normal perf record with -S should trigger it, I don't remember what
> exactly I was doing at that moment. But no worries, we all missed it the
> first time around. :)
>
> > Anyway, I think the code is much better with this factored out:
> >
> > Acked-by: Will Deacon <will.deacon@arm.com>
>
> Thanks!
What's the status on this patch? I don't see it in mainline or linux-next,
but it would ideally get in as a fix for 4.14.
Will
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode
2017-09-28 11:29 ` Will Deacon
@ 2017-09-28 11:40 ` Peter Zijlstra
0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2017-09-28 11:40 UTC (permalink / raw)
To: Will Deacon; +Cc: Alexander Shishkin, Ingo Molnar, linux-kernel
On Thu, Sep 28, 2017 at 12:29:15PM +0100, Will Deacon wrote:
> What's the status on this patch? I don't see it in mainline or linux-next,
> but it would ideally get in as a fix for 4.14.
Its in the patch queue I'm trying to get mingo to merge ;-) It is indeed
queued for /urgent.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] perf/aux: Only update ->aux_wakeup in non-overwrite mode
2017-09-06 16:08 [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode Alexander Shishkin
2017-09-12 22:37 ` Will Deacon
@ 2017-09-29 9:28 ` tip-bot for Alexander Shishkin
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Alexander Shishkin @ 2017-09-29 9:28 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, torvalds, tglx, alexander.shishkin, mingo, peterz, hpa
Commit-ID: 441430eb54a00586f95f1aefc48e0801bbd6a923
Gitweb: https://git.kernel.org/tip/441430eb54a00586f95f1aefc48e0801bbd6a923
Author: Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Wed, 6 Sep 2017 19:08:11 +0300
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 29 Sep 2017 10:06:45 +0200
perf/aux: Only update ->aux_wakeup in non-overwrite mode
The following commit:
d9a50b0256 ("perf/aux: Ensure aux_wakeup represents most recent wakeup index")
changed the AUX wakeup position calculation to rounddown(), which causes
a division-by-zero in AUX overwrite mode (aka "snapshot mode").
The zero denominator results from the fact that perf record doesn't set
aux_watermark to anything, in which case the kernel will set it to half
the AUX buffer size, but only for non-overwrite mode. In the overwrite
mode aux_watermark stays zero.
The good news is that, AUX overwrite mode, wakeups don't happen and
related bookkeeping is not relevant, so we can simply forego the whole
wakeup updates.
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: will.deacon@arm.com
Link: http://lkml.kernel.org/r/20170906160811.16510-1-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/ring_buffer.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index af71a84e..f684d8e 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -412,6 +412,19 @@ err:
return NULL;
}
+static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
+{
+ if (rb->aux_overwrite)
+ return false;
+
+ if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
+ return true;
+ }
+
+ return false;
+}
+
/*
* Commit the data written by hardware into the ring buffer by adjusting
* aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
@@ -451,10 +464,8 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
}
rb->user_page->aux_head = rb->aux_head;
- if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ if (rb_need_aux_wakeup(rb))
wakeup = true;
- rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
- }
if (wakeup) {
if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
@@ -484,9 +495,8 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
rb->aux_head += size;
rb->user_page->aux_head = rb->aux_head;
- if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) {
+ if (rb_need_aux_wakeup(rb)) {
perf_output_wakeup(handle);
- rb->aux_wakeup = rounddown(rb->aux_head, rb->aux_watermark);
handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-29 9:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 16:08 [PATCH] perf/aux: Only update aux_wakeup in non-overwrite mode Alexander Shishkin
2017-09-12 22:37 ` Will Deacon
2017-09-13 10:34 ` Alexander Shishkin
2017-09-28 11:29 ` Will Deacon
2017-09-28 11:40 ` Peter Zijlstra
2017-09-29 9:28 ` [tip:perf/urgent] perf/aux: Only update ->aux_wakeup " tip-bot for Alexander Shishkin
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.