Re: [PATCH v3 2/3] ring_buffer: Change some static functions to bool

[Uros Bizjak <ubizjak@gmail.com>]

On Fri, Mar 3, 2023 at 5:34 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> > -static int rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set)
> > +static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set)
> >   {
> >       unsigned long cnt, top, bottom, msb;
> >       unsigned long cnt2, top2, bottom2, msb2;
> > @@ -1490,7 +1488,7 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
> >       return NULL;
> >   }
> >
> > -static int rb_head_page_replace(struct buffer_page *old,
> > +static bool rb_head_page_replace(struct buffer_page *old,
> >                               struct buffer_page *new)
>
> nit: did you miss ret in rb_get_reader_page()?

Indeed, thanks for noticing!

> >   static __always_inline void
> > @@ -5408,9 +5407,8 @@ bool ring_buffer_empty(struct trace_buffer *buffer)
> >   {
> >       struct ring_buffer_per_cpu *cpu_buffer;
> >       unsigned long flags;
> > -     bool dolock;
> > +     bool dolock, ret;
> >       int cpu;
> > -     int ret;
>
> nit: would have been nice if you would not have changed existing stuff
> i.e each variable on a separate line.
> >
> >       /* yes this is racy, but if you don't like the race, lock the buffer */
> >       for_each_buffer_cpu(buffer, cpu) {
> > @@ -5438,8 +5436,7 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu)
> >   {
> >       struct ring_buffer_per_cpu *cpu_buffer;
> >       unsigned long flags;
> > -     bool dolock;
> > -     int ret;
> > +     bool dolock, ret;
>
> same here

I'll do it in the way you propose in the follow-up revisiton.

> >
> >       if (!cpumask_test_cpu(cpu, buffer->cpumask))
> >               return true;
>
>
> Feel free to add
>
> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>

Thanks.

Uros.