From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Skip invalid sub-buffers when validating the persistent ring buffer
instead of discarding the entire ring buffer. Only skipped buffers
are invalidated (cleared).

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.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
  Changes in v7:
  - Combined with Handling RB_MISSED_* flags patch, focus on validation at boot.
  - Remove checking subbuffer data when validating metadata, because it should be done
    later.
  - Do not mark the discarded sub buffer page but just reset it.
  Changes in v6:
  - Show invalid page detection message once per CPU.
  Changes in v5:
  - Instead of showing errors for each page, just show the number
    of discarded pages at last.
  Changes in v3:
  - Record missed data event on commit.
---
 kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index b6f3ac99834f..8599de5cf59b 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -396,6 +396,12 @@ 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 int rb_page_size(struct buffer_page *bpage)
+{
+	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
+}
+
 static void free_buffer_page(struct buffer_page *bpage)
 {
 	/* Range pages are not to be freed */
@@ -1819,7 +1825,7 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
 
 	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
 
-	/* Is the meta buffers and the subbufs themselves have correct data? */
+	/* Is the meta buffers themselves have correct data? */
 	for (i = 0; i < meta->nr_subbufs; i++) {
 		if (meta->buffers[i] < 0 ||
 		    meta->buffers[i] >= meta->nr_subbufs) {
@@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
 			return false;
 		}
 
-		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
-			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
-			return false;
-		}
-
 		if (test_bit(meta->buffers[i], subbuf_mask)) {
 			pr_info("Ring buffer boot meta [%d] array has duplicates\n", cpu);
 			return false;
@@ -1902,13 +1903,16 @@ static int rb_read_data_buffer(struct buffer_data_page *dpage, int tail, int cpu
 	return events;
 }
 
-static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)
+static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu,
+			      struct ring_buffer_cpu_meta *meta)
 {
 	unsigned long long ts;
 	u64 delta;
 	int tail;
 
 	tail = local_read(&dpage->commit);
+	if (tail <= 0 || tail > meta->subbuf_size)
+		return -1;
 	return rb_read_data_buffer(dpage, tail, cpu, &ts, &delta);
 }
 
@@ -1919,6 +1923,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	struct buffer_page *head_page, *orig_head;
 	unsigned long entry_bytes = 0;
 	unsigned long entries = 0;
+	int discarded = 0;
 	int ret;
 	u64 ts;
 	int i;
@@ -1929,13 +1934,13 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	orig_head = head_page = cpu_buffer->head_page;
 
 	/* Do the reader page first */
-	ret = rb_validate_buffer(cpu_buffer->reader_page->page, cpu_buffer->cpu);
+	ret = rb_validate_buffer(cpu_buffer->reader_page->page, cpu_buffer->cpu, meta);
 	if (ret < 0) {
 		pr_info("Ring buffer reader page is invalid\n");
 		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;
@@ -1964,7 +1969,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 			break;
 
 		/* Stop rewind if the page is invalid. */
-		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
+		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu, meta);
 		if (ret < 0)
 			break;
 
@@ -2043,21 +2048,24 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 		if (head_page == cpu_buffer->reader_page)
 			continue;
 
-		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
+		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu, meta);
 		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 += local_read(&head_page->page->commit);
-		local_set(&cpu_buffer->head_page->entries, ret);
+			if (!discarded)
+				pr_info("Ring buffer meta [%d] invalid buffer page detected\n",
+					cpu_buffer->cpu);
+			discarded++;
+			/* 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;
 	}
@@ -2071,7 +2079,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	local_set(&cpu_buffer->entries, entries);
 	local_set(&cpu_buffer->entries_bytes, entry_bytes);
 
-	pr_info("Ring buffer meta [%d] is from previous boot!\n", cpu_buffer->cpu);
+	pr_info("Ring buffer meta [%d] is from previous boot! (%d pages discarded)\n",
+		cpu_buffer->cpu, discarded);
 	return;
 
  invalid:
@@ -3258,12 +3267,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)
 {

On Sat,  7 Mar 2026 23:26:38 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

>  kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index b6f3ac99834f..8599de5cf59b 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -396,6 +396,12 @@ 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 int rb_page_size(struct buffer_page *bpage)
> +{
> +	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
> +}
> +
>  static void free_buffer_page(struct buffer_page *bpage)
>  {
>  	/* Range pages are not to be freed */
> @@ -1819,7 +1825,7 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
>  
>  	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
>  
> -	/* Is the meta buffers and the subbufs themselves have correct data? */
> +	/* Is the meta buffers themselves have correct data? */

I just realized that the origin didn't have correct grammar. But we
still check the subbufs, why remove that comment?

The original should have said:

	/* Do the meta buffers and subbufs have correct data? */

>  	for (i = 0; i < meta->nr_subbufs; i++) {
>  		if (meta->buffers[i] < 0 ||
>  		    meta->buffers[i] >= meta->nr_subbufs) {
> @@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
>  			return false;
>  		}
>  
> -		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
> -			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
> -			return false;
> -		}

This should still be checked, although it doesn't need to fail the loop
but instead continue to the next buffer.

Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
the sub buffer was corrupted and should be skipped.

And honestly, the commit should never be greater than the subbuf_size,
even if corrupted. As we are only worried about corruption due to cache
not writing out. That should not corrupt the commit size (now we can
ignore the flags and use page size instead).

So, perhaps we should invalidate the entire buffer if the commit part
is corrupted, as that is a major corruption.

-- Steve

On Sat, 7 Mar 2026 10:27:11 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat,  7 Mar 2026 23:26:38 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> >  kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
> >  1 file changed, 33 insertions(+), 30 deletions(-)
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index b6f3ac99834f..8599de5cf59b 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -396,6 +396,12 @@ 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 int rb_page_size(struct buffer_page *bpage)
> > +{
> > +	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
> > +}
> > +
> >  static void free_buffer_page(struct buffer_page *bpage)
> >  {
> >  	/* Range pages are not to be freed */
> > @@ -1819,7 +1825,7 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> >  
> >  	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
> >  
> > -	/* Is the meta buffers and the subbufs themselves have correct data? */
> > +	/* Is the meta buffers themselves have correct data? */
> 
> I just realized that the origin didn't have correct grammar. But we
> still check the subbufs, why remove that comment?
> 
> The original should have said:
> 
> 	/* Do the meta buffers and subbufs have correct data? */

I just removed the data check from this loop, so I think this should
focus on checking metadata itself. The data is checked later.

> 
> >  	for (i = 0; i < meta->nr_subbufs; i++) {
> >  		if (meta->buffers[i] < 0 ||
> >  		    meta->buffers[i] >= meta->nr_subbufs) {
> > @@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> >  			return false;
> >  		}
> >  
> > -		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
> > -			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
> > -			return false;
> > -		}
> 
> This should still be checked, although it doesn't need to fail the loop
> but instead continue to the next buffer.

We already have another check of the data in the loop in
rb_meta_validate_events() so data corruption should be
handled there.

> 
> Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
> the sub buffer was corrupted and should be skipped.

Yes, if RB_MISSED_EVENTS bit is set, the commit field is out of range.
That is checked in rb_validate_buffer().

> 
> And honestly, the commit should never be greater than the subbuf_size,
> even if corrupted. As we are only worried about corruption due to cache
> not writing out. That should not corrupt the commit size (now we can
> ignore the flags and use page size instead).

Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
we will see the bit is set and commit size is different. 

Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
ring buffer on reboot") drops clearing commit field for unwinding the
buffer.

@@ -5342,7 +5440,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
         */
        local_set(&cpu_buffer->reader_page->write, 0);
        local_set(&cpu_buffer->reader_page->entries, 0);
-       local_set(&cpu_buffer->reader_page->page->commit, 0);
        cpu_buffer->reader_page->real_end = 0;
 
Should we clear the RB_MISSED_* bits here?

Thanks,

> 
> So, perhaps we should invalidate the entire buffer if the commit part
> is corrupted, as that is a major corruption.
> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Mon, 9 Mar 2026 08:53:17 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > > @@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > >  			return false;
> > >  		}
> > >  
> > > -		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
> > > -			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
> > > -			return false;
> > > -		}  
> > 
> > This should still be checked, although it doesn't need to fail the loop
> > but instead continue to the next buffer.  
> 
> We already have another check of the data in the loop in
> rb_meta_validate_events() so data corruption should be
> handled there.

Hmm, OK.

> 
> > 
> > Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
> > the sub buffer was corrupted and should be skipped.  
> 
> Yes, if RB_MISSED_EVENTS bit is set, the commit field is out of range.
> That is checked in rb_validate_buffer().
> 
> > 
> > And honestly, the commit should never be greater than the subbuf_size,
> > even if corrupted. As we are only worried about corruption due to cache
> > not writing out. That should not corrupt the commit size (now we can
> > ignore the flags and use page size instead).  
> 
> Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
> we will see the bit is set and commit size is different. 

The RB_MISSED_EVENTS is only set on the reader page.

If the kernel crashes no boot up while reading the validated buffer,
then that's a bit more than what this is supposed to handle.

> 
> Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
> read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
> ring buffer on reboot") drops clearing commit field for unwinding the
> buffer.

But that should be fine, as it's only read only. Once tracing is
started, it should be reset.

-- Steve

Hi Steve,

On Sun, 8 Mar 2026 22:19:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 9 Mar 2026 08:53:17 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
> > > the sub buffer was corrupted and should be skipped.  
> > 
> > Yes, if RB_MISSED_EVENTS bit is set, the commit field is out of range.
> > That is checked in rb_validate_buffer().
> > 
> > > 
> > > And honestly, the commit should never be greater than the subbuf_size,
> > > even if corrupted. As we are only worried about corruption due to cache
> > > not writing out. That should not corrupt the commit size (now we can
> > > ignore the flags and use page size instead).  
> > 
> > Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
> > we will see the bit is set and commit size is different. 
> 
> The RB_MISSED_EVENTS is only set on the reader page.

When is it reset after read? I think I removed that with commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent ring buffer on reboot"). So the
flags will be remains until the page is reused (as a writer page).

> 
> If the kernel crashes no boot up while reading the validated buffer,
> then that's a bit more than what this is supposed to handle.

But the above commit recovers the subbufs which has been read in the
previous boot. This means commit field of those recovered subbufs
can have those flags.

> 
> > 
> > Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
> > read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
> > ring buffer on reboot") drops clearing commit field for unwinding the
> > buffer.
> 
> But that should be fine, as it's only read only. Once tracing is
> started, it should be reset.

IIUC, RB_MISSED_* flags are using 30 and 31 th bits and committed
bytes counter is usually use 0-11 th bits. So other bits should be
cleared. 

So, if "commit & RB_MISSED_MASK" is under the subbuf_size, this
subbuf looks OK for checking its entries(timestamp data).
e.g.

unsigned long commit;

commit = local_read(subbuf->commit) & ~RB_MISSED_MASK;
if (commit > meta->subbuf_size)
  return -EINVAL;
return ;

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Mon, 9 Mar 2026 22:32:44 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > > Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
> > > we will see the bit is set and commit size is different.   
> > 
> > The RB_MISSED_EVENTS is only set on the reader page.  
> 
> When is it reset after read? I think I removed that with commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent ring buffer on reboot"). So the
> flags will be remains until the page is reused (as a writer page).

And that shouldn't be a problem.

> 
> > 
> > If the kernel crashes no boot up while reading the validated buffer,
> > then that's a bit more than what this is supposed to handle.  
> 
> But the above commit recovers the subbufs which has been read in the
> previous boot. This means commit field of those recovered subbufs
> can have those flags.

Yes, and since those flags are only set by the validator, if the validator
sees them set, it should skip processing the subbuffer. It knows it was
already flagged as corrupt.

> 
> >   
> > > 
> > > Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
> > > read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
> > > ring buffer on reboot") drops clearing commit field for unwinding the
> > > buffer.  
> > 
> > But that should be fine, as it's only read only. Once tracing is
> > started, it should be reset.  
> 
> IIUC, RB_MISSED_* flags are using 30 and 31 th bits and committed
> bytes counter is usually use 0-11 th bits. So other bits should be
> cleared. 
> 
> So, if "commit & RB_MISSED_MASK" is under the subbuf_size, this
> subbuf looks OK for checking its entries(timestamp data).
> e.g.

But it shouldn't be.  The only way the RB_MISSED flag gets set in the
writer portion is if on a previous boot the validator detected that the
subbuffer was corrupted. If they are set for any other reason, the
subbuffer should be considered corrupted anyway.

-- Steve

On Mon, 9 Mar 2026 08:53:17 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Sat, 7 Mar 2026 10:27:11 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Sat,  7 Mar 2026 23:26:38 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > 
> > >  kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
> > >  1 file changed, 33 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > index b6f3ac99834f..8599de5cf59b 100644
> > > --- a/kernel/trace/ring_buffer.c
> > > +++ b/kernel/trace/ring_buffer.c
> > > @@ -396,6 +396,12 @@ 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 int rb_page_size(struct buffer_page *bpage)
> > > +{
> > > +	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
> > > +}
> > > +
> > >  static void free_buffer_page(struct buffer_page *bpage)
> > >  {
> > >  	/* Range pages are not to be freed */
> > > @@ -1819,7 +1825,7 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > >  
> > >  	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
> > >  
> > > -	/* Is the meta buffers and the subbufs themselves have correct data? */
> > > +	/* Is the meta buffers themselves have correct data? */
> > 
> > I just realized that the origin didn't have correct grammar. But we
> > still check the subbufs, why remove that comment?
> > 
> > The original should have said:
> > 
> > 	/* Do the meta buffers and subbufs have correct data? */
> 
> I just removed the data check from this loop, so I think this should
> focus on checking metadata itself. The data is checked later.

Other checks in the loop are;

- the entries in meta::buffers[] are inside correct range.
- the duplicated entries in the meta::buffers[].

So this only checks the meta::buffers[] (index array) now.

/*
 * Ensure the meta::buffers have correct data. The data in each subbufs are
 * checked later in rb_meta_validate_events().
 */

This will be more clear.

> 
> > 
> > >  	for (i = 0; i < meta->nr_subbufs; i++) {
> > >  		if (meta->buffers[i] < 0 ||
> > >  		    meta->buffers[i] >= meta->nr_subbufs) {
> > > @@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > >  			return false;
> > >  		}
> > >  
> > > -		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
> > > -			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
> > > -			return false;
> > > -		}
> > 
> > This should still be checked, although it doesn't need to fail the loop
> > but instead continue to the next buffer.
> 
> We already have another check of the data in the loop in
> rb_meta_validate_events() so data corruption should be
> handled there.
> 
> > 
> > Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
> > the sub buffer was corrupted and should be skipped.
> 
> Yes, if RB_MISSED_EVENTS bit is set, the commit field is out of range.
> That is checked in rb_validate_buffer().
> 
> > 
> > And honestly, the commit should never be greater than the subbuf_size,
> > even if corrupted. As we are only worried about corruption due to cache
> > not writing out. That should not corrupt the commit size (now we can
> > ignore the flags and use page size instead).
> 
> Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
> we will see the bit is set and commit size is different. 
> 
> Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
> read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
> ring buffer on reboot") drops clearing commit field for unwinding the
> buffer.
> 
> @@ -5342,7 +5440,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>          */
>         local_set(&cpu_buffer->reader_page->write, 0);
>         local_set(&cpu_buffer->reader_page->entries, 0);
> -       local_set(&cpu_buffer->reader_page->page->commit, 0);
>         cpu_buffer->reader_page->real_end = 0;
>  
> Should we clear the RB_MISSED_* bits here?

Ah, no. ignore this. If there is a sudden reboot, the broken
commit will be there anyway. But we can recover it.

Thank you,

> 
> Thanks,
> 
> > 
> > So, perhaps we should invalidate the entire buffer if the commit part
> > is corrupted, as that is a major corruption.
> > 
> > -- Steve
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Mon, 9 Mar 2026 09:53:07 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > > 
> > > I just realized that the origin didn't have correct grammar. But we
> > > still check the subbufs, why remove that comment?
> > > 
> > > The original should have said:
> > > 
> > > 	/* Do the meta buffers and subbufs have correct data? */  
> > 
> > I just removed the data check from this loop, so I think this should
> > focus on checking metadata itself. The data is checked later.  
> 
> Other checks in the loop are;
> 
> - the entries in meta::buffers[] are inside correct range.
> - the duplicated entries in the meta::buffers[].
> 
> So this only checks the meta::buffers[] (index array) now.
> 
> /*
>  * Ensure the meta::buffers have correct data. The data in each subbufs are
>  * checked later in rb_meta_validate_events().
>  */
> 
> This will be more clear.

Sure.

-- Steve