linux-kernel - Re: [PATCH 2/4] MCS spinlocks: Convert osq lock to atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1404837870.2448.9.camel@j-VirtualBox>
Date:	Tue, 08 Jul 2014 09:44:30 -0700
From:	Jason Low <jason.low2@...com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	peterz@...radead.org, torvalds@...ux-foundation.org,
	paulmck@...ux.vnet.ibm.com, mingo@...nel.org, Waiman.Long@...com,
	davidlohr@...com, linux-kernel@...r.kernel.org, tglx@...utronix.de,
	riel@...hat.com, akpm@...ux-foundation.org, hpa@...or.com,
	tim.c.chen@...ux.intel.com, konrad.wilk@...cle.com, aswin@...com,
	scott.norton@...com, chegu_vinod@...com
Subject: Re: [PATCH 2/4] MCS spinlocks: Convert osq lock to atomic_t to
 reduce overhead

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@...com> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/