On Tue, Mar 18, 2025 at 4:25 PM Mateusz Guzik <mjgu...@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 10:46:39AM +0100, Christoph Hellwig wrote:
> > Replace int used as bool with the actual bool type for return values that
> > can only be true or false.
> >
> [snip]
>
> > -int lockref_get_not_zero(struct lockref *lockref)
> > +bool lockref_get_not_zero(struct lockref *lockref)
> > {
> > - int retval;
> > + bool retval = false;
> >
> > CMPXCHG_LOOP(
> > new.count++;
> > if (old.count <= 0)
> > - return 0;
> > + return false;
> > ,
> > - return 1;
> > + return true;
> > );
> >
> > spin_lock(&lockref->lock);
> > - retval = 0;
> > if (lockref->count > 0) {
> > lockref->count++;
> > - retval = 1;
> > + retval = true;
> > }
> > spin_unlock(&lockref->lock);
> > return retval;
>
> While this looks perfectly sane, it worsens codegen around the atomic
> on x86-64 at least with gcc 13.3.0. It bisected to this commit and
> confirmed top of next-20250318 with this reverted undoes it.
>
> The expected state looks like this:
> f0 48 0f b1 13 lock cmpxchg %rdx,(%rbx)
> 75 0e jne ffffffff81b33626
> <lockref_get_not_dead+0x46>
>
> However, with the above patch I see:
> f0 48 0f b1 13 lock cmpxchg %rdx,(%rbx)
> 40 0f 94 c5 sete %bpl
> 40 84 ed test %bpl,%bpl
> 74 09 je ffffffff81b33636
> <lockref_get_not_dead+0x46>
>
> This is not the end of the world, but also really does not need to be
> there.
>
> Given that the patch is merely a cosmetic change, I would suggest I gets
> dropped.
fwiw I confirmed clang does *not* have the problem, I don't know about gcc 14.
Maybe I'll get around to testing it, but first I'm gonna need to carve
out the custom asm into a standalone testcase.
Regardless, 13 suffering the problem is imo a good enough reason to
whack the change.
--
Mateusz Guzik <mjguzik gmail.com>
