On Wed, Oct 31, 2018 at 09:32:43PM +0000, Paul Burton wrote:
> Hi Guenter,
>
> On Wed, Oct 31, 2018 at 12:52:18PM -0700, Guenter Roeck wrote:
> > +/*
> > + * Generic version of __cmpxchg_u64, to be used for cmpxchg64().
> > + * Takes u64 parameters.
> > + */
> > +u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
> > +{
> > + raw_spinlock_t *lock = lock_addr(ptr);
> > + unsigned long flags;
> > + u64 prev;
> > +
> > + raw_spin_lock_irqsave(lock, flags);
> > + prev = READ_ONCE(*ptr);
> > + if (prev == old)
> > + *ptr = new;
> > + raw_spin_unlock_irqrestore(lock, flags);
> > +
> > + return prev;
> > +}
> > +EXPORT_SYMBOL(__cmpxchg_u64);
>
> This is only going to work if we know that memory modified using
> __cmpxchg_u64() is *always* modified using __cmpxchg_u64(). Without that
> guarantee there's nothing to stop some other CPU writing to *ptr after
> the READ_ONCE() above but before we write new to it.
>
> As far as I'm aware this is not a guarantee we currently provide, so it
> would mean making that a requirement for cmpxchg64() users & auditing
> them all. That would also leave cmpxchg64() with semantics that differ
> from plain cmpxchg(), and semantics that may surprise people. In my view
> that's probably not worth it, and it would be better to avoid using
> cmpxchg64() on systems that can't properly support it.
>
Good point. Unfortunately this is also true for the architectures with
similar implementations, ie at least sparc32 (and possibly parisc).
The alternatives I can see are
- Do not use cmpxchg64() outside architecture code (ie drop its use from
the offending driver, and keep doing the same whenever the problem comes
up again).
or
- Introduce something like ARCH_HAS_CMPXCHG64 and use it to determine
if cmpxchg64 is supported or not.
Any preference ?
Thanks,
Guenter
