From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 8449E3237 for ; Thu, 16 Apr 2015 17:14:53 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 16 Apr 2015 08:14:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,588,1422950400"; d="scan'208";a="557145051" Received: from irsmsx108.ger.corp.intel.com ([163.33.3.3]) by orsmga003.jf.intel.com with ESMTP; 16 Apr 2015 08:14:03 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.2]) by IRSMSX108.ger.corp.intel.com ([169.254.11.216]) with mapi id 14.03.0224.002; Thu, 16 Apr 2015 16:14:01 +0100 From: "Ananyev, Konstantin" To: Wang Dong , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. Thread-Index: AQHQdGz6+8vgd33r00S1uKt4k2uohp1NH2QwgADsIoCAAKA6oIAAzbkAgAAeYQA= Date: Thu, 16 Apr 2015 15:14:00 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772582141626B@irsmsx105.ger.corp.intel.com> References: <2601191342CEEE43887BDE71AB97725821415A3A@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725821415E37@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.181] 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: Thu, 16 Apr 2015 15:14:54 -0000 > -----Original Message----- > From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6f= ae@outlook.com] On Behalf Of Wang Dong > Sent: Thursday, April 16, 2015 12:36 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv p= kts. >=20 > > > > > >> -----Original Message----- > >> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4b= c6fae@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 rec= v pkts. > >> > >> > >> > >>> 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 = pkts. > >>>> > >>>> Like transmit packets, before update receive descriptor's tail point= er, 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_ixgb= e/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_mbu= f **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 u= pdating 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 sen= se 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 > >> > >> Yes, current implementation works well with IA, and the transmit packe= ts > >> function's rte_wmb() is also unneccessary. > >> > >> 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 o= n > >> 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. > >> > >> So, I think it is neccessary to add rte_wmb() to recv pkts function. > >> > >> 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 whe= n we are goint to update RDT, i.e: > > if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE= (rxq->rdt_reg_addr, ...); } > Yes, I put it in a wrong place, it will reduce performance. It's better > to place it in that you suggested. > > > > 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to= work on machines with weak memory ordering. > > I think that to make it work properly, you'll need an rmb() bewtween re= ading 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; > Yes, it seems wmb is not enough for weak memory ordering processor. Both > rmb and wmb are needed. > > > > 3. As Stephen pointed in his mail, we shouldn't penalise IA implementat= ion with unnecessary barriers > > As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-= March/015202.html > > probably the best is to introduce a new macros: rte_smp_*mb (or somethi= ng) that would be architecture dependent: > > compiler_barrier on IA, proper HW barrier on machines with weak memory = ordering and update the code to use it. > > > > 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 oth= er PMD, > > step 3 (or similar) need to be done on rte_ring enqueue/dequeue code. > > > > Konstantin > Yes, a new set of macros should be introduced first, then we can update > PMD code. Did anyone are working on it now ? As far as I know, no one is working on it right now. So, I suppose, you are welcome to start :)=20 Konstantin >=20 > Dong > > > >>> > >>> > >>>> rxq->rx_tail =3D rx_id; > >>>> > >>>> /* > >>>> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, stru= ct rte_mbuf **rx_pkts, > >>>> first_seg =3D NULL; > >>>> } > >>>> > >>>> + rte_wmb(); > >>>> + > >>>> /* > >>>> * Record index of the next RX descriptor to probe. > >>>> */ > >>>> -- > >>>> 1.9.1 > >>>