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

[Waiman Long]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/