Re: [PATCH 2/4] MCS spinlocks: Convert osq lock to atomic_t to reduce overhead

[Jason Low]

On Tue, 2014-07-08 at 09:38 -0400, Steven Rostedt wrote:
> On Mon,  7 Jul 2014 11:50:17 -0700
> Jason Low <jason.low2@xxxxxx> wrote:

> >  #ifdef CONFIG_RWSEM_GENERIC_SPINLOCK
> > @@ -33,7 +32,7 @@ struct rw_semaphore {
> >  	 * if the owner is running on the cpu.
> >  	 */
> >  	struct task_struct *owner;
> > -	struct optimistic_spin_node *osq; /* spinner MCS lock */
> > +	struct optimistic_spin_queue osq; /* spinner MCS lock */
> >  #endif
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  	struct lockdep_map	dep_map;
> > @@ -70,7 +69,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
> >  	  __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),	\
> >  	  LIST_HEAD_INIT((name).wait_list),		\
> >  	  NULL, /* owner */				\
> > -	  NULL /* mcs lock */                           \
> > +	  { ATOMIC_INIT(OSQ_UNLOCKED_VAL) } /* osq */   \
> 
> This should probably be a macro, similar to the __RWSEM_DEP_MAP_INIT()
> below.  Open coded inits are evil.
> 
>   OSQ_LOCK_INIT() ?

I agree that we should use a macro here for the lock instead of directly
initializing it. Same with using a macro instead of directly calling the
atomic_sets in the later parts of this patch.

> 
> >  	  __RWSEM_DEP_MAP_INIT(name) }
> >  #else
> >  #define __RWSEM_INITIALIZER(name)			\
> > diff --git a/kernel/locking/mcs_spinlock.c b/kernel/locking/mcs_spinlock.c
> > index e9866f7..124a3bb 100644
> > --- a/kernel/locking/mcs_spinlock.c
> > +++ b/kernel/locking/mcs_spinlock.c
> > @@ -17,18 +17,43 @@
> >  static DEFINE_PER_CPU_SHARED_ALIGNED(struct optimistic_spin_node, osq_node);
> >  
> >  /*
> > + * We use the value 0 to represent "no CPU", thus the encoded value
> > + * will be the CPU number incremented by 1.
> > + */
> > +static inline int encode_cpu(int cpu_nr)
> > +{
> > +	return (cpu_nr + 1);
> 
> return is not a function, remove the parenthesis (checkpatch should
> point that out to you too).

I ran checkpatch and it didn't seem to be an issue. I was using the
parenthesis as "operator precedence" rather than a function call.
However, those parenthesis aren't necessary so we can delete them
anyway.

> > +}
> > +
> > +static inline struct optimistic_spin_node *decode_cpu(int encoded_cpu_val)
> > +{
> > +	int cpu_nr = encoded_cpu_val - 1;
> > +
> > +	return per_cpu_ptr(&osq_node, cpu_nr);
> > +}
> > +
> > +/*
> >   * Get a stable @node->next pointer, either for unlock() or unqueue() purposes.
> >   * Can return NULL in case we were the last queued and we updated @lock instead.
> >   */
> >  static inline struct optimistic_spin_node *
> > -osq_wait_next(struct optimistic_spin_node **lock,
> > +osq_wait_next(struct optimistic_spin_queue *lock,
> >  	      struct optimistic_spin_node *node,
> >  	      struct optimistic_spin_node *prev)
> >  {
> >  	struct optimistic_spin_node *next = NULL;
> > +	int curr = encode_cpu(smp_processor_id()), old;
> 
> Add a second line for "int old". Having it after an initialization is
> weird and confusing.

Sure. Thanks!

--
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/