From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0051.outbound.protection.outlook.com [104.47.2.51]) by dpdk.org (Postfix) with ESMTP id 369A21B28B for ; Mon, 30 Oct 2017 20:47:23 +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=6KCmHOL1toJpPTldvmX3MnO3JcN7uI52e5o6lHAlRuM=; b=Es7rbPFUr/nsUx/SuNrE4hdgYTuJF1GVhTRIyWC/z0V3upqu1xDi/tkBeSiC+LEk6Chg9QkLJxRCtPA5tBMA0NJuq6mtVjwQ2GWUYLmK3oLDgqZdZc7lMjbFqHLT8MmzfKmS/+llfqKBZAnHECtConHfBs4MPXTCjwh4lBBM05Q= Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com (10.167.127.17) by VI1PR05MB1263.eurprd05.prod.outlook.com (10.162.122.141) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.178.6; Mon, 30 Oct 2017 19:47:20 +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; Mon, 30 Oct 2017 19:47:20 +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: AQHTUYq9a24kIUe5FUSj5xzP7LIAbKL8tXqA Date: Mon, 30 Oct 2017 19:47:20 +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> In-Reply-To: <20171030142350.GC26782@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: [85.64.136.190] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; VI1PR05MB1263; 6:G1xUDnvs0SiGDGK0/N0vpBsgKpYw/N4137PX6l+V2GO8QhzPjtirR2E2dRE+sNbcCWrVg7BeKDDiNmguGkF7PmJfAL+SxT67Lfk4BXVQl0wcmUuXQkUg8Ccw4yCfcrrLlMyh2LFJdkUgcdAsKnRs3jhvSWXKR4XdDCIXBvIfWblU30oq0zPP4pRNbMCReHtzvSKozg9tax5ziXmwjvca8Z0/Bb/16gl/E8741jGIAav2fOpG0FDPfsbJDqxaAfOEW6MdL1DkPFgj5jZeDHmPuSXj1r9RGl2+NS4guXqqcD7KOmqY9WJ1Mj4PmVjAmP9utX4W/nVpvP5rdfQN8yBUVnvxwv0SqL8YyQPs4ThjmhU=; 5:hxn2flsljykhnsQR/8bc+hg0dnfbP16OjgOiCcFzjxoPf9LnihF+c8HwSaink65cuC7/rm9q0wKuNqMxO1Bz2gjKmDbZGeQggFxaVMm2XCKUz1anGzA6xZjOOmaUc8W+JFbXuEBdfmKc5rOmZ59ddXscbTtxBT2L8sxP5WxfPH0=; 24:pApRoBckMo3POcoIIC0xQtu5DmptzgBtbcks1LTp3xQBTLudRlkykX/0Sl+GNYOReDJHEiimJCh0CDmriXXppMEOXDzhbkolzWxQMDCd/SY=; 7:acFuT/H6hkPkOsuhUm2UY5Mz3WffK2MT4H6dI4J3r47ZlsMrckaosueiyOuOVNF4P9+Xfrfb4nzU+qMt8AJzqLEjSe3zNNHr1WkrIMu9GcTo4C8NP+hWa3g9fXYpXjZbYOC/vxGFw6unnonBCIHIP2PwmdN2+A7jSptvrs65lgf4z+RZkNd+7cfxE481p01Hm+obtimUSxmh6Ck5tE6nitjH3C4fyJbXvq2kMMdZOwckGZu2ag8N33OnwGZw/VG/ x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 0e9229e7-9915-4b98-2d6f-08d51fcf086e x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(4534020)(4602075)(48565401081)(2017052603199); SRVR:VI1PR05MB1263; x-ms-traffictypediagnostic: VI1PR05MB1263: 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)(5005006)(8121501046)(3002001)(100000703101)(100105400095)(10201501046)(3231020)(93006095)(93001095)(6055026)(6041248)(20161123562025)(20161123558100)(20161123560025)(20161123564025)(20161123555025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:VI1PR05MB1263; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:VI1PR05MB1263; x-forefront-prvs: 0476D4AB88 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(376002)(346002)(39860400002)(189002)(13464003)(199003)(24454002)(7696004)(6916009)(3280700002)(5250100002)(101416001)(66066001)(97736004)(106356001)(25786009)(189998001)(86362001)(105586002)(14454004)(305945005)(74316002)(33656002)(107886003)(68736007)(7736002)(55016002)(53936002)(2950100002)(99286003)(2900100001)(3846002)(6116002)(102836003)(9686003)(93886005)(8936002)(316002)(5660300001)(81156014)(81166006)(54906003)(6246003)(8676002)(2906002)(50986999)(4326008)(478600001)(76176999)(53546010)(6506006)(229853002)(54356999)(3660700001)(6436002); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR05MB1263; 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: 0e9229e7-9915-4b98-2d6f-08d51fcf086e X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Oct 2017 19:47:20.2454 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR05MB1263 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: Mon, 30 Oct 2017 19:47:23 -0000 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 >=20 > 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 >=20 > This sounds like an interesting performance improvement, can you share th= e > typical or expected amount (percentage/hard numbers) for a given use case > as part of the commit log? >=20 Yes, it improves performance, I will share numbers. > More comments below. >=20 > > --- > > 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(); >=20 > 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). >=20 > > #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(); >=20 > This one could be removed entirely as well, which is more or less what th= e > 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 implementati= on. >=20 > On the other hand in my opinion a barrier (compiler or otherwise) might b= e > needed before the doorbell write, to make clear it cannot somehow be done > earlier in case something attempts to optimize it away. >=20 I think we can remove it entirely (compiler can't optimize the ci_db store = since in depends in previous code (cons_index). > > sq->tail =3D sq->tail + nr_txbbs; > > /* Update the list of packets posted for transmission. */ > > elts_comp -=3D pkts; > > @@ -321,6 +321,7 @@ static int handle_multi_segs(struct rte_mbuf *buf, > > * control segment. > > */ > > if ((uintptr_t)dseg & (uintptr_t)(MLX4_TXBB_SIZE - 1)) { > > +#if RTE_CACHE_LINE_SIZE < 64 > > /* > > * Need a barrier here before writing the byte_count > > * fields to make sure that all the data is visible @@ - > 331,6 > > +332,7 @@ static int handle_multi_segs(struct rte_mbuf *buf, > > * data, and end up sending the wrong data. > > */ > > rte_io_wmb(); > > +#endif /* RTE_CACHE_LINE_SIZE */ >=20 > Interesting one. >=20 > > dseg->byte_count =3D byte_count; > > } else { > > /* > > @@ -469,8 +471,7 @@ static int handle_multi_segs(struct rte_mbuf *buf, > > break; > > } > > #endif /* NDEBUG */ > > - /* Need a barrier here before byte count store. */ > > - rte_io_wmb(); > > + /* Never be TXBB aligned, no need compiler barrier. > */ >=20 > The reason there was a barrier here at all was unclear, so if it's really= useless, > you don't even need to describe why. It is because there is a barrier in multi segment similar stage. I think it can help for future review. >=20 > > dseg->byte_count =3D rte_cpu_to_be_32(buf- > >data_len); > > > > /* 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(); >=20 > 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 c= ode. >=20 As I investigated, I understood that for CPUs which don't save store order = between different addresses(arm,ppc), the rte_io_wmb is converted to rte_wm= b. So for thus who save it(x86) we just need the right order in compiler code = because all the relevant stores are targeted to same memory domain(DRAM) an= d 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. > Unless the comment above this barrier is wrong, this change may cause har= d- > to-debug issues down the road, you should drop it. >=20 > > 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 > > >=20 > -- > Adrien Mazarguil > 6WIND Thanks!