From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-VE1-obe.outbound.protection.outlook.com (mail-eopbgr50074.outbound.protection.outlook.com [40.107.5.74]) by dpdk.org (Postfix) with ESMTP id EA923914 for ; Thu, 14 Jul 2016 06:17:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=coriant.onmicrosoft.com; s=selector1-coriant-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=e8yN15k/SBsyxV+PAB0MVUARqxHXGoEGo38Q0iVnFP4=; b=crVJFHPTg63bpDVi/xnP1mH7r/cy23B850/YHamScztCWg1YO/mfYzj+Gkpc5YC1d8QVGEbqwXO1NHIa/j130oeKGKSJCMUfapXMBIDZcgqPP/XPKbpuA6LNzAKXCd4QaQznpmL6HQk/IyQ5FgGgFqmvRZlCAVRajyrKS6dTD70= Received: from HE1PR04MB1337.eurprd04.prod.outlook.com (10.163.175.23) by HE1PR04MB1339.eurprd04.prod.outlook.com (10.163.175.25) with Microsoft SMTP Server (TLS) id 15.1.539.14; Thu, 14 Jul 2016 04:17:08 +0000 Received: from HE1PR04MB1337.eurprd04.prod.outlook.com ([10.163.175.23]) by HE1PR04MB1337.eurprd04.prod.outlook.com ([10.163.175.23]) with mapi id 15.01.0539.019; Thu, 14 Jul 2016 04:17:09 +0000 From: "Kuusisaari, Juhamatti" To: "Ananyev, Konstantin" , "'dev@dpdk.org'" Thread-Topic: [dpdk-dev] [PATCH] lib: move rte_ring read barrier to correct location Thread-Index: AQHR214MoG1fIZBOPEC9vEAt0w2bq6ATCsYAgAAIC6CAABd0gIABBgWwgAByaYCAAHRrAIAAs/QAgACLSgCAAPhwgA== Date: Thu, 14 Jul 2016 04:17:09 +0000 Message-ID: References: <20160711102055.14855-1-juhamatti.kuusisaari@coriant.com> <2601191342CEEE43887BDE71AB97725836B7C858@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B7C916@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B7D20B@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B7D628@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B7D850@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B7D850@irsmsx105.ger.corp.intel.com> Accept-Language: fi-FI, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Juhamatti.Kuusisaari@coriant.com; x-originating-ip: [138.111.130.175] x-ms-office365-filtering-correlation-id: 05a7fb6e-10db-4921-7990-08d3ab9db916 x-microsoft-exchange-diagnostics: 1; HE1PR04MB1339; 6:4nY3s94HEFJcbeFZE6dFPUQXpY5UNmKELcrbvQiAF6jso5pP+eukZUftiquy50evTaQKOnX4OVRsXXod5jFRaqKAFPxCwgVOEY6aKelo9S//oex5umWBQPpt3iuWm8DgxlzENv6Uhc4b70eq0I/sjXMtroV5ZCFQ4Yi+FXrgxGZZ4aYLYgbDPtetqJUY5U3QQO5uBbKdA5ivioXq9ZDLoiUXVuRKNwHFrdmkuFNdqdmwsbSG87GVBO540YrafIaAchoiNL23kCtmHenaJDMcCSBROs98Xtfw/lWxZFiCE0uQ4ARIHgAgnq/h320b+HlkhQBJ+lzTnP3T3Eu9r5ux8g==; 5:GwT0ZRkiW9uK4jnMeJBOkI70qUl3LLln9qMW5yb41AnNHQQ7zgrtqMVXaYpa3OhLQg7pZAOB5g2mcqqnWFmmyT9VtJLz16GTDPe97+5vy5u7evF5VU/ruET/mvUJkAi/jD+ef0R6BnIycQK0n9TCfA==; 24:ZcQQqEP0ftZzLeJ5Mi5/jEHmFJ0Kl/DO2E5/Z5HyibfXEMQzUy2mDWQw8xZ2n5ALZuSk48Y0nQlovGZqzYCHo9lIhTdhJliWBNcRQnfZ5ic=; 7:tCXJbybfpxXO78ovBmhk3DIlM8xGUKLpoP2Pt2/S6lUz7H78oghdXA0vYnnPjcppQ/5uJyJ/rBr022h7yLZFBoxgD5G4brqfmtLyY8M3qEKkAjtxAtMa3T+CWZmTiNvTtxhhuegs1AyCsKLZtdik7XnLr/7rmxlVPo/GHSYVf2xDtDTw5zZWwMq8HVQyl488jR5M2e935rDbrHnC78wcbxJnVklRijRAKrQXpa533nlmEgFcja3ksxzgcbczerj3 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:HE1PR04MB1339; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(51653755401839); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026); SRVR:HE1PR04MB1339; BCL:0; PCL:0; RULEID:; SRVR:HE1PR04MB1339; x-forefront-prvs: 00032065B2 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(189002)(51444003)(199003)(10400500002)(5001770100001)(19580395003)(189998001)(106116001)(2900100001)(106356001)(101416001)(97736004)(107886002)(3280700002)(5002640100001)(3660700001)(87936001)(81166006)(9686002)(2950100001)(19580405001)(68736007)(66066001)(81156014)(122556002)(76176999)(54356999)(50986999)(93886004)(2906002)(77096005)(33656002)(76576001)(74316002)(86362001)(5003600100003)(7736002)(7696003)(8936002)(7846002)(305945005)(92566002)(586003)(6116002)(3846002)(102836003)(105586002)(561944003)(491001); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR04MB1339; H:HE1PR04MB1337.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: coriant.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: coriant.com X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jul 2016 04:17:09.1573 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 76595477-907e-4695-988b-a6b39087332d X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR04MB1339 Subject: Re: [dpdk-dev] [PATCH] lib: move rte_ring read barrier to correct location X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jul 2016 04:17:11 -0000 Hi Konstantin, =20 > > > > > It is quite safe to move the barrier before DEQUEUE because after > > > > > the DEQUEUE there is nothing really that we would want to protect > with a > > > read barrier. > > > > > > > > I don't think so. > > > > If you remove barrier after DEQUEUE(), that means on systems with > > > > relaxed memory ordering cons.tail could be updated before DEQUEUE() > > > > will be finished and producer can overwrite queue entries that were > not > > > yet dequeued. > > > > So if cpu can really do such speculative out of order loads, then w= e > > > > do need for __rte_ring_sc_do_dequeue() something like: > > > > > > > > rte_smp_rmb(); > > > > DEQUEUE_PTRS(); > > > > rte_smp_rmb(); > > > > You have a valid point here, there needs to be a guarantee that cons_ta= il > cannot > > be updated before DEQUEUE is completed. Nevertheless, my point was > that it is > > not guaranteed with a read barrier anyway. The implementation has the > following > > sequence > > > > DEQUEUE_PTRS(); (i.e. READ/LOAD) > > rte_smp_rmb(); > > .. > > r->cons.tail =3D cons_next; (i.e WRITE/STORE) > > > > Above read barrier does not guarantee any ordering for the following > writes/stores. > > As a guarantee is needed, I think we in fact need to change the read ba= rrier > on the > > dequeue to a full barrier, which guarantees the read+write order, as > follows > > > > DEQUEUE_PTRS(); > > rte_smp_mb(); > > .. > > r->cons.tail =3D cons_next; > > > > If you agree, I can for sure prepare another patch for this issue. >=20 > Hmm, I think for __rte_ring_mc_do_dequeue() we are ok with smp_rmb(), > as we have to read cons.tail anyway. Are you certain that this read creates strong enough dependency between read of cons.tail and the write of it on the mc_do_dequeue()? I think it do= es not=20 really create any control dependency there as the next write is not depende= nt=20 of the result of the read. The CPU also knows already the value that will b= e written=20 to cons.tail and that value does not depend on the previous read either. Th= e CPU=20 does not know we are planning to do a spinlock there, so it might do things= =20 out-of-order without proper dependencies. > For __rte_ring_sc_do_dequeue(), I think you right, we might need > something stronger. > I don't want to put rte_smp_mb() here as it would cause full HW barrier e= ven > on machines > with strong memory order (IA). > I think that rte_smp_wmb() might be enough here: > it would force cpu to wait till writes in DEQUEUE_PTRS() are become visib= le, > which > means reads have to be completed too. In practice I think that rte_smp_wmb() would work fine, even though it is n= ot=20 strictly according to the book. Below solution would be my proposal as a fi= x to the=20 issue of sc dequeueing (and also to mc dequeueing, if we have the problem o= f CPU=20 completely ignoring the spinlock in reality there): DEQUEUE_PTRS(); .. rte_smp_wmb(); r->cons.tail =3D cons_next; -- Juhamatti > Another option would be to define a new macro: rte_weak_mb() or so, > that would be expanded into CB on boxes with strong memory model, > and to full MB on machines with relaxed ones. > Interested to hear what ARM and PPC guys think. > Konstantin >=20 > P.S. Another thing a bit off-topic - for PPC guys: > As I can see smp_rmb/smp_wmb are just a complier barriers: > find lib/librte_eal/common/include/arch/ppc_64/ -type f | xargs grep smp_ > lib/librte_eal/common/include/arch/ppc_64/rte_atomic.h:#define > rte_smp_mb() rte_mb() > lib/librte_eal/common/include/arch/ppc_64/rte_atomic.h:#define > rte_smp_wmb() rte_compiler_barrier() > lib/librte_eal/common/include/arch/ppc_64/rte_atomic.h:#define > rte_smp_rmb() rte_compiler_barrier() > My knowledge about PPC architecture is rudimental, but is that really eno= ugh? >=20 > > > > Thanks, > > -- > > Juhamatti > > > > > > Konstantin > > > > > > > > > The read > > > > > barrier is mapped to a compiler barrier on strong memory model > > > > > systems and this works fine too as the order of the head,tail > > > > > updates is still guaranteed on the new location. Even if the prob= lem > > > > > would be theoretical on most systems, it is worth fixing as the r= isk for > > > problems is very low. > > > > > > > > > > -- > > > > > Juhamatti > > > > > > > > > > > Konstantin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Juhamatti Kuusisaari > > > > > > > > > > > > > > > > > > --- > > > > > > > > > lib/librte_ring/rte_ring.h | 6 ++++-- > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/lib/librte_ring/rte_ring.h > > > > > > > > > b/lib/librte_ring/rte_ring.h index eb45e41..a923e49 10064= 4 > > > > > > > > > --- a/lib/librte_ring/rte_ring.h > > > > > > > > > +++ b/lib/librte_ring/rte_ring.h > > > > > > > > > @@ -662,9 +662,10 @@ __rte_ring_mc_do_dequeue(struct > > > > > > > > > rte_ring *r, > > > > > > > > void **obj_table, > > > > > > > > > cons_next); > > > > > > > > > } while (unlikely(success =3D=3D 0)); > > > > > > > > > > > > > > > > > > + rte_smp_rmb(); > > > > > > > > > + > > > > > > > > > /* copy in table */ > > > > > > > > > DEQUEUE_PTRS(); > > > > > > > > > - rte_smp_rmb(); > > > > > > > > > > > > > > > > > > /* > > > > > > > > > * If there are other dequeues in progress that > > > > > > > > > preceded us, @@ -746,9 +747,10 @@ > > > > > > > > > __rte_ring_sc_do_dequeue(struct rte_ring *r, > > > > > > > > void **obj_table, > > > > > > > > > cons_next =3D cons_head + n; > > > > > > > > > r->cons.head =3D cons_next; > > > > > > > > > > > > > > > > > > + rte_smp_rmb(); > > > > > > > > > + > > > > > > > > > /* copy in table */ > > > > > > > > > DEQUEUE_PTRS(); > > > > > > > > > - rte_smp_rmb(); > > > > > > > > > > > > > > > > > > __RING_STAT_ADD(r, deq_success, n); > > > > > > > > > r->cons.tail =3D cons_next; > > > > > > > > > -- > > > > > > > > > 2.9.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > > > > > =3D=3D > > > > > > > > > The information contained in this message may be privileg= ed > > > > > > > > > and confidential and protected from disclosure. If the > > > > > > > > > reader of this message is not the intended recipient, or = an > > > > > > > > > employee or agent responsible for delivering this message= to > > > > > > > > > the intended recipient, you are hereby notified that any > > > > > > > > > reproduction, dissemination or distribution of this > > > > > > > > > communication is strictly prohibited. If you have receive= d > > > > > > > > > this communication in error, please notify us immediately= by > > > > > > > > > replying to the message and deleting it from your > > > > > > computer. Thank you. > > > > > > > > > Coriant-Tellabs > > > > > > > > > > > > > > > > > > > > > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > > > > > =3D=3D