2024-03-12 15:54:45

by Steven Rostedt

Subject: [PATCH] ring-buffer: Do not set shortest_full when full target is hit

From: "Steven Rostedt (Google)" <[email protected]>

The rb_watermark_hit() checks if the amount of data in the ring buffer is
above the percentage level passed in by the "full" variable. If it is, it
returns true.

But it also sets the "shortest_full" field of the cpu_buffer that informs
writers that it needs to call the irq_work if the amount of data on the
ring buffer is above the requested amount.

The rb_watermark_hit() always sets the shortest_full even if the amount in
the ring buffer is what it wants. As it is not going to wait, because it
has what it wants, there's no reason to set shortest_full.

Cc: [email protected]
Fixes: 42fb0a1e84ff5 ("tracing/ring-buffer: Have polling block on watermark")
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
kernel/trace/ring_buffer.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 9b887d44b8d9..350607cce869 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -834,9 +834,10 @@ static bool rb_watermark_hit(struct trace_buffer *buffer, int cpu, int full)
pagebusy = cpu_buffer->reader_page == cpu_buffer->commit_page;
ret = !pagebusy && full_hit(buffer, cpu, full);

- if (!cpu_buffer->shortest_full ||
- cpu_buffer->shortest_full > full)
- cpu_buffer->shortest_full = full;
+ if (!ret && (!cpu_buffer->shortest_full ||
+ cpu_buffer->shortest_full > full)) {
+ cpu_buffer->shortest_full = full;
+ }
raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
}
return ret;
--
2.43.0

2024-03-12 16:14:06

by Masami Hiramatsu

Subject: Re: [PATCH] ring-buffer: Do not set shortest_full when full target is hit

On Tue, 12 Mar 2024 11:56:41 -0400
Steven Rostedt <[email protected]> wrote:

> From: "Steven Rostedt (Google)" <[email protected]>
>
> The rb_watermark_hit() checks if the amount of data in the ring buffer is
> above the percentage level passed in by the "full" variable. If it is, it
> returns true.
>
> But it also sets the "shortest_full" field of the cpu_buffer that informs
> writers that it needs to call the irq_work if the amount of data on the
> ring buffer is above the requested amount.
>
> The rb_watermark_hit() always sets the shortest_full even if the amount in
> the ring buffer is what it wants. As it is not going to wait, because it
> has what it wants, there's no reason to set shortest_full.
>

Yeah, it should avoid setting if !ret. Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <[email protected]>

Thank you,

> Cc: [email protected]
> Fixes: 42fb0a1e84ff5 ("tracing/ring-buffer: Have polling block on watermark")
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
> kernel/trace/ring_buffer.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 9b887d44b8d9..350607cce869 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -834,9 +834,10 @@ static bool rb_watermark_hit(struct trace_buffer *buffer, int cpu, int full)
> pagebusy = cpu_buffer->reader_page == cpu_buffer->commit_page;
> ret = !pagebusy && full_hit(buffer, cpu, full);
>
> - if (!cpu_buffer->shortest_full ||
> - cpu_buffer->shortest_full > full)
> - cpu_buffer->shortest_full = full;
> + if (!ret && (!cpu_buffer->shortest_full ||
> + cpu_buffer->shortest_full > full)) {
> + cpu_buffer->shortest_full = full;
> + }
> raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> }
> return ret;
> --
> 2.43.0
>


--
Masami Hiramatsu (Google) <[email protected]>