[dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory

Diogo Behrens diogo.behrens at huawei.com
Wed Nov 25 09:41:34 CET 2020


Hi Stephen, 

the patch we submitted is safe, it has been verified that it indeed fixes the bug:
Besides rmem and genmc model checkers, we verified the bugfix with herd7 tool,
which is the official tool ARM provides to check such pieces of code against their
memory model.

Actually, I think there are other barriers in this MCS lock implementation that
could be weakened without causing problems, but that would be an optimization.
This patch is just to fix the bug.

Best regards,
-Diogo

-----Original Message-----
From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli at arm.com] 
Sent: Wednesday, November 25, 2020 5:51 AM
To: Stephen Hemminger <stephen at networkplumber.org>
Cc: Diogo Behrens <diogo.behrens at huawei.com>; thomas at monjalon.net; david.marchand at redhat.com; dev at dpdk.org; nd <nd at arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>; nd <nd at arm.com>
Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory

<snip>

> >
> > >
> > >     The initialization me->locked=1 in lock() must happen before
> > >     next->locked=0 in unlock(), otherwise a thread may hang forever,
> > >     waiting me->locked become 0. On weak memory systems (such as
> ARMv8),
> > >     the current implementation allows me->locked=1 to be reordered with
> > >     announcing the node (pred->next=me) and, consequently, to be
> > >     reordered with next->locked=0 in unlock().
> > >
> > >     This fix adds a release barrier to pred->next=me, forcing
> > >     me->locked=1 to happen before this operation.
> > >
> > > Signed-off-by: Diogo Behrens <diogo.behrens at huawei.com>
> > The change looks fine to me.  I have tested this on few x86 and Arm machines.
> > Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> 
> Maybe a simpler alternative would be as fast and safer.
Why is this safer?

> By using compare_exchange you can get same effect in one operation.
> Like the following UNTESTED.
> 
> diff --git a/lib/librte_eal/include/generic/rte_mcslock.h
> b/lib/librte_eal/include/generic/rte_mcslock.h
> index 78b0df295e2d..9c537ce577e6 100644
> --- a/lib/librte_eal/include/generic/rte_mcslock.h
> +++ b/lib/librte_eal/include/generic/rte_mcslock.h
> @@ -48,23 +48,23 @@ rte_mcslock_lock(rte_mcslock_t **msl, rte_mcslock_t
> *me)
>  	rte_mcslock_t *prev;
> 
>  	/* Init me node */
> -	__atomic_store_n(&me->locked, 1, __ATOMIC_RELAXED);
> -	__atomic_store_n(&me->next, NULL, __ATOMIC_RELAXED);
> +	me->locked = 1;
> 
> -	/* If the queue is empty, the exchange operation is enough to acquire
> -	 * the lock. Hence, the exchange operation requires acquire semantics.
> -	 * The store to me->next above should complete before the node is
> -	 * visible to other CPUs/threads. Hence, the exchange operation
> requires
> -	 * release semantics as well.
> +	/*
> +	 * Atomic insert into single linked list
>  	 */
> -	prev = __atomic_exchange_n(msl, me, __ATOMIC_ACQ_REL);
> +	do {
> +		prev = __atomic_load_n(msl, __ATOMIC_RELAXED);
> +		me->next = prev;
This needs to be __atomic_store_n(__ATOMIC_RELEASE) as it can sink below the following line.

> +	} while (!__atomic_compare_exchange_n(&msl, me, prev,
> +					    __ATOMIC_ACQUIRE,
> __ATOMIC_RELAXED));
> +
>  	if (likely(prev == NULL)) {
>  		/* Queue was empty, no further action required,
>  		 * proceed with lock taken.
>  		 */
>  		return;
>  	}
> -	__atomic_store_n(&prev->next, me, __ATOMIC_RELAXED);
> 
>  	/* The while-load of me->locked should not move above the previous
>  	 * store to prev->next. Otherwise it will cause a deadlock. Need a


More information about the dev mailing list