DPDK patches and discussions
 help / color / mirror / Atom feed
From: Diogo Behrens <diogo.behrens@huawei.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	Stephen Hemminger <stephen@networkplumber.org>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	"dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory
Date: Wed, 25 Nov 2020 08:41:34 +0000	[thread overview]
Message-ID: <a7d91daf10354b64916726950098e858@huawei.com> (raw)
In-Reply-To: <DBAPR08MB581461EC03BD251C6E403EB898FA0@DBAPR08MB5814.eurprd08.prod.outlook.com>

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@arm.com] 
Sent: Wednesday, November 25, 2020 5:51 AM
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: Diogo Behrens <diogo.behrens@huawei.com>; thomas@monjalon.net; david.marchand@redhat.com; dev@dpdk.org; nd <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@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@huawei.com>
> > The change looks fine to me.  I have tested this on few x86 and Arm machines.
> > Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@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

  reply	other threads:[~2020-11-25  8:41 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-26  9:20 Diogo Behrens
2020-08-26 10:17 ` Phil Yang
2020-08-27  8:56   ` Diogo Behrens
2020-08-28  9:19     ` Phil Yang
2020-08-31 18:45       ` Honnappa Nagarahalli
2020-10-06 21:49         ` Thomas Monjalon
2020-10-07  9:55           ` Diogo Behrens
2020-10-20 11:56             ` Thomas Monjalon
2020-10-20 21:49               ` Honnappa Nagarahalli
2020-11-22 18:07                 ` Thomas Monjalon
2020-11-23 15:06                   ` Honnappa Nagarahalli
2020-11-23 15:44                     ` Stephen Hemminger
2020-11-23 18:16                       ` Honnappa Nagarahalli
2020-11-23 18:29 ` Honnappa Nagarahalli
2020-11-23 19:36   ` Stephen Hemminger
2020-11-25  4:50     ` Honnappa Nagarahalli
2020-11-25  8:41       ` Diogo Behrens [this message]
2020-11-25 14:16   ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a7d91daf10354b64916726950098e858@huawei.com \
    --to=diogo.behrens@huawei.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=nd@arm.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).