| Message ID | 20230228175929.7534-2-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve trace/ring_buffer.c | expand |
On Tue, 28 Feb 2023 18:59:27 +0100 Uros Bizjak <ubizjak@gmail.com> wrote: > The results of some static functions are not used. Change the > type of these function to void and remove unnecessary returns. > > No functional change intended. NAK, instead of dropping the errors, please handle it on the caller side. Thank you, > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > --- > kernel/trace/ring_buffer.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index af50d931b020..05fdc92554df 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -1569,15 +1569,12 @@ static void rb_tail_page_update(struct ring_buffer_per_cpu *cpu_buffer, > } > } > > -static int rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, > +static void rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, > struct buffer_page *bpage) > { > unsigned long val = (unsigned long)bpage; > > - if (RB_WARN_ON(cpu_buffer, val & RB_FLAG_MASK)) > - return 1; > - > - return 0; > + RB_WARN_ON(cpu_buffer, val & RB_FLAG_MASK); > } > > /** > @@ -1587,30 +1584,28 @@ static int rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, > * As a safety measure we check to make sure the data pages have not > * been corrupted. > */ > -static int rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer) > +static void rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer) > { > struct list_head *head = rb_list_head(cpu_buffer->pages); > struct list_head *tmp; > > if (RB_WARN_ON(cpu_buffer, > rb_list_head(rb_list_head(head->next)->prev) != head)) > - return -1; > + return; > > if (RB_WARN_ON(cpu_buffer, > rb_list_head(rb_list_head(head->prev)->next) != head)) > - return -1; > + return; > > for (tmp = rb_list_head(head->next); tmp != head; tmp = rb_list_head(tmp->next)) { > if (RB_WARN_ON(cpu_buffer, > rb_list_head(rb_list_head(tmp->next)->prev) != tmp)) > - return -1; > + return; > > if (RB_WARN_ON(cpu_buffer, > rb_list_head(rb_list_head(tmp->prev)->next) != tmp)) > - return -1; > + return; > } > - > - return 0; > } > > static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer, > @@ -4500,7 +4495,6 @@ rb_update_read_stamp(struct ring_buffer_per_cpu *cpu_buffer, > default: > RB_WARN_ON(cpu_buffer, 1); > } > - return; > } > > static void > @@ -4531,7 +4525,6 @@ rb_update_iter_read_stamp(struct ring_buffer_iter *iter, > default: > RB_WARN_ON(iter->cpu_buffer, 1); > } > - return; > } > > static struct buffer_page * > @@ -4946,7 +4939,6 @@ rb_reader_unlock(struct ring_buffer_per_cpu *cpu_buffer, bool locked) > { > if (likely(locked)) > raw_spin_unlock(&cpu_buffer->reader_lock); > - return; > } > > /** > -- > 2.39.2 >
On Tue, Feb 28, 2023 at 11:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Tue, 28 Feb 2023 18:59:27 +0100 > Uros Bizjak <ubizjak@gmail.com> wrote: > > > The results of some static functions are not used. Change the > > type of these function to void and remove unnecessary returns. > > > > No functional change intended. > > NAK, instead of dropping the errors, please handle it on the caller side. I was under the impression that the intention of these two functions is to warn if there is any corruption in data pages detected. Please note that the patch has no effect on code size, as the compiler is smart enough to drop unused return values by itself. So, the change is mostly cosmetic as I was just bothered by unused returns. I'm not versed enough in the code to introduce additional error handling, so considering its minimal impact, the patch can just be dropped. Uros.
On Wed, 1 Mar 2023 09:46:50 +0100 Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Feb 28, 2023 at 11:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Tue, 28 Feb 2023 18:59:27 +0100 > > Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > The results of some static functions are not used. Change the > > > type of these function to void and remove unnecessary returns. > > > > > > No functional change intended. > > > > NAK, instead of dropping the errors, please handle it on the caller side. > > I was under the impression that the intention of these two functions > is to warn if there is any corruption in data pages detected. Please > note that the patch has no effect on code size, as the compiler is > smart enough to drop unused return values by itself. So, the change is > mostly cosmetic as I was just bothered by unused returns. I'm not > versed enough in the code to introduce additional error handling, so > considering its minimal impact, the patch can just be dropped. > I'm not against the change. Masami, I don't think we need to check the return values, as when these checks fail, it triggers RB_WARN_ON() which disables the ring buffer involved, and that should stop further progress of other calls to the affected ring buffer. -- Steve
On Wed, 1 Mar 2023 11:34:16 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 1 Mar 2023 09:46:50 +0100 > Uros Bizjak <ubizjak@gmail.com> wrote: > > > On Tue, Feb 28, 2023 at 11:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > On Tue, 28 Feb 2023 18:59:27 +0100 > > > Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > > The results of some static functions are not used. Change the > > > > type of these function to void and remove unnecessary returns. > > > > > > > > No functional change intended. > > > > > > NAK, instead of dropping the errors, please handle it on the caller side. > > > > I was under the impression that the intention of these two functions > > is to warn if there is any corruption in data pages detected. Please > > note that the patch has no effect on code size, as the compiler is > > smart enough to drop unused return values by itself. So, the change is > > mostly cosmetic as I was just bothered by unused returns. I'm not > > versed enough in the code to introduce additional error handling, so > > considering its minimal impact, the patch can just be dropped. > > > > I'm not against the change. > > Masami, > > I don't think we need to check the return values, as when these checks > fail, it triggers RB_WARN_ON() which disables the ring buffer involved, and > that should stop further progress of other calls to the affected ring > buffer. Ah, so the RB_WARN_ON() has a side effect which stops all further operations. OK, I got it. Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > -- Steve >
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index af50d931b020..05fdc92554df 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c @@ -1569,15 +1569,12 @@ static void rb_tail_page_update(struct ring_buffer_per_cpu *cpu_buffer, } } -static int rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, +static void rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, struct buffer_page *bpage) { unsigned long val = (unsigned long)bpage; - if (RB_WARN_ON(cpu_buffer, val & RB_FLAG_MASK)) - return 1; - - return 0; + RB_WARN_ON(cpu_buffer, val & RB_FLAG_MASK); } /** @@ -1587,30 +1584,28 @@ static int rb_check_bpage(struct ring_buffer_per_cpu *cpu_buffer, * As a safety measure we check to make sure the data pages have not * been corrupted. */ -static int rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer) +static void rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer) { struct list_head *head = rb_list_head(cpu_buffer->pages); struct list_head *tmp; if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(head->next)->prev) != head)) - return -1; + return; if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(head->prev)->next) != head)) - return -1; + return; for (tmp = rb_list_head(head->next); tmp != head; tmp = rb_list_head(tmp->next)) { if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(tmp->next)->prev) != tmp)) - return -1; + return; if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(tmp->prev)->next) != tmp)) - return -1; + return; } - - return 0; } static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer, @@ -4500,7 +4495,6 @@ rb_update_read_stamp(struct ring_buffer_per_cpu *cpu_buffer, default: RB_WARN_ON(cpu_buffer, 1); } - return; } static void @@ -4531,7 +4525,6 @@ rb_update_iter_read_stamp(struct ring_buffer_iter *iter, default: RB_WARN_ON(iter->cpu_buffer, 1); } - return; } static struct buffer_page * @@ -4946,7 +4939,6 @@ rb_reader_unlock(struct ring_buffer_per_cpu *cpu_buffer, bool locked) { if (likely(locked)) raw_spin_unlock(&cpu_buffer->reader_lock); - return; } /**
The results of some static functions are not used. Change the type of these function to void and remove unnecessary returns. No functional change intended. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Uros Bizjak <ubizjak@gmail.com> --- kernel/trace/ring_buffer.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)