ring-buffer: Making persistent ring buffer robust
Subsystems (2)
Masami Hiramatsu (Google)
maintainerv3 cover ring-buffer: Making persistent ring buffer robust
Hi,
Here is the 3rd version of improvement patches for making persistent ring buffers robust to failures. This fixes some issues of persistent ring buffer on real machines.
The previous version is here:
In this version, I rebased the series on top of trace/fixes branch (thus the first patch has been merged.)
I updated the description [1/3], used RB_MISSED_EVENTS flag to indicate the corrupted page [3/3] and I found a new bug on using rb_data_page::commit as index. So added a new fix [2/3].
Thank you,
---
Masami Hiramatsu (Google) (3):
ring-buffer: Flush and stop persistent ring buffer on panic
ring-buffer: Handle RB_MISSED_* flags on commit field correctly
ring-buffer: Skip invalid sub-buffers when validating persistent ring buffer
kernel/trace/ring_buffer.c | 72 +++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 25 deletions(-)
--
Masami Hiramatsu (Google) <[email protected]>
Masami Hiramatsu (Google)
maintainerPATCH v3 1/3 ring-buffer: Flush and stop persistent ring buffer on panic
From: Masami Hiramatsu (Google) <[email protected]>
On real hardware, panic and machine reboot may not flush hardware cache to memory. This means the persistent ring buffer, which relies on a coherent state of memory, may not have its events written to the buffer and they may be lost. Moreover, there may be inconsistency with the counters which are used for validation of the integrity of the persistent ring buffer which may cause all data to be discarded.
To avoid this issue, stop recording of the ring buffer on panic and flush the cache of the ring buffer's memory.
--- kernel/trace/ring_buffer.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
kernel/trace/ring_buffer.c +21 -0
@@ -6,6 +6,7 @@*/ #include <linux/sched/isolation.h> #include <linux/trace_recursion.h> +#include <linux/panic_notifier.h> #include <linux/trace_events.h> #include <linux/ring_buffer.h> #include <linux/trace_clock.h>
@@ -589,6 +590,7 @@ struct trace_buffer {unsigned long range_addr_start;unsigned long range_addr_end; +struct notifier_block flush_nb;struct ring_buffer_meta *meta;
@@ -2471,6 +2473,16 @@ static void rb_free_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)kfree (cpu_buffer); } +static int rb_flush_buffer_cb (struct notifier_block *nb,unsigned long event,void *data) +{ + struct trace_buffer *buffer = container_of(nb, struct trace_buffer, flush_nb); + +ring_buffer_record_disable (buffer); +flush_kernel_vmap_range ((void *)buffer->range_addr_start, + buffer->range_addr_end - buffer->range_addr_start); +return NOTIFY_DONE ; +} + static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags, int order, unsigned long start, unsigned long end,
@@ -2590,6 +2602,12 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,mutex_init (&buffer->mutex); +/* Persistent ring buffer needs to flush cache before reboot. */ +if (start & end) { + buffer->flush_nb.notifier_call = rb_flush_buffer_cb; +atomic_notifier_chain_register (&panic_notifier_list, &buffer->flush_nb); + } +return_ptr (buffer); fail_free_buffers:
@@ -2677,6 +2695,9 @@ ring_buffer_free(struct trace_buffer *buffer) {int cpu; +if (buffer->range_addr_start && buffer->range_addr_end) +atomic_notifier_chain_unregister (&panic_notifier_list, &buffer->flush_nb); +cpuhp_state_remove_instance (CPUHP_TRACE_RB_PREPARE , &buffer->node);irq_work_sync (&buffer->irq_work.work);
Masami Hiramatsu (Google)
maintainerPATCH v3 2/3 ring-buffer: Handle RB_MISSED_* flags on commit field correctly
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Since the MSBs of rb_data_page::commit are used for storing RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits when it is used for finding the size of data pages.
---
Changes in v3:
- Newly added.
--- kernel/trace/ring_buffer.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
kernel/trace/ring_buffer.c +16 -16
@@ -395,6 +395,18 @@ static __always_inline unsigned int rb_page_commit(struct buffer_page *bpage)return local_read (&bpage->page->commit); } +/* Size is determined by what has been committed */ +static __always_inline unsigned rb_page_size (struct buffer_page *bpage) +{ +return rb_page_commit (bpage) & ~RB_MISSED_MASK ; +} + +static __always_inline unsigned +rb_commit_index (struct ring_buffer_per_cpu *cpu_buffer) +{ +return rb_page_commit (cpu_buffer->commit_page); +} +static void free_buffer_page (struct buffer_page *bpage) {/* Range pages are not to be freed */
@@ -1907,7 +1919,7 @@ static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)u64 delta;int tail; - tail =local_read (&dpage->commit); + tail =local_read (&dpage->commit) & ~RB_MISSED_MASK ;return rb_read_data_buffer (dpage, tail, cpu, &ts, &delta); }
@@ -1934,7 +1946,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer) goto invalid; } entries += ret; - entry_bytes +=local_read (&cpu_buffer->reader_page->page->commit); + entry_bytes +=rb_page_size (cpu_buffer->reader_page);local_set (&cpu_buffer->reader_page->entries, ret); ts = head_page->page->time_stamp;
@@ -2054,7 +2066,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)local_inc (&cpu_buffer->pages_touched); entries += ret; - entry_bytes +=local_read (&head_page->page->commit); + entry_bytes +=rb_page_size (head_page);local_set (&cpu_buffer->head_page->entries, ret);if (head_page == cpu_buffer->commit_page)
@@ -3257,18 +3269,6 @@ rb_iter_head_event(struct ring_buffer_iter *iter)return NULL ; } -/* Size is determined by what has been committed */ -static __always_inline unsigned rb_page_size (struct buffer_page *bpage) -{ -return rb_page_commit (bpage) & ~RB_MISSED_MASK ; -} - -static __always_inline unsigned -rb_commit_index (struct ring_buffer_per_cpu *cpu_buffer) -{ -return rb_page_commit (cpu_buffer->commit_page); -} -static __always_inline unsigned rb_event_index (struct ring_buffer_per_cpu *cpu_buffer,struct ring_buffer_event *event) {
@@ -4433,7 +4433,7 @@ static void check_buffer(struct ring_buffer_per_cpu *cpu_buffer,if (tail ==CHECK_FULL_PAGE ) { full = true; - tail =local_read (&bpage->commit); + tail =local_read (&bpage->commit) & ~RB_MISSED_MASK ; }else if (info->add_timestamp & (RB_ADD_STAMP_FORCE |RB_ADD_STAMP_ABSOLUTE )) {/* Ignore events with absolute time stamps */
Masami Hiramatsu (Google)
maintainerRe: [PATCH v3 2/3] ring-buffer: Handle RB_MISSED_* flags on commit field correctly
Quoted 69 lines · On Tue, 24 Feb 2026 01:16:11 +0900 "Masami Hiramatsu (Google)"…
From: Masami Hiramatsu (Google) <[email protected]>
Since the MSBs of
rb_data_page::commitare used for storingRB_MISSED_EVENTSandRB_MISSED_STORED, we need to mask out those bits when it is used for finding the size of data pages.Fixes 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")Fixes 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")Signed-off-by Masami Hiramatsu (Google) <[email protected]>---
Changes in v3:
- Newly added.--- kernel/trace/ring_buffer.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)kernel/trace/ring_buffer.c +15
-3@@ -395,6 +395,18 @@ static __always_inline unsigned int rb_page_commit(struct buffer_page *bpage)return local_read (&bpage->page->commit); } +/* Size is determined by what has been committed */ +static __always_inline unsigned rb_page_size (struct buffer_page *bpage) +{ +return rb_page_commit (bpage) & ~RB_MISSED_MASK ; +} + +static __always_inline unsigned +rb_commit_index (struct ring_buffer_per_cpu *cpu_buffer) +{ +return rb_page_commit (cpu_buffer->commit_page); +} +static void free_buffer_page (struct buffer_page *bpage) {/* Range pages are not to be freed */ @@ -1907,7 +1919,7 @@ static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)u64 delta;int tail; - tail =local_read (&dpage->commit); + tail =local_read (&dpage->commit) & ~RB_MISSED_MASK ;return rb_read_data_buffer (dpage, tail, cpu, &ts, &delta); }@@ -1934,7 +1946,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer) goto invalid; } entries += ret; - entry_bytes +=local_read (&cpu_buffer->reader_page->page->commit); + entry_bytes +=rb_page_size (cpu_buffer->reader_page);local_set (&cpu_buffer->reader_page->entries, ret); ts = head_page->page->time_stamp;@@ -2054,7 +2066,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)local_inc (&cpu_buffer->pages_touched); entries += ret; - entry_bytes +=local_read (&head_page->page->commit); + entry_bytes +=rb_page_size (head_page);local_set (&cpu_buffer->head_page->entries, ret);if (head_page == cpu_buffer->commit_page)@@ -3257,18 +3269,6 @@ rb_iter_head_event(struct ring_buffer_iter *iter)return NULL ;
kernel/trace/ring_buffer.c rb_iter_head_event(struct ring_buffer_iter *iter) · line 3267
Quoted 10 lines
kernel/trace/ring_buffer.c +0
-8} -/* Size is determined by what has been committed */ -static __always_inline unsigned rb_page_size (struct buffer_page *bpage) -{ -return rb_page_commit (bpage) & ~RB_MISSED_MASK ; -} - -static __always_inline unsigned -rb_commit_index (struct ring_buffer_per_cpu *cpu_buffer)
Oops, this data structure is defined after this line. Let me fix it.
Quoted 17 lines
kernel/trace/ring_buffer.c +1
-5-{ -return rb_page_commit (cpu_buffer->commit_page); -} -static __always_inline unsigned rb_event_index (struct ring_buffer_per_cpu *cpu_buffer,struct ring_buffer_event *event) {@@ -4433,7 +4433,7 @@ static void check_buffer(struct ring_buffer_per_cpu *cpu_buffer,if (tail ==CHECK_FULL_PAGE ) { full = true; - tail =local_read (&bpage->commit); + tail =local_read (&bpage->commit) & ~RB_MISSED_MASK ; }else if (info->add_timestamp & (RB_ADD_STAMP_FORCE |RB_ADD_STAMP_ABSOLUTE )) {/* Ignore events with absolute time stamps */
Signature · 1 line
Masami Hiramatsu (Google) <[email protected]>
Masami Hiramatsu (Google)
maintainerPATCH v3 3/3 ring-buffer: Skip invalid sub-buffers when validating persistent ring buffer
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Skip invalid sub-buffers when validating the persistent ring buffer instead of discarding the entire ring buffer. Also, mark there are missed events on the discarded buffer.
If the cache data in memory fails to be synchronized during a reboot, the persistent ring buffer may become partially corrupted, but other sub-buffers may still contain readable event data. Only discard the subbuffersa that ar found to be corrupted.
---
Changes in v3:
- Record missed data event on commit.
--- kernel/trace/ring_buffer.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
kernel/trace/ring_buffer.c +11 -10
@@ -2058,17 +2058,18 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)if (ret <0 ) {pr_info ("Ring buffer meta [%d] invalid buffer page\n" , cpu_buffer->cpu); - goto invalid; - } - -/* If the buffer has content, update pages_touched */ -if (ret) -local_inc (&cpu_buffer->pages_touched); - - entries += ret; - entry_bytes +=rb_page_size (head_page); -local_set (&cpu_buffer->head_page->entries, ret); +/* Instead of discard whole ring buffer, discard only this sub-buffer. */ +local_set (&head_page->entries,0 ); +local_set (&head_page->page->commit,RB_MISSED_EVENTS ); + }else { +/* If the buffer has content, update pages_touched */ +if (ret) +local_inc (&cpu_buffer->pages_touched); + entries += ret; + entry_bytes +=rb_page_size (head_page); +local_set (&cpu_buffer->head_page->entries, ret); + }if (head_page == cpu_buffer->commit_page)break ; }