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

[Waiman Long <waiman.long@hp.com>]

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