Re: [PATCH v2] introduce atomic_pointer to fix a race condition in cancelable mcs spinlocks

Waiman Long
2014-06-02T17:30:19Z

On 06/02/2014 12:43 PM, Paul E. McKenney wrote:
> On Mon, Jun 02, 2014 at 06:25:25PM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 02, 2014 at 12:00:45PM -0400, Mikulas Patocka wrote:
>>> struct optimistic_spin_queue {
>>> - struct optimistic_spin_queue *next, *prev;
>>> + atomic_pointer(struct optimistic_spin_queue *) next;
>>> + struct optimistic_spin_queue *prev;
>>> int locked; /* 1 if lock acquired */
>>> };
>>>
>>> Index: linux-3.15-rc8/include/asm-generic/atomic-long.h
>>> ===================================================================
>>> --- linux-3.15-rc8.orig/include/asm-generic/atomic-long.h 2014-06-02 17:11:17.000000000 +0200
>>> +++ linux-3.15-rc8/include/asm-generic/atomic-long.h 2014-06-02 17:11:50.000000000 +0200
>>> @@ -255,4 +255,31 @@ static inline long atomic_long_add_unles
>>>
>>> #endif /* BITS_PER_LONG == 64 */
>>>
>>> +#define atomic_pointer(type) \
>>> +union { \
>>> + atomic_long_t __a; \
>>> + type __t; \
>>> + char __check_sizeof[sizeof(type) == sizeof(long) ? 1 : -1]; \
>>> +}
>> That's still entirely disgusting, and afaict entirely redundant. You can
>> do that test in the operators below just fine.
>>
>>> +#define ATOMIC_POINTER_INIT(i) { .__t = (i) }
>>> +
>>> +#define atomic_pointer_read(v) ((typeof((v)->__t))atomic_long_read(&(v)->__a))
>>> +
>>> +#define atomic_pointer_set(v, i) ({ \
>>> + typeof((v)->__t) __i = (i); \
>>> + atomic_long_set(&(v)->__a, (long)(__i)); \
>>> +})
>>> +
>>> +#define atomic_pointer_xchg(v, i) ({ \
>>> + typeof((v)->__t) __i = (i); \
>>> + (typeof((v)->__t))atomic_long_xchg(&(v)->__a, (long)(__i)); \
>>> +})
>>> +
>>> +#define atomic_pointer_cmpxchg(v, old, new) ({ \
>>> + typeof((v)->__t) __old = (old); \
>>> + typeof((v)->__t) __new = (new); \
>>> + (typeof((v)->__t))atomic_long_cmpxchg(&(v)->__a, (long)(__old), (long)(__new));\
>>> +})
>> And I can't say I'm a particular fan of these ops either, as alternative
>> I'm almost inclined to just exclude parisc from using opt spinning.
> That is an excellent point for this particular issue. Do parisc systems
> really support enough CPUs to make queued spinlocks worthwhile? If not,
> maybe we should just have parisc stick with traditional spinlocks.

The operation in question is the optimistic spinning code of mutex which
is currently active, I think, for all architectures. It is not related
to the queued spinlock, though it will have the same problem.

Yes, by disabling the MUTEX_SPIN_ON_OWNER config variable from PA-RISC,
we can disable optimistic spinning.

-Longman