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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFULd4Z87YEkmtogBGmnCoAEq1HWeXb3JcJhr0rSnpjyiZ0nXA@mail.gmail.com>
Date:   Sat, 4 Mar 2023 09:56:36 +0100
From:   Uros Bizjak <ubizjak@...il.com>
To:     Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH v3 2/3] ring_buffer: Change some static functions to bool

On Fri, Mar 3, 2023 at 5:34 PM Mukesh Ojha <quic_mojha@...cinc.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@...cinc.com>

Thanks.

Uros.