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

[Dong.Wang <dong.wang.pro@hotmail.com>]

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