perf ring-buffer fixes
Peter Zijlstra
maintainercover perf ring-buffer fixes
This is basically a split of Yabin's last patch.
Peter Zijlstra
maintainerPATCH 1/4 perf/ring_buffer: Fix exposing a temporarily decreased data_head.
In perf_output_put_handle(), an IRQ/NMI can happen in below location and write records to the same ring buffer:
... local_dec_and_test(&rb->nest) ... <-- an IRQ/NMI can happen here rb->user_page->data_head = head; ...
In this case, a value A is written to data_head in the IRQ, then a value B is written to data_head after the IRQ. And A > B. As a result, data_head is temporarily decreased from A to B. And a reader may see data_head < data_tail if it read the buffer frequently enough, which creates unexpected behaviors.
This can be fixed by moving dec(&rb->nest) to after updating data_head, which prevents the IRQ/NMI above from updating data_head.
--- kernel/events/ring_buffer.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
kernel/events/ring_buffer.c +20 -4
@@ -51,11 +51,18 @@ static void perf_output_put_handle(struc head =local_read (&rb->head);/* -* IRQ/NMI can happen here, which means we can miss a head update. +* IRQ/NMI can happen here and advance @rb->head, causing our +* load above to be stale. */ -if (!local_dec_and_test (&rb->nest)) +/* +* If this isn't the outermost nesting, we don't have to update +* @rb->user_page->data_head. +*/ +if (local_read (&rb->nest) >1 ) { +local_dec (&rb->nest); goto out; + }/* * Since the mmap() consumer (userspace) can run on a different CPU:
@@ -87,9 +94,18 @@ static void perf_output_put_handle(struc rb->user_page->data_head = head;/* -* Now check if we missed an update -- rely on previous implied -* compiler barriers to force a re-read. +* We must publish the head before decrementing the nest count, +* otherwise an IRQ/NMI can publish a more recent head value and our +* write will (temporarily) publish a stale value. +*/ +barrier (); +local_set (&rb->nest,0 ); + +/* +* Ensure we decrement @rb->nest before we validate the @rb->head. +* Otherwise we cannot be sure we caught the 'last' nested update. */ +barrier ();if (unlikely (head !=local_read (&rb->head))) {local_inc (&rb->nest); goto again;
Re: [PATCH 1/4] perf/ring_buffer: Fix exposing a temporarily decreased data_head.
Quoted 17 lines · Peter Zijlstra
In
perf_output_put_handle(), an IRQ/NMI can happen in below location and write records to the same ring buffer:... local_dec_and_test(&rb->nest) ... <-- an IRQ/NMI can happen here rb->user_page->data_head = head; ...In this case, a value A is written to
data_headin the IRQ, then a value B is written todata_headafter the IRQ. And A > B. As a result,data_headis temporarily decreased from A to B. And a reader may seedata_head<data_tailif it read the buffer frequently enough, which creates unexpected behaviors.This can be fixed by moving
dec(&rb->nest)to after updatingdata_head, which prevents the IRQ/NMI above from updatingdata_head.
Quoted 9 lines
Cc Arnaldo Carvalho de Melo <[email protected]>Cc Ingo Molnar <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Alexander Shishkin <[email protected]>Fixes ef60777c9abd ("perf: Optimize the perf_output() path by removing IRQ-disables")Signed-off-by Yabin Cui <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
So these are missing a bunch of:
From: Yabin Cui <[email protected]>
lines, right?
Thanks,
Ingo
Peter Zijlstra
maintainerRe: [PATCH 1/4] perf/ring_buffer: Fix exposing a temporarily decreased data_head.
Quoted 30 lines · Ingo Molnar
Quoted 17 lines · Peter Zijlstra
In
perf_output_put_handle(), an IRQ/NMI can happen in below location and write records to the same ring buffer:... local_dec_and_test(&rb->nest) ... <-- an IRQ/NMI can happen here rb->user_page->data_head = head; ...In this case, a value A is written to
data_headin the IRQ, then a value B is written todata_headafter the IRQ. And A > B. As a result,data_headis temporarily decreased from A to B. And a reader may seedata_head<data_tailif it read the buffer frequently enough, which creates unexpected behaviors.This can be fixed by moving
dec(&rb->nest)to after updatingdata_head, which prevents the IRQ/NMI above from updatingdata_head.Quoted 9 lines
Cc Arnaldo Carvalho de Melo <[email protected]>Cc Ingo Molnar <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Alexander Shishkin <[email protected]>Fixes ef60777c9abd ("perf: Optimize the perf_output() path by removing IRQ-disables")Signed-off-by Yabin Cui <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Quoted 6 lines
So these are missing a bunch of:
From: Yabin Cui <[email protected]>
lines, right?
The first. certainly, the rest, while inspired by his patch, is more complete than what he did.
Re: [PATCH 1/4] perf/ring_buffer: Fix exposing a temporarily decreased data_head.
Quoted 33 lines · Peter Zijlstra
Quoted 32 lines · Ingo Molnar
Quoted 17 lines · Peter Zijlstra
In
perf_output_put_handle(), an IRQ/NMI can happen in below location and write records to the same ring buffer:... local_dec_and_test(&rb->nest) ... <-- an IRQ/NMI can happen here rb->user_page->data_head = head; ...In this case, a value A is written to
data_headin the IRQ, then a value B is written todata_headafter the IRQ. And A > B. As a result,data_headis temporarily decreased from A to B. And a reader may seedata_head<data_tailif it read the buffer frequently enough, which creates unexpected behaviors.This can be fixed by moving
dec(&rb->nest)to after updatingdata_head, which prevents the IRQ/NMI above from updatingdata_head.Quoted 9 lines
Cc Arnaldo Carvalho de Melo <[email protected]>Cc Ingo Molnar <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Alexander Shishkin <[email protected]>Fixes ef60777c9abd ("perf: Optimize the perf_output() path by removing IRQ-disables")Signed-off-by Yabin Cui <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>So these are missing a bunch of:
Quoted 7 lines
Quoted 4 lines
From: Yabin Cui <[email protected]>
lines, right?
The first. certainly, the rest, while inspired by his patch, is more complete than what he did.
Oh, indeed - I adjusted the first patch only.
Thanks,
Ingo
tip-bot for Yabin Cui
merge bot[tip:perf/urgent] perf/ring_buffer: Fix exposing a temporarily decreased data_head
In reply to Peter Zijlstraperf/ring_buffer: Fix exposing a temporarily decreased data_head
Commit-ID: 1b038c6e05ff70a1e66e3e571c2e6106bdb75f53 Gitweb: https://git.kernel.org/tip/1b038c6e05ff70a1e66e3e571c2e6106bdb75f53 Author: Yabin Cui <[email protected]> AuthorDate: Fri, 17 May 2019 13:52:31 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 24 May 2019 09:00:10 +0200
perf/ring_buffer: Fix exposing a temporarily decreased data_head
In perf_output_put_handle(), an IRQ/NMI can happen in below location and write records to the same ring buffer:
... local_dec_and_test(&rb->nest) ... <-- an IRQ/NMI can happen here rb->user_page->data_head = head; ...
In this case, a value A is written to data_head in the IRQ, then a value B is written to data_head after the IRQ. And A > B. As a result, data_head is temporarily decreased from A to B. And a reader may see data_head < data_tail if it read the buffer frequently enough, which creates unexpected behaviors.
This can be fixed by moving dec(&rb->nest) to after updating data_head, which prevents the IRQ/NMI above from updating data_head.
[ Split up by peterz. ]
--- kernel/events/ring_buffer.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
kernel/events/ring_buffer.c +20 -4
@@ -51,11 +51,18 @@ again: head =local_read (&rb->head);/* -* IRQ/NMI can happen here, which means we can miss a head update. +* IRQ/NMI can happen here and advance @rb->head, causing our +* load above to be stale. */ -if (!local_dec_and_test (&rb->nest)) +/* +* If this isn't the outermost nesting, we don't have to update +* @rb->user_page->data_head. +*/ +if (local_read (&rb->nest) >1 ) { +local_dec (&rb->nest); goto out; + }/* * Since the mmap() consumer (userspace) can run on a different CPU:
@@ -87,9 +94,18 @@ again: rb->user_page->data_head = head;/* -* Now check if we missed an update -- rely on previous implied -* compiler barriers to force a re-read. +* We must publish the head before decrementing the nest count, +* otherwise an IRQ/NMI can publish a more recent head value and our +* write will (temporarily) publish a stale value. +*/ +barrier (); +local_set (&rb->nest,0 ); + +/* +* Ensure we decrement @rb->nest before we validate the @rb->head. +* Otherwise we cannot be sure we caught the 'last' nested update. */ +barrier ();if (unlikely (head !=local_read (&rb->head))) {local_inc (&rb->nest); goto again;
Peter Zijlstra
maintainerPATCH 2/4 perf/ring_buffer: Add ordering to rb->nest increment
In reply to Peter ZijlstraSimilar to how decrementing rb->next too early can cause data_head to (temporarily) be observed to go backward, so too can this happen when we increment too late.
This barrier() ensures the rb->head load happens after the increment, both the one in the 'goto again' path, as the one from perf_output_get_handle() -- albeit very unlikely to matter for the latter.
--- kernel/events/ring_buffer.c | 9 +++++++++ 1 file changed, 9 insertions(+)
kernel/events/ring_buffer.c +9 -0
@@ -48,6 +48,15 @@ static void perf_output_put_handle(strucunsigned long head; again: +/* +* In order to avoid publishing a head value that goes backwards, +* we must ensure the load of @rb->head happens after we've +* incremented @rb->nest. +* +* Otherwise we can observe a @rb->head value before one published +* by an IRQ/NMI happening between the load and the increment. +*/ +barrier (); head =local_read (&rb->head);/*
tip-bot for Peter Zijlstra
merge bot[tip:perf/urgent] perf/ring_buffer: Add ordering to rb->nest increment
perf/ring_buffer: Add ordering to rb->nest increment
Commit-ID: 3f9fbe9bd86c534eba2faf5d840fd44c6049f50e Gitweb: https://git.kernel.org/tip/3f9fbe9bd86c534eba2faf5d840fd44c6049f50e Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 17 May 2019 13:52:32 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 24 May 2019 09:00:10 +0200
perf/ring_buffer: Add ordering to rb->nest increment
Similar to how decrementing rb->next too early can cause data_head to (temporarily) be observed to go backward, so too can this happen when we increment too late.
This barrier() ensures the rb->head load happens after the increment, both the one in the 'goto again' path, as the one from perf_output_get_handle() -- albeit very unlikely to matter for the latter.
--- kernel/events/ring_buffer.c | 9 +++++++++ 1 file changed, 9 insertions(+)
kernel/events/ring_buffer.c +9 -0
@@ -48,6 +48,15 @@ static void perf_output_put_handle(struct perf_output_handle *handle)unsigned long head; again: +/* +* In order to avoid publishing a head value that goes backwards, +* we must ensure the load of @rb->head happens after we've +* incremented @rb->nest. +* +* Otherwise we can observe a @rb->head value before one published +* by an IRQ/NMI happening between the load and the increment. +*/ +barrier (); head =local_read (&rb->head);/*
Peter Zijlstra
maintainerPATCH 3/4 perf/ring-buffer: Always use {READ,WRITE}_ONCE() for rb->user_page data
In reply to Peter ZijlstraWe must use {READ,WRITE}_ONCE() on rb->user_page data such that concurrent usage will see whole values. A few key sites were missing this.
--- kernel/events/ring_buffer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/events/ring_buffer.c +3 -3
@@ -100,7 +100,7 @@ static void perf_output_put_handle(struc* See perf_output_begin(). */ smp_wmb ();/* B, matches C */ - rb->user_page->data_head = head; +WRITE_ONCE (rb->user_page->data_head, head);/* * We must publish the head before decrementing the nest count,
@@ -496,7 +496,7 @@ void perf_aux_output_end(struct perf_outperf_event_aux_event (handle->event, aux_head, size, handle->aux_flags); - rb->user_page->aux_head = rb->aux_head; +WRITE_ONCE (rb->user_page->aux_head, rb->aux_head);if (rb_need_aux_wakeup (rb)) wakeup = true;
@@ -528,7 +528,7 @@ int perf_aux_output_skip(struct perf_out rb->aux_head += size; - rb->user_page->aux_head = rb->aux_head; +WRITE_ONCE (rb->user_page->aux_head, rb->aux_head);if (rb_need_aux_wakeup (rb)) {perf_output_wakeup (handle); handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
tip-bot for Peter Zijlstra
merge bot[tip:perf/urgent] perf/ring-buffer: Always use {READ,WRITE}_ONCE() for rb->user_page data
perf/ring-buffer: Always use {READ,WRITE}_ONCE() for rb->user_page data
Commit-ID: 4d839dd9e4356bbacf3eb0ab13a549b83b008c21 Gitweb: https://git.kernel.org/tip/4d839dd9e4356bbacf3eb0ab13a549b83b008c21 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 17 May 2019 13:52:33 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 24 May 2019 09:00:11 +0200
perf/ring-buffer: Always use {READ,WRITE}_ONCE() for rb->user_page data
We must use {READ,WRITE}_ONCE() on rb->user_page data such that concurrent usage will see whole values. A few key sites were missing this.
--- kernel/events/ring_buffer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/events/ring_buffer.c +3 -3
@@ -100,7 +100,7 @@ again:* See perf_output_begin(). */ smp_wmb ();/* B, matches C */ - rb->user_page->data_head = head; +WRITE_ONCE (rb->user_page->data_head, head);/* * We must publish the head before decrementing the nest count,
@@ -496,7 +496,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)perf_event_aux_event (handle->event, aux_head, size, handle->aux_flags); - rb->user_page->aux_head = rb->aux_head; +WRITE_ONCE (rb->user_page->aux_head, rb->aux_head);if (rb_need_aux_wakeup (rb)) wakeup = true;
@@ -528,7 +528,7 @@ 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; +WRITE_ONCE (rb->user_page->aux_head, rb->aux_head);if (rb_need_aux_wakeup (rb)) {perf_output_wakeup (handle); handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
Peter Zijlstra
maintainerPATCH 4/4 perf/ring-buffer: Use regular variables for nesting
In reply to Peter ZijlstraWhile the IRQ/NMI will nest, the nest-count will be invariant over the actual exception, since it will decrement equal to increment.
This means we can -- carefully -- use a regular variable since the typical LOAD-STORE race doesn't exist (similar to preempt_count).
This optimizes the ring-buffer for all LOAD-STORE architectures, since they need to use atomic ops to implement local_t.
--- kernel/events/internal.h | 4 ++-- kernel/events/ring_buffer.c | 41 ++++++++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 17 deletions(-)
kernel/events/internal.h +2 -2
@@ -24,7 +24,7 @@ struct ring_buffer {atomic_t poll;/* POLL_ for wakeups */ local_t head;/* write position */ -local_t nest;/* nested writers */ +unsigned int nest;/* nested writers */ local_t events;/* event limit */ local_t wakeup;/* wakeup stamp */ local_t lost;/* nr records lost */
@@ -41,7 +41,7 @@ struct ring_buffer {/* AUX area */ long aux_head; -local_t aux_nest; +unsigned int aux_nest;long aux_wakeup;/* last aux_watermark boundary crossed by aux_head */ unsigned long aux_pgoff;int aux_nr_pages;
kernel/events/ring_buffer.c +26 -15
101 lines not on this page · raw message
tip-bot for Peter Zijlstra
merge bot[tip:perf/urgent] perf/ring-buffer: Use regular variables for nesting
perf/ring-buffer: Use regular variables for nesting
Commit-ID: 5322ea58a06da2e69c5ef36a9b4d4b9255edd423 Gitweb: https://git.kernel.org/tip/5322ea58a06da2e69c5ef36a9b4d4b9255edd423 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 17 May 2019 13:52:34 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 24 May 2019 09:00:11 +0200
perf/ring-buffer: Use regular variables for nesting
While the IRQ/NMI will nest, the nest-count will be invariant over the actual exception, since it will decrement equal to increment.
This means we can -- carefully -- use a regular variable since the typical LOAD-STORE race doesn't exist (similar to preempt_count).
This optimizes the ring-buffer for all LOAD-STORE architectures, since they need to use atomic ops to implement local_t.
--- kernel/events/internal.h | 4 ++-- kernel/events/ring_buffer.c | 41 ++++++++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 17 deletions(-)
kernel/events/internal.h +2 -2
@@ -24,7 +24,7 @@ struct ring_buffer {atomic_t poll;/* POLL_ for wakeups */ local_t head;/* write position */ -local_t nest;/* nested writers */ +unsigned int nest;/* nested writers */ local_t events;/* event limit */ local_t wakeup;/* wakeup stamp */ local_t lost;/* nr records lost */
@@ -41,7 +41,7 @@ struct ring_buffer {/* AUX area */ long aux_head; -local_t aux_nest; +unsigned int aux_nest;long aux_wakeup;/* last aux_watermark boundary crossed by aux_head */ unsigned long aux_pgoff;int aux_nr_pages;
kernel/events/ring_buffer.c +26 -15
101 lines not on this page · raw message