From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8B30DA0527; Wed, 25 Nov 2020 09:41:40 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0617EC940; Wed, 25 Nov 2020 09:41:38 +0100 (CET) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by dpdk.org (Postfix) with ESMTP id 957F8C93C for ; Wed, 25 Nov 2020 09:41:36 +0100 (CET) Received: from fraeml712-chm.china.huawei.com (unknown [172.18.147.207]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4CgvTL1cB2z67HmK; Wed, 25 Nov 2020 16:39:50 +0800 (CST) Received: from lhreml736-chm.china.huawei.com (10.201.108.87) by fraeml712-chm.china.huawei.com (10.206.15.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Wed, 25 Nov 2020 09:41:34 +0100 Received: from lhreml728-chm.china.huawei.com (10.201.108.79) by lhreml736-chm.china.huawei.com (10.201.108.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1913.5; Wed, 25 Nov 2020 08:41:34 +0000 Received: from lhreml728-chm.china.huawei.com ([10.201.108.79]) by lhreml728-chm.china.huawei.com ([10.201.108.79]) with mapi id 15.01.1913.007; Wed, 25 Nov 2020 08:41:34 +0000 From: Diogo Behrens To: Honnappa Nagarahalli , Stephen Hemminger CC: "thomas@monjalon.net" , "david.marchand@redhat.com" , "dev@dpdk.org" , nd Thread-Topic: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory Thread-Index: AQHWwcaSOqSFXnd9v02KDV0dJx5bZ6nWHG2AgAItIgCAADntMA== Date: Wed, 25 Nov 2020 08:41:34 +0000 Message-ID: References: <20200826092002.19395-1-diogo.behrens@huawei.com> <20201123113651.46323c54@hermes.local> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.206.134.146] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Stephen,=20 the patch we submitted is safe, it has been verified that it indeed fixes t= he bug: Besides rmem and genmc model checkers, we verified the bugfix with herd7 to= ol, which is the official tool ARM provides to check such pieces of code agains= t 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 optimizati= on. This patch is just to fix the bug. Best regards, -Diogo -----Original Message----- From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]=20 Sent: Wednesday, November 25, 2020 5:51 AM To: Stephen Hemminger Cc: Diogo Behrens ; thomas@monjalon.net; david.ma= rchand@redhat.com; dev@dpdk.org; nd ; Honnappa Nagarahalli ; nd Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory > > > > > > > > The initialization me->locked=3D1 in lock() must happen before > > > next->locked=3D0 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=3D1 to be reordered = with > > > announcing the node (pred->next=3Dme) and, consequently, to be > > > reordered with next->locked=3D0 in unlock(). > > > > > > This fix adds a release barrier to pred->next=3Dme, forcing > > > me->locked=3D1 to happen before this operation. > > > > > > Signed-off-by: Diogo Behrens > > The change looks fine to me. I have tested this on few x86 and Arm mac= hines. > > Acked-by: Honnappa Nagarahalli >=20 > 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. >=20 > 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; >=20 > /* Init me node */ > - __atomic_store_n(&me->locked, 1, __ATOMIC_RELAXED); > - __atomic_store_n(&me->next, NULL, __ATOMIC_RELAXED); > + me->locked =3D 1; >=20 > - /* 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 =3D __atomic_exchange_n(msl, me, __ATOMIC_ACQ_REL); > + do { > + prev =3D __atomic_load_n(msl, __ATOMIC_RELAXED); > + me->next =3D prev; This needs to be __atomic_store_n(__ATOMIC_RELEASE) as it can sink below th= e following line. > + } while (!__atomic_compare_exchange_n(&msl, me, prev, > + __ATOMIC_ACQUIRE, > __ATOMIC_RELAXED)); > + > if (likely(prev =3D=3D NULL)) { > /* Queue was empty, no further action required, > * proceed with lock taken. > */ > return; > } > - __atomic_store_n(&prev->next, me, __ATOMIC_RELAXED); >=20 > /* The while-load of me->locked should not move above the previous > * store to prev->next. Otherwise it will cause a deadlock. Need a