From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0064.outbound.protection.outlook.com [104.47.1.64]) by dpdk.org (Postfix) with ESMTP id 19CAC1B235 for ; Tue, 31 Oct 2017 12:35:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=1BjFCHXBTh8IUMXi8hqUq51sSFEEfYwkKeU6ZGSiS8Y=; b=cMrZ1/Xlxey+j/FWGaaTi48x0g+nCgm2P08tyv1LZFlICSbceX8VAEuwJkR1eg3qsYWdsSIB+KngHlKkcglAtqAaPZJTia/+t1YM/SdOXJWBqIYVVaiGsSrUmOYE/ylYmeamiWiQm6LCcbHI2KCuO4nyKKTMBLY85ygCDh0AAw0= Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com (10.167.127.17) by AM3PR05MB1251.eurprd05.prod.outlook.com (10.163.7.141) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.178.6; Tue, 31 Oct 2017 11:35:30 +0000 Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b]) by HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b%13]) with mapi id 15.20.0178.012; Tue, 31 Oct 2017 11:35:29 +0000 From: Matan Azrad To: Adrien Mazarguil CC: "dev@dpdk.org" , Ophir Munk Thread-Topic: [PATCH v3 6/7] net/mlx4: mitigate Tx path memory barriers Thread-Index: AQHTUYq9a24kIUe5FUSj5xzP7LIAbKL8tXqAgAEKUQCAABBh4A== Date: Tue, 31 Oct 2017 11:35:29 +0000 Message-ID: References: <1508768520-4810-1-git-send-email-ophirmu@mellanox.com> <1509358049-18854-1-git-send-email-matan@mellanox.com> <1509358049-18854-7-git-send-email-matan@mellanox.com> <20171030142350.GC26782@6wind.com> <20171031101722.GL26782@6wind.com> In-Reply-To: <20171031101722.GL26782@6wind.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM3PR05MB1251; 6:z/Pu7iWkh1iLpCnWdFD3NjMh7dY9Z+mgitSmzTsEIq4HmayX8At9w5eRaPKC0eqsloJZ+E1fWEUa4OQXqkUgMhbTOevwbl+5eyLevtmXBAhj89NrM+dWMnSdKCSI1CaZmBgrkWm7xJr2omhOfjlhCSu5koqErm6PdlyiuA7w29PwH42n5RghHzyRHKoM4U2FSUIsMP69Kn+vnmFZce6Urmrpz/SbfMl029wpE54FB67j4LXBdpVLKHUVOq1hy3LTL0F5Invwh4GMHd0wMdGQtDYuCNiKCzPFN7HNTIk1DofFWWLeV4LSKITCJPPEpen5p7VEzIURUBa4ctD7sc74HjYOKnbfapVi46xLngYEI74=; 5:LWvVmAVjZ0WVM9xcAG8X347sCOpRDtthkFuPpJyiE5U16SBVloGGtHsvy6vrbhVYofga43Xp18fiuSxTcEsxgr1AbdOWYZk2+9VyOlOD8lFxCLd2YjR1u8a05EBehlpXNaCHRnjpVk3ObgpKD1ecnHcOV4dPhB9w0nv/h4bPOGI=; 24:YhNaoEW2KXbf/Y+zANbhu7Yck/CppwLXdADat0wzddDevG05204Ce9XJulLU/nJCKg+2EPSaXsE31QLcy9Ho20kbBAqSgv0HMalDdyt380w=; 7:AC2/rFj/AS5WeIj+pJwGCRzrvYp3X859GBHgRGd96NvK9Wcuj1FE0j8BUlprBC0NykQf59S+jR06Xhu64AJtyKi3M/owv3Iham/8a7aQxu6/nMoOF4ekDZlVk2WXKjUKP3+D3MG22d746PNI36d1hJYlWJyR9ULZ9HUkOLcB3NFZ1gsuYiiF4LDMnLKTfTOf7NT9hb7YyT4baHi2aPgPblrBDCLYGVCZTeFl4wdmEODk2yJO/j40sBMAQZi+TNnq x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 41e1496c-3353-4f0a-95ad-08d520537d32 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081)(4534020)(4602075)(2017052603199); SRVR:AM3PR05MB1251; x-ms-traffictypediagnostic: AM3PR05MB1251: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-exchange-antispam-report-test: UriScan:(60795455431006)(788757137089); x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(100000703101)(100105400095)(93006095)(93001095)(3231020)(10201501046)(3002001)(6055026)(6041248)(20161123560025)(20161123562025)(20161123555025)(20161123558100)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:AM3PR05MB1251; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:AM3PR05MB1251; x-forefront-prvs: 04772EA191 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39860400002)(346002)(376002)(189002)(199003)(76104003)(24454002)(51444003)(13464003)(7736002)(6506006)(6246003)(107886003)(2900100001)(106356001)(6436002)(102836003)(6116002)(3846002)(14454004)(5660300001)(9686003)(97736004)(105586002)(229853002)(189998001)(5250100002)(86362001)(53936002)(7696004)(55016002)(99286003)(2950100002)(66066001)(81166006)(81156014)(4326008)(8936002)(6916009)(50986999)(74316002)(76176999)(54356999)(316002)(33656002)(305945005)(54906003)(68736007)(2906002)(3660700001)(93886005)(25786009)(8676002)(53546010)(3280700002)(478600001)(101416001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM3PR05MB1251; H:HE1PR0502MB3659.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.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: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 41e1496c-3353-4f0a-95ad-08d520537d32 X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Oct 2017 11:35:29.7009 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR05MB1251 Subject: Re: [dpdk-dev] [PATCH v3 6/7] net/mlx4: mitigate Tx path memory barriers 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: , X-List-Received-Date: Tue, 31 Oct 2017 11:35:32 -0000 Hi Adrien > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Tuesday, October 31, 2017 12:17 PM > To: Matan Azrad > Cc: dev@dpdk.org; Ophir Munk > Subject: Re: [PATCH v3 6/7] net/mlx4: mitigate Tx path memory barriers >=20 > Hi Matan, >=20 > On Mon, Oct 30, 2017 at 07:47:20PM +0000, Matan Azrad wrote: > > Hi Adrien > > > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > Sent: Monday, October 30, 2017 4:24 PM > > > To: Matan Azrad > > > Cc: dev@dpdk.org; Ophir Munk > > > Subject: Re: [PATCH v3 6/7] net/mlx4: mitigate Tx path memory > > > barriers > > > > > > On Mon, Oct 30, 2017 at 10:07:28AM +0000, Matan Azrad wrote: > > > > Replace most of the memory barriers by compiler barriers since > > > > they are all targeted to the DRAM; This improves code efficiency > > > > for systems which force store order between different addresses. > > > > > > > > Only the doorbell record store should be protected by memory > > > > barrier since it is targeted to the PCI memory domain. > > > > > > > > Limit pre byte count store compiler barrier for systems with cache > > > > line size smaller than 64B (TXBB size). > > > > > > > > Signed-off-by: Matan Azrad > > > > > > This sounds like an interesting performance improvement, can you > > > share the typical or expected amount (percentage/hard numbers) for a > > > given use case as part of the commit log? > > > > > > > Yes, it improves performance, I will share numbers. >=20 > First I must add I thought rte_io_[rw]mb() was really only a renamed > compiler barrier, I better understand its purpose now, thanks. >=20 > (more below.) >=20 > > > More comments below. > > > > > > > --- > > > > drivers/net/mlx4/mlx4_rxtx.c | 11 ++++++----- > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/mlx4/mlx4_rxtx.c > > > > b/drivers/net/mlx4/mlx4_rxtx.c index 8ea8851..482c399 100644 > > > > --- a/drivers/net/mlx4/mlx4_rxtx.c > > > > +++ b/drivers/net/mlx4/mlx4_rxtx.c > > > > @@ -168,7 +168,7 @@ struct pv { > > > > /* > > > > * Make sure we read the CQE after we read the ownership > > > bit. > > > > */ > > > > - rte_rmb(); > > > > + rte_io_rmb(); > > > > > > OK for this one since the rest of the code should not be run due to > > > the condition (I'm not even sure even a compiler barrier is necessary= at all > here). > > > > > > > #ifndef NDEBUG > > > > if (unlikely((cqe->owner_sr_opcode & > > > MLX4_CQE_OPCODE_MASK) =3D=3D > > > > MLX4_CQE_OPCODE_ERROR)) { @@ -203,7 +203,7 > @@ struct pv { > > > > */ > > > > cq->cons_index =3D cons_index; > > > > *cq->set_ci_db =3D rte_cpu_to_be_32(cq->cons_index & > > > MLX4_CQ_DB_CI_MASK); > > > > - rte_wmb(); > > > > + rte_io_wmb(); > > > > > > This one could be removed entirely as well, which is more or less > > > what the move to a compiler barrier does. Nothing in subsequent code > > > depends on this doorbell being written, so this can piggy back on > > > any subsequent rte_wmb(). > > > > Yes, you right, probably this code was taken from multi thread > implementation. > > > > > > On the other hand in my opinion a barrier (compiler or otherwise) > > > might be needed before the doorbell write, to make clear it cannot > > > somehow be done earlier in case something attempts to optimize it > away. > > > > > I think we can remove it entirely (compiler can't optimize the ci_db st= ore > since in depends in previous code (cons_index). >=20 > Right, however you may still run into issues if the compiler determines t= he > final cons_index value by looking at the loop and decides to store it bef= ore > entering/leaving it. That's the kind of problematic optimization I was th= inking > of. >=20 > The barrier in that sense is just to assert the order of seemingly unrela= ted > load/stores. I think that If I left the rte_io_rmb after CQE owner check we can earn bot= h: 1. The concern of read ordering while reading the owner before using CQE. 2. The ci_db concern: the compiler must read the last CQE(which is not vali= d and we have no more stamp to do) before it knows the last value of cons_i= ndex.=20 So we can remove entirely this rte_io_wmb in completion function. What do you think?=20 =20 > > > > /* Fill the control parameters for this packet. */ @@ - > > > 533,7 > > > > +534,7 @@ static int handle_multi_segs(struct rte_mbuf *buf, > > > > * setting ownership bit (because HW can start > > > > * executing as soon as we do). > > > > */ > > > > - rte_wmb(); > > > > + rte_io_wmb(); > > > > > > This one looks dangerous. A compiler barrier is not strong enough to > > > guarantee the order in which CPU will execute instructions, it only > > > makes sure what follows the barrier doesn't appear before it in the > generated code. > > > > > As I investigated, I understood that for CPUs which don't save store or= der > between different addresses(arm,ppc), the rte_io_wmb is converted to > rte_wmb. > > So for thus who save it(x86) we just need the right order in compiler c= ode > because all the relevant stores are targeted to same memory domain(DRAM) > and therefore also the actual store is guaranteed. > > Unlike doorbell store which directed to different memory domain (PCI). > > So the only place which need rte_wmb() is before doorbell write. >=20 > Fair enough, although after re-reading the code I think there's another i= ssue > present since the beginning: both ctrl and dseg pointers are not volatile= , this > fact doesn't guarantee intermediate writes will occur in the expected ord= er > or even at all, even in the presence of a barrier. >=20 > The volatile attribute should be inherited from both struct mlx4_cq and s= truct > mlx4_sq (buf, db and most if not all other pointers). I think a separate = fixes > commit should add it for safety. Great notice , I will add it, Thanks! >=20 > > > Unless the comment above this barrier is wrong, this change may > > > cause hard- to-debug issues down the road, you should drop it. > > > > > > > ctrl->owner_opcode =3D rte_cpu_to_be_32(owner_opcode | > > > > ((sq->head & sq->txbb_cnt) ? > > > > MLX4_BIT_WQE_OWN : > > > 0)); > > > > -- > > > > 1.8.3.1 > > > > > > > > > > -- > > > Adrien Mazarguil > > > 6WIND > > > > Thanks! >=20 > -- > Adrien Mazarguil > 6WIND