Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220809165615.9694-1-ubizjak@gmail.com> <20220809220511.GI3600936@dread.disaster.area>
 <20220809230244.GJ3600936@dread.disaster.area>
In-Reply-To: <20220809230244.GJ3600936@dread.disaster.area>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Wed, 10 Aug 2022 09:02:16 +0200
Message-ID: <CAFULd4YkSG4RES8=P5BjF8JH5En++XN6LpVNd391eYcy_baeyw@mail.gmail.com>
Subject: Re: [PATCH] fs/xfs: Use atomic64_try_cmpxchg in xlog_grant_{add,sub}_space
To:     Dave Chinner <david@fromorbit.com>
Cc:     linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Darrick J. Wong" <djwong@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 10, 2022 at 1:02 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Aug 10, 2022 at 08:05:11AM +1000, Dave Chinner wrote:
> > On Tue, Aug 09, 2022 at 06:56:15PM +0200, Uros Bizjak wrote:
> > > Use `!atomic64_try_cmpxchg(ptr, &old, new)` instead of
> > > `atomic64_cmpxchg(ptr, old, new) != old` in xlog_grant_{add,sub}_space.
> > > This has two benefits:
> > >
> > > - The x86 cmpxchg instruction returns success in the ZF flag, so this
> > >   change saves a compare after cmpxchg, as well as a related move
> > >   instruction in the front of cmpxchg.
> > >
> > > - atomic64_try_cmpxchg implicitly assigns the *ptr value to &old when
> > >   cmpxchg fails, enabling further code simplifications.
> >
> > Do the two cmpxchg operations have the same memory ordering
> > semantics on failure?

Yes. The API also provides _acquire, _release and _relaxed variants of
both, atomic64_cmpxchg and atomic64_try_cmpxchg. On x86, these two
functions actually compile to the same CMPXCHG instruction, the
difference is only in how the comparison is handled:

      15:    48 09 c2                 or     %rax,%rdx
      18:    48 89 c8                 mov    %rcx,%rax
      1b:    f0 48 0f b1 16           lock cmpxchg %rdx,(%rsi)
      20:    48 39 c1                 cmp    %rax,%rcx
      23:    74 2a                    je     4f <xlog_grant_add_space+0x4f>

becomes:

     29c:    48 09 ca                 or     %rcx,%rdx
     29f:    f0 48 0f b1 16           lock cmpxchg %rdx,(%rsi)
     2a4:    75 d2                    jne    278 <xlog_grant_add_space+0x8>

And as demonstrated in [1], even the fallback code compiles to a
better assembly.

[1] https://lore.kernel.org/lkml/CAFULd4bc54+_FmJ=f++zzz99mR8r5c11-Y49pz86Yb8G3dyJpA@mail.gmail.com/

> > > This patch has no functional change.
> >
> > The patch looks ok, but ....
> >
> > ... I'm about 2 hours away from posting a patchset that completely
> > removes the cmpxchg and the new grant head accounting has
> > significantly lower fast path overhead. It also opens the door for
> > tracking more than 2GB of log space in the grant heads.
>
> FYI, the original RFC for this was posted a bit over a month ago:
>
> https://lore.kernel.org/linux-xfs/20220708015558.1134330-1-david@fromorbit.com/

-static void
+void
 xlog_grant_sub_space(

[...]

- old = head_val;
- new = xlog_assign_grant_head_val(cycle, space);
- head_val = atomic64_cmpxchg(&head->grant, old, new);
- } while (head_val != old);
+ atomic64_sub(bytes, &head->grant);
 }

I actually wondered why these two functions were not implemented as
atomic64_{add,sub}. Of course, this is a much better solution that
renders the proposed patch obsolete.

BR,
Uros.