linux-kernel - Re: [PATCH v2 2/3] mutex: Queue mutex spinners with MCS lock to reduce cacheline contention

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]

Date:	Tue, 16 Apr 2013 08:05:21 -0400
From:	Waiman Long <Waiman.Long@...com>
To:	Davidlohr Bueso <davidlohr.bueso@...com>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	David Howells <dhowells@...hat.com>,
	Dave Jones <davej@...hat.com>,
	Clark Williams <williams@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org, x86@...nel.org,
	linux-arch@...r.kernel.org,
	"Chandramouleeswaran, Aswin" <aswin@...com>,
	"Norton, Scott J" <scott.norton@...com>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH v2 2/3] mutex: Queue mutex spinners with MCS lock to reduce
 cacheline contention

On 04/16/2013 12:24 AM, Davidlohr Bueso wrote:
> On Mon, 2013-04-15 at 10:37 -0400, Waiman Long wrote:
> [...]
>> +typedef struct mspin_node {
>> +	struct mspin_node *next;
>> +	int		   locked;	/* 1 if lock acquired */
>> +} mspin_node_t;
>> +
>> +typedef mspin_node_t	*mspin_lock_t;
> I think we could do without the typedefs, specially mspin_lock_t.
Yes, we can do without the typedefs.

>> +
>> +#define	MLOCK(mutex)	((mspin_lock_t *)&((mutex)->spin_mlock))
>> +
>> +static noinline void mspin_lock(mspin_lock_t *lock,  mspin_node_t *node)
>> +{
>> +	mspin_node_t *prev;
>> +
>> +	/* Init node */
>> +	node->locked = 0;
>> +	node->next   = NULL;
>> +
>> +	prev = xchg(lock, node);
>> +	if (likely(prev == NULL)) {
>> +		/* Lock acquired */
>> +		node->locked = 1;
>> +		return;
>> +	}
>> +	ACCESS_ONCE(prev->next) = node;
>> +	smp_wmb();
>> +	/* Wait until the lock holder passes the lock down */
>> +	while (!ACCESS_ONCE(node->locked))
>> +		arch_mutex_cpu_relax();
>> +}
>> +
>> +static void mspin_unlock(mspin_lock_t *lock,  mspin_node_t *node)
>> +{
>> +	mspin_node_t *next = ACCESS_ONCE(node->next);
>> +
>> +	if (likely(!next)) {
>> +		/*
>> +		 * Release the lock by setting it to NULL
>> +		 */
>> +		if (cmpxchg(lock, node, NULL) == node)
>> +			return;
>> +		/* Wait until the next pointer is set */
>> +		while (!(next = ACCESS_ONCE(node->next)))
>> +			arch_mutex_cpu_relax();
>> +	}
>> +	barrier();
>> +	ACCESS_ONCE(next->locked) = 1;
>> +	smp_wmb();
> Do we really need the compiler barrier call? The CPUs can reorder
> anyway. I assume the smp_wbm() call makes sure no there's no funny
> business before the next lock is acquired, might be worth commenting.

The smp_wmb() calls are to make sure that the writes are committed to 
memory rather than staying in the cache only. They are safety measures. 
The barrier() call probably is not needed because of the next pointer 
data dependency, but it doesn't have an actual cost either as it doesn't 
translate to any instruction.

Regards,
Longman
--
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/