From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6E187377E for ; Thu, 16 Apr 2015 00:52:17 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 15 Apr 2015 15:52:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,584,1422950400"; d="scan'208";a="680807230" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga001.jf.intel.com with ESMTP; 15 Apr 2015 15:52:15 -0700 Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by IRSMSX107.ger.corp.intel.com (163.33.3.99) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 15 Apr 2015 23:52:14 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.2]) by irsmsx155.ger.corp.intel.com ([169.254.14.178]) with mapi id 14.03.0224.002; Wed, 15 Apr 2015 23:52:14 +0100 From: "Ananyev, Konstantin" To: Dong.Wang , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. Thread-Index: AQHQdGz6+8vgd33r00S1uKt4k2uohp1NH2QwgADsIoCAAKA6oA== Date: Wed, 15 Apr 2015 22:52:14 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725821415E37@irsmsx105.ger.corp.intel.com> References: <2601191342CEEE43887BDE71AB97725821415A3A@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. 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: Wed, 15 Apr 2015 22:52:18 -0000 > -----Original Message----- > From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6f= ae@outlook.com] On Behalf Of Dong.Wang > Sent: Wednesday, April 15, 2015 2:46 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv p= kts. >=20 >=20 >=20 > > Hi, > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong > >> Sent: Saturday, April 11, 2015 4:34 PM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pk= ts. > >> > >> Like transmit packets, before update receive descriptor's tail pointer= , rte_wmb() should be added after writing recv descriptor. > >> > >> Signed-off-by: Dong Wang > >> --- > >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/= ixgbe_rxtx.c > >> index 9da2c7e..d504688 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf = **rx_pkts, > >> */ > >> rx_pkts[nb_rx++] =3D rxm; > >> } > >> + > >> + rte_wmb(); > >> + > > > > Why do you think it is necessary? > > I can't see any good reason to put wmb() here. > > I would understand if, at least you'll try to insert it just before upd= ating RDT: > > rx_id =3D (uint16_t) ((rx_id =3D=3D 0) ? > > (rxq->nb_rx_desc - 1) : (rx_id - = 1)); > > + rte_wmb(); > > IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); > > > > That is not needed IA with current implementation, but would make sense= for machines with relaxed memory ordering. > > Though right now DPDK IXGBE PMD is supported only on IA, anyway. > > Same for ixgbe_recv_scattered_pkts(). > > > > Konstantin >=20 > Yes, current implementation works well with IA, and the transmit packets > function's rte_wmb() is also unneccessary. >=20 > But there are two reasons for adding rte_wmb() in recv pkts function: > 1) The memory barrier in recv pkts function and xmit pkts function are > inconsistent, rte_wmb() should be added to recv pkts function or be > removed from xmit pkts function. > 2) DPDK will support PowerPC processor (Other developers are working on > it), I check the memory ordering of PowerPC, there was no mention of > store-store instruction's principle in MPC8544 Reference Manual, only > said it is weak memory ordering. >=20 > So, I think it is neccessary to add rte_wmb() to recv pkts function. >=20 > Dong What I was trying to say: 1. I think you put barrier in a wrong place. Even for machines with weak memory ordering, we need a barrier only when we= are goint to update RDT, i.e: if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE(rxq= ->rdt_reg_addr, ...); } 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to wor= k on machines with weak memory ordering. I think that to make it work properly, you'll need an rmb() bewtween readin= g DD bit and rest of RXD: rxdp =3D &rx_ring[rx_id]; staterr =3D rxdp->wb.upper.status_error; + rte_rmb(); if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) break; rxd =3D *rxdp; 3. As Stephen pointed in his mail, we shouldn't penalise IA implementation = with unnecessary barriers=20 As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-Marc= h/015202.html probably the best is to introduce a new macros: rte_smp_*mb (or something) = that would be architecture dependent: compiler_barrier on IA, proper HW barrier on machines with weak memory orde= ring and update the code to use it. =20 So, if you like to fix that issue, please do that in a proper way. BTW, I think that for PPC support even before touching ixgbe or any other P= MD, step 3 (or similar) need to be done on rte_ring enqueue/dequeue code.=20 Konstantin > > > > > >> rxq->rx_tail =3D rx_id; > >> > >> /* > >> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct= rte_mbuf **rx_pkts, > >> first_seg =3D NULL; > >> } > >> > >> + rte_wmb(); > >> + > >> /* > >> * Record index of the next RX descriptor to probe. > >> */ > >> -- > >> 1.9.1 > >