MIME-Version: 1.0
References: <20230303151706.57851-1-ubizjak@gmail.com> <20230303151706.57851-3-ubizjak@gmail.com>
 <c63f96bb-2972-b96e-5105-d47c89f8571e@quicinc.com>
In-Reply-To: <c63f96bb-2972-b96e-5105-d47c89f8571e@quicinc.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Sat, 4 Mar 2023 09:56:36 +0100
Message-ID: <CAFULd4Z87YEkmtogBGmnCoAEq1HWeXb3JcJhr0rSnpjyiZ0nXA@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] ring_buffer: Change some static functions to bool
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 3, 2023 at 5:34=E2=80=AFPM 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 *buffe=
r)
> >   {
> >       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 buf=
fer */
> >       for_each_buffer_cpu(buffer, cpu) {
> > @@ -5438,8 +5436,7 @@ bool ring_buffer_empty_cpu(struct trace_buffer *b=
uffer, 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.