MIME-Version: 1.0
References: <20230318080059.1109286-1-leobras@redhat.com> <8f698f49-135a-4263-8471-96f406919cb1@spud>
 <4f35f41c143b02b6b815e7eb527ca3acd8b4aabe.camel@redhat.com> <18e07d0a-7e21-46d0-9166-6ee07817352a@spud>
In-Reply-To: <18e07d0a-7e21-46d0-9166-6ee07817352a@spud>
From:   Leonardo Bras Soares Passos <leobras@redhat.com>
Date:   Tue, 21 Mar 2023 14:01:59 -0300
Message-ID: <CAJ6HWG5zHMB1-vVsuQ+nG3EC12JAi2MP5o8GbSQ9QJfgLEQNnw@mail.gmail.com>
Subject: Re: [RFC PATCH 0/6] Deduplicating RISCV cmpxchg.h macros
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     Conor Dooley <conor@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 21, 2023 at 4:49=E2=80=AFAM Conor Dooley <conor.dooley@microchi=
p.com> wrote:
>
> On Tue, Mar 21, 2023 at 03:30:35AM -0300, Leonardo Br=C3=A1s 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, result=
ing a
> > > > total of 6 patches. I did this so it becomes easier to review and r=
emove
> > > > the last levels if desired, but I have no issue squashing them if i=
t'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 oppos=
ed
> > > 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=3Driscv64-linux-gnu- ] but I could not get any error in a=
ny 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 th=
e 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 happen=
s.
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