Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Guenter Roeck]

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