Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220510154217.5216-1-ubizjak@gmail.com> <20220513091034.GH76023@worktop.programming.kicks-ass.net>
 <CAFULd4bAAKc=wo6vFkN6xQqBjaSJhF3L+WmuymSsC6-ec6TuvA@mail.gmail.com>
In-Reply-To: <CAFULd4bAAKc=wo6vFkN6xQqBjaSJhF3L+WmuymSsC6-ec6TuvA@mail.gmail.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Fri, 13 May 2022 17:36:19 +0200
Message-ID: <CAFULd4Zck5YRfhoXsg_aPKpFbnkkr_xZ6ejDpmp=ysZemwoQAg@mail.gmail.com>
Subject: Re: [PATCH] locking/atomic/x86: Introduce try_cmpxchg64
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        kvm@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Mark Rutland <mark.rutland@arm.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Marco Elver <elver@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, May 13, 2022 at 12:20 PM Uros Bizjak <ubizjak@gmail.com> wrote:

> > > +#define arch_try_cmpxchg64(ptr, po, n)                               \
> > > +({                                                           \
> > > +     bool success;                                           \
> > > +     __typeof__(*(ptr)) __prev;                              \
> > > +     __typeof__(ptr) _old = (__typeof__(ptr))(po);           \
> > > +     __typeof__(*(ptr)) __old = *_old;                       \
> > > +     __typeof__(*(ptr)) __new = (n);                         \
> > > +     alternative_io(LOCK_PREFIX_HERE                         \
> > > +                     "call cmpxchg8b_emu",                   \
> > > +                     "lock; cmpxchg8b (%%esi)" ,             \
> > > +                    X86_FEATURE_CX8,                         \
> > > +                    "=A" (__prev),                           \
> > > +                    "S" ((ptr)), "0" (__old),                \
> > > +                    "b" ((unsigned int)__new),               \
> > > +                    "c" ((unsigned int)(__new>>32))          \
> > > +                    : "memory");                             \
> > > +     success = (__prev == __old);                            \
> > > +     if (unlikely(!success))                                 \
> > > +             *_old = __prev;                                 \
> > > +     likely(success);                                        \
> > > +})
> >
> > Wouldn't this be better written like the normal fallback wrapper?
> >
> > static __always_inline bool
> > arch_try_cmpxchg64(u64 *v, u64 *old, u64 new)
> > {
> >         u64 r, o = *old;
> >         r = arch_cmpxchg64(v, o, new);
> >         if (unlikely(r != o))
> >                 *old = r;
> >         return likely(r == o);
> > }
> >
> > Less magical, same exact code.
>
> Also, I tried to follow up the existing #defines. Will improve the
> code according to your suggestion here.

In the v2 patch, generic fallbacks were introduced, so that
arch_try_cmpxchg64 can be used when only arch_cmpxchg64 is defined.

Uros.