Re: [PATCH v5] ring-buffer: Ensure proper resetting of atomic variables in ring_buffer_reset_online_cpus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ran afoul of the taboo filters:

Illegal-Object: Syntax error in From: address found on vger.kernel.org:
        From:   Tze-nan.Wu<Tze-nan.Wu@xxxxxxxxxxxx>
                                  ^-missing end of mailbox

I've seen a few of these but I've been unable to replicate the problem. I've a suspicion but so far been unable to prove the theory on what's wrong.

- John

On 4/25/23 18:17, Steven Rostedt wrote:

For some reason, this email did not make it to
linux-trace-kernel@xxxxxxxxxxxxxxx, and therefore did not make it into
patchwork?

John?

-- Steve


On Wed, 26 Apr 2023 09:04:44 +0800
Tze-nan.Wu <Tze-nan.Wu@xxxxxxxxxxxx> wrote:

From: "Tze-nan Wu" <Tze-nan.Wu@xxxxxxxxxxxx>

In ring_buffer_reset_online_cpus, the buffer_size_kb write operation
may permanently fail if the cpu_online_mask changes between two
for_each_online_buffer_cpu loops. The number of increases and decreases
on both cpu_buffer->resize_disabled and cpu_buffer->record_disabled may be
inconsistent, causing some CPUs to have non-zero values for these atomic
variables after the function returns.

This issue can be reproduced by "echo 0 > trace" while hotplugging cpu.
After reproducing success, we can find out buffer_size_kb will not be
functional anymore.

To prevent leaving 'resize_disabled' and 'record_disabled' non-zero after
ring_buffer_reset_online_cpus returns, we ensure that each atomic variable
has been set up before atomic_sub() to it.

Cc: stable@xxxxxxxxxxxxxxx
Cc: npiggin@xxxxxxxxx
Fixes: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU")
Reviewed-by: Cheng-Jui Wang <cheng-jui.wang@xxxxxxxxxxxx>
Signed-off-by: Tze-nan Wu <Tze-nan.Wu@xxxxxxxxxxxx>
---
Changes from v4 to v5: https://lore.kernel.org/lkml/20230412112401.25081-1-Tze-nan.Wu@xxxxxxxxxxxx/
   - Move the define before the function
---
  kernel/trace/ring_buffer.c | 16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 76a2d91eecad..253ef85a9ec3 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -5345,6 +5345,9 @@ void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu)
  }
  EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
+/* Flag to ensure proper resetting of atomic variables */
+#define RESET_BIT	(1 << 30)
+
  /**
   * ring_buffer_reset_online_cpus - reset a ring buffer per CPU buffer
   * @buffer: The ring buffer to reset a per cpu buffer of
@@ -5361,20 +5364,27 @@ void ring_buffer_reset_online_cpus(struct trace_buffer *buffer)
  	for_each_online_buffer_cpu(buffer, cpu) {
  		cpu_buffer = buffer->buffers[cpu];
- atomic_inc(&cpu_buffer->resize_disabled);
+		atomic_add(RESET_BIT, &cpu_buffer->resize_disabled);
  		atomic_inc(&cpu_buffer->record_disabled);
  	}
/* Make sure all commits have finished */
  	synchronize_rcu();
- for_each_online_buffer_cpu(buffer, cpu) {
+	for_each_buffer_cpu(buffer, cpu) {
  		cpu_buffer = buffer->buffers[cpu];
+ /*
+		 * If a CPU came online during the synchronize_rcu(), then
+		 * ignore it.
+		 */
+		if (!(atomic_read(&cpu_buffer->resize_disabled) & RESET_BIT))
+			continue;
+
  		reset_disabled_cpu_buffer(cpu_buffer);
atomic_dec(&cpu_buffer->record_disabled);
-		atomic_dec(&cpu_buffer->resize_disabled);
+		atomic_sub(RESET_BIT, &cpu_buffer->resize_disabled);
  	}
mutex_unlock(&buffer->mutex);




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux