Re: [PATCH] ring-buffer: Include dropped pages in counting dirty patches

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

On Fri, 21 Oct 2022 12:30:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The function ring_buffer_nr_dirty_pages() was created to find out how many
> pages are filled in the ring buffer. There's two running counters. One is
> incremented whenever a new page is touched (pages_touched) and the other
> is whenever a page is read (pages_read). The dirty count is the number
> touched minus the number read. This is used to determine if a blocked task
> should be woken up if the percentage of the ring buffer it is waiting for
> is hit.
> 
> The problem is that it does not take into account dropped pages (when the
> new writes overwrite pages that were not read). And then the dirty pages
> will always be greater than the percentage.
> 
> Add a new counter to keep track of lost pages, and include that in the
> accounting of dirty pages so that it is actually accurate.
> 
> Fixes: 2c2b0a78b3739 ("ring-buffer: Add percentage of ring buffer full to wake up reader")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

This looks good to me. But I have just a nitpick below.

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


> ---
>  kernel/trace/ring_buffer.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index b60047de897e..f712006f6dd3 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -519,6 +519,7 @@ struct ring_buffer_per_cpu {
>  	local_t				committing;
>  	local_t				commits;
>  	local_t				pages_touched;
> +	local_t				pages_lost;
>  	local_t				pages_read;
>  	long				last_pages_touch;
>  	size_t				shortest_full;
> @@ -894,10 +895,18 @@ size_t ring_buffer_nr_pages(struct trace_buffer *buffer, int cpu)
>  size_t ring_buffer_nr_dirty_pages(struct trace_buffer *buffer, int cpu)
>  {
>  	size_t read;
> +	size_t lost;
>  	size_t cnt;
>  
>  	read = local_read(&buffer->buffers[cpu]->pages_read);
> +	lost = local_read(&buffer->buffers[cpu]->pages_lost);
>  	cnt = local_read(&buffer->buffers[cpu]->pages_touched);
> +
> +	if (WARN_ON_ONCE(cnt < lost))
> +		return 0;
> +
> +	cnt -= lost;
> +
>  	/* The reader can read an empty page, but not more than that */
>  	if (cnt < read) {
>  		WARN_ON_ONCE(read > cnt + 1);
> @@ -2020,6 +2029,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
>  			 */
>  			local_add(page_entries, &cpu_buffer->overrun);
>  			local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes);
> +			local_inc(&cpu_buffer->pages_lost);

Maybe we can make this part a static helper function so that we don't
repeat it below?

>  		}
>  
>  		/*
> @@ -2504,6 +2514,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
>  		 */
>  		local_add(entries, &cpu_buffer->overrun);
>  		local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes);
> +		local_inc(&cpu_buffer->pages_lost);

Thanks,

>  
>  		/*
>  		 * The entries will be zeroed out when we move the
> @@ -5254,6 +5265,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
>  	local_set(&cpu_buffer->committing, 0);
>  	local_set(&cpu_buffer->commits, 0);
>  	local_set(&cpu_buffer->pages_touched, 0);
> +	local_set(&cpu_buffer->pages_lost, 0);
>  	local_set(&cpu_buffer->pages_read, 0);
>  	cpu_buffer->last_pages_touch = 0;
>  	cpu_buffer->shortest_full = 0;
> -- 
> 2.35.1
> 


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