Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1262367
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | [for-next][PATCH 07/13] ring_buffer: Do no not complete benchmark reader too early |
| Date | 2015-11-04 16:40 +0100 |
| Message-ID | <qr8ki-2go-37@gated-at.bofh.it> (permalink) |
| References | <qr8kh-2go-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
From: Petr Mladek <pmladek@suse.com>
It seems that complete(&read_done) might be called too early
in some situations.
1st scenario:
-------------
CPU0 CPU1
ring_buffer_producer_thread()
wake_up_process(consumer);
wait_for_completion(&read_start);
ring_buffer_consumer_thread()
complete(&read_start);
ring_buffer_producer()
# producing data in
# the do-while cycle
ring_buffer_consumer();
# reading data
# got error
# set kill_test = 1;
set_current_state(
TASK_INTERRUPTIBLE);
if (reader_finish) # false
schedule();
# producer still in the middle of
# do-while cycle
if (consumer && !(cnt % wakeup_interval))
wake_up_process(consumer);
# spurious wakeup
while (!reader_finish &&
!kill_test)
# leaving because
# kill_test == 1
reader_finish = 0;
complete(&read_done);
1st BANG: We might access uninitialized "read_done" if this is the
the first round.
# producer finally leaving
# the do-while cycle because kill_test == 1;
if (consumer) {
reader_finish = 1;
wake_up_process(consumer);
wait_for_completion(&read_done);
2nd BANG: This will never complete because consumer already did
the completion.
2nd scenario:
-------------
CPU0 CPU1
ring_buffer_producer_thread()
wake_up_process(consumer);
wait_for_completion(&read_start);
ring_buffer_consumer_thread()
complete(&read_start);
ring_buffer_producer()
# CPU3 removes the module <--- difference from
# and stops producer <--- the 1st scenario
if (kthread_should_stop())
kill_test = 1;
ring_buffer_consumer();
while (!reader_finish &&
!kill_test)
# kill_test == 1 => we never go
# into the top level while()
reader_finish = 0;
complete(&read_done);
# producer still in the middle of
# do-while cycle
if (consumer && !(cnt % wakeup_interval))
wake_up_process(consumer);
# spurious wakeup
while (!reader_finish &&
!kill_test)
# leaving because kill_test == 1
reader_finish = 0;
complete(&read_done);
BANG: We are in the same "bang" situations as in the 1st scenario.
Root of the problem:
--------------------
ring_buffer_consumer() must complete "read_done" only when "reader_finish"
variable is set. It must not be skipped due to other conditions.
Note that we still must keep the check for "reader_finish" in a loop
because there might be spurious wakeups as described in the
above scenarios.
Solution:
----------
The top level cycle in ring_buffer_consumer() will finish only when
"reader_finish" is set. The data will be read in "while-do" cycle
so that they are not read after an error (kill_test == 1)
or a spurious wake up.
In addition, "reader_finish" is manipulated by the producer thread.
Therefore we add READ_ONCE() to make sure that the fresh value is
read in each cycle. Also we add the corresponding barrier
to synchronize the sleep check.
Next we set the state back to TASK_RUNNING for the situation where we
did not sleep.
Just from paranoid reasons, we initialize both completions statically.
This is safer, in case there are other races that we are unaware of.
As a side effect we could remove the memory barrier from
ring_buffer_producer_thread(). IMHO, this was the reason for
the barrier. ring_buffer_reset() uses spin locks that should
provide the needed memory barrier for using the buffer.
Link: http://lkml.kernel.org/r/1441629518-32712-2-git-send-email-pmladek@suse.com
Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
kernel/trace/ring_buffer_benchmark.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index a1503a027ee2..9ea7949366b3 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -24,8 +24,8 @@ struct rb_page {
static int wakeup_interval = 100;
static int reader_finish;
-static struct completion read_start;
-static struct completion read_done;
+static DECLARE_COMPLETION(read_start);
+static DECLARE_COMPLETION(read_done);
static struct ring_buffer *buffer;
static struct task_struct *producer;
@@ -178,10 +178,14 @@ static void ring_buffer_consumer(void)
read_events ^= 1;
read = 0;
- while (!reader_finish && !kill_test) {
- int found;
+ /*
+ * Continue running until the producer specifically asks to stop
+ * and is ready for the completion.
+ */
+ while (!READ_ONCE(reader_finish)) {
+ int found = 1;
- do {
+ while (found && !kill_test) {
int cpu;
found = 0;
@@ -195,17 +199,23 @@ static void ring_buffer_consumer(void)
if (kill_test)
break;
+
if (stat == EVENT_FOUND)
found = 1;
+
}
- } while (found && !kill_test);
+ }
+ /* Wait till the producer wakes us up when there is more data
+ * available or when the producer wants us to finish reading.
+ */
set_current_state(TASK_INTERRUPTIBLE);
if (reader_finish)
break;
schedule();
}
+ __set_current_state(TASK_RUNNING);
reader_finish = 0;
complete(&read_done);
}
@@ -389,13 +399,10 @@ static int ring_buffer_consumer_thread(void *arg)
static int ring_buffer_producer_thread(void *arg)
{
- init_completion(&read_start);
-
while (!kthread_should_stop() && !kill_test) {
ring_buffer_reset(buffer);
if (consumer) {
- smp_wmb();
wake_up_process(consumer);
wait_for_completion(&read_start);
}
--
2.6.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[for-next][PATCH 00/13] tracing: Some more last minute cleanups and fixes for 4.4 Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 13/13] tracing: Put back comma for empty fields in boot string parsing Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 05/13] tracing: Rename max_stack_lock to stack_trace_max_lock Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 12/13] tracing: Apply tracer specific options from kernel command line. Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 09/13] tracing: Allow dumping traces without tracking trace started cpus Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 02/13] recordmcount: Fix endianness handling bug for nop_mcount Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 07/13] ring_buffer: Do no not complete benchmark reader too early Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 06/13] tracing: Remove redundant TP_ARGS redefining Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 08/13] ring_buffer: Fix more races when terminating the producer in the benchmark Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 10/13] ring_buffer: Remove unneeded smp_wmb() before wakeup of reader benchmark Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 04/13] tracing: Allow arch-specific stack tracer Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 03/13] recordmcount: arm64: Replace the ignored mcount call into nop Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100 [for-next][PATCH 01/13] tracepoints: Fix documentation of RCU lockdep checks Steven Rostedt <rostedt@goodmis.org> - 2015-11-04 16:40 +0100
csiph-web