OK, let me start it~~~ I'll send another patch after several days. Dong
--- Original Message --- From: "Ananyev, Konstantin" <konstantin.anan...@intel.com> Sent: April 16, 2015 11:14 PM To: "Wang Dong" <dong.wang.pro at hotmail.com>, dev at dpdk.org Subject: RE: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > -----Original Message----- > From: outlook_739db8e1c4bc6fae at outlook.com > [mailto:outlook_739db8e1c4bc6fae at outlook.com] On Behalf Of Wang Dong > Sent: Thursday, April 16, 2015 12:36 PM > To: Ananyev, Konstantin; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > > > > > > >> -----Original Message----- > >> From: outlook_739db8e1c4bc6fae at outlook.com > >> [mailto:outlook_739db8e1c4bc6fae at outlook.com] On Behalf Of Dong.Wang > >> Sent: Wednesday, April 15, 2015 2:46 PM > >> To: Ananyev, Konstantin; dev at dpdk.org > >> Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv > >> pkts. > >> > >> > >> > >>> Hi, > >>> > >>>> -----Original Message----- > >>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of WangDong > >>>> Sent: Saturday, April 11, 2015 4:34 PM > >>>> To: dev at dpdk.org > >>>> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > >>>> > >>>> Like transmit packets, before update receive descriptor's tail pointer, > >>>> rte_wmb() should be added after writing recv descriptor. > >>>> > >>>> Signed-off-by: Dong Wang <dong.wang.pro at hotmail.com> > >>>> --- > >>>> 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++] = 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 > >>> updating RDT: > >>> rx_id = (uint16_t) ((rx_id == 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 > >> > >> Yes, current implementation works well with IA, and the transmit packets > >> 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 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. > >> > >> 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 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, ...); } > 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 > > reading DD bit and rest of RXD: > > > > rxdp = &rx_ring[rx_id]; > > staterr = rxdp->wb.upper.status_error; > > + rte_rmb(); > > if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > > break; > > rxd = *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 implementation > > 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 something) > > 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 other > > 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 :) Konstantin > > Dong > > > >>> > >>> > >>>> rxq->rx_tail = rx_id; > >>>> > >>>> /* > >>>> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct > >>>> rte_mbuf **rx_pkts, > >>>> first_seg = NULL; > >>>> } > >>>> > >>>> + rte_wmb(); > >>>> + > >>>> /* > >>>> * Record index of the next RX descriptor to probe. > >>>> */ > >>>> -- > >>>> 1.9.1 > >>>
