Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts.

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@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 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 pointer, rte_wmb() should be added after writing recv descriptor.
> >>
> >> Signed-off-by: Dong Wang <dong.wang.pro@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, ...); }

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;

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

> >
> >
> >>   	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
> >