linux-kernel - Re: [RFC PATCH 0/6] Deduplicating RISCV cmpxchg.h macros

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ6HWG5zHMB1-vVsuQ+nG3EC12JAi2MP5o8GbSQ9QJfgLEQNnw@mail.gmail.com>
Date:   Tue, 21 Mar 2023 14:01:59 -0300
From:   Leonardo Bras Soares Passos <leobras@...hat.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Conor Dooley <conor@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/6] Deduplicating RISCV cmpxchg.h macros

On Tue, Mar 21, 2023 at 4:49 AM Conor Dooley <conor.dooley@...rochip.com> wrote:
>
> On Tue, Mar 21, 2023 at 03:30:35AM -0300, Leonardo Brás wrote:
> > Hello Conor, thanks for the feedback!
> >
> >
> > On Sun, 2023-03-19 at 20:35 +0000, Conor Dooley wrote:
> > > On Sat, Mar 18, 2023 at 05:00:54AM -0300, Leonardo Bras wrote:
> > > > While studying riscv's cmpxchg.h file, I got really interested in
> > > > understanding how RISCV asm implemented the different versions of
> > > > {cmp,}xchg.
> > > >
> > > > When I understood the pattern, it made sense for me to remove the
> > > > duplications and create macros to make it easier to understand what exactly
> > > > changes between the versions: Instruction sufixes & barriers.
> > > >
> > > > I split those changes in 3 levels for each cmpxchg and xchg, resulting a
> > > > total of 6 patches. I did this so it becomes easier to review and remove
> > > > the last levels if desired, but I have no issue squashing them if it's
> > > > better.
> > > >
> > > > Please provide comments.
> > > >
> > > > Thanks!
> > > > Leo
> > > >
> > > > Leonardo Bras (6):
> > > >   riscv/cmpxchg: Deduplicate cmpxchg() asm functions
> > > >   riscv/cmpxchg: Deduplicate cmpxchg() macros
> > > >   riscv/cmpxchg: Deduplicate arch_cmpxchg() macros
> > >
> > > >   riscv/cmpxchg: Deduplicate xchg() asm functions
> > >
> > > FWIW, this patch seems to break the build pretty badly:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20230318080059.1109286-5-leobras@redhat.com/
> >
> > Thanks for pointing out!
> >
> > It was an intermediary error:
> > Sufix for amoswap on acquire version was "d.aqrl" instead of the
> > correct".d.aqrl", and that caused the fail.
> >
> > I did not notice anything because the next commit made it more general, and thus
> > removed this line of code. I will send a v2-RFC shortly.
> >
> > I see that patch 4/6 has 5 fails, but on each one of them I can see:
> > "I: build output in /ci/workspace/[...]", or
> > ""I: build output in /tmp/[...]".
>
> I don't push the built artifacts anywhere, just the brief logs -
> although the "failed to build" log isn't very helpful other than telling
> you the build broke.
> That seems like a bug w.r.t. where tuxmake prints its output. I'll try
> to fix that.

Thanks for that :)

>
> > I could not find any reference to where this is saved, though.
> > Could you point where can I find the error output, just for the sake of further
> > debugging?
> >
> > >
> > > Patches 1 & 5 also add quite a lot of sparse issues (like 1000), but I
> > > think that may be more of an artifact of the testing process as opposed
> > > to something caused by this patchset.
> >
> > For those I can see the build output diffs. Both added error lines to
> > conchuod/build_rv64_gcc_allmodconfig.
> > I tried to mimic this with [make allmodconfig + gcc build with
> > CROSS_COMPILE=riscv64-linux-gnu- ] but I could not get any error in any patch.
>
> If you can't replicate them, it's probably because they come from
> sparse. I only really mentioned it here in case you went looking at the
> other patches in the series and were wondering why things were so amiss.

Oh, it makes sense.

>
> > For patch 1/6 it removes as much error lines (-) as it adds (+), and the error
> > messages are mostly the same, apart for an line number.
>
> I don't see a line number difference, but rather an increase in the
> number of times the same error lines have appeared in the output.
> I don't find the single-line output from sparse to really be very
> helpful, I should probably have a bit of a think how to present that
> information better.

Oh, I see.
The number at the beginning relates to the number of times the error happens.
Ok it makes sense to me now :)

>
> > For patch 5/6 it actually adds many more lines, but tracking (some of) the
> > errors gave me no idea why.
>
> Probably just sparse being unhappy with some way the macros were
> changed - but some of it ("Should it be static?" bits) look very much
> like the patch causing things to be rebuilt only for the "after the
> patch" build, but somehow not for the "before" build.

Humm, not sure I could understand that last part:
What I get is that the patch 5/6 is causing things to be rebuilt, and
it was not like that on 1-4/6.
Is that what you said?
If so, and you are doing it as an incremental build, changing the
macros in 5/6 should be triggering rebuilds, but it does not make
sense to not be rebuilt in 1-4/6 as they change the same macros.

Thanks for the feedback!

Best regards,
Leo