On Wed, Aug 10, 2022 at 1:02 AM Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
> > > 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@xxxxxxxxxxxxx/
-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.
