Re: [PATCH v2] net/ixgbe: add proper memory barriers for some Rx functions

[zhoumin <zhoumin@loongson.cn>]

Hi Morten,

Thanks for your comments.

On Fri, Apr 28, 2023 at 2:27PM, Morten Brørup wrote:
>> From: Zhang, Qi Z [mailto:qi.z.zhang@intel.com]
>> Sent: Friday, 28 April 2023 05.44
>>
>>> From: Min Zhou <zhoumin@loongson.cn>
>>> Sent: Monday, April 24, 2023 5:06 PM
>>>
>>> Segmentation fault has been observed while running the
>>> ixgbe_recv_pkts_lro() function to receive packets on the Loongson 3C5000
>>> processor which has 64 cores and 4 NUMA nodes.
>>>
>>>  From the ixgbe_recv_pkts_lro() function, we found that as long as the first
>>> packet has the EOP bit set, and the length of this packet is less than or
>> equal
>>> to rxq->crc_len, the segmentation fault will definitely happen even though
>>> on the other platforms, such as X86.
>>>
>>> Because when processd the first packet the first_seg->next will be NULL, if
>> at
>>> the same time this packet has the EOP bit set and its length is less than or
>>> equal to rxq->crc_len, the following loop will be excecuted:
>>>
>>>      for (lp = first_seg; lp->next != rxm; lp = lp->next)
>>>          ;
>>>
>>> We know that the first_seg->next will be NULL under this condition. So the
>>> expression of lp->next->next will cause the segmentation fault.
>>>
>>> Normally, the length of the first packet with EOP bit set will be greater
>> than
>>> rxq->crc_len. However, the out-of-order execution of CPU may make the
>>> read ordering of the status and the rest of the descriptor fields in this
>>> function not be correct. The related codes are as following:
>>>
>>>          rxdp = &rx_ring[rx_id];
>>>   #1     staterr = rte_le_to_cpu_32(rxdp->wb.upper.status_error);
>>>
>>>          if (!(staterr & IXGBE_RXDADV_STAT_DD))
>>>              break;
>>>
>>>   #2     rxd = *rxdp;
>>>
>>> The sentence #2 may be executed before sentence #1. This action is likely to
>>> make the ready packet zero length. If the packet is the first packet and has
>>> the EOP bit set, the above segmentation fault will happen.
>>>
>>> So, we should add rte_rmb() to ensure the read ordering be correct. We also
>>> did the same thing in the ixgbe_recv_pkts() function to make the rxd data be
>>> valid even thougth we did not find segmentation fault in this function.
>>>
>>> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
>>> ---
>>> v2:
>>> - Make the calling of rte_rmb() for all platforms
>>> ---
>>>   drivers/net/ixgbe/ixgbe_rxtx.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>>> index c9d6ca9efe..302a5ab7ff 100644
>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>>> @@ -1823,6 +1823,8 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf
>>> **rx_pkts,
>>>   		staterr = rxdp->wb.upper.status_error;
>>>   		if (!(staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD)))
>>>   			break;
>>> +
>>> +		rte_rmb();
>> So "volatile" does not prevent re-order with Loongson compiler?
> "Volatile" does not prevent re-ordering on any compiler. "Volatile" only prevents caching of the variable marked volatile.
>
> https://wiki.sei.cmu.edu/confluence/display/c/CON02-C.+Do+not+use+volatile+as+a+synchronization+primitive
>
> Thinking out loud: I don't know the performance cost of rte_rmb(); perhaps using atomic accesses with the optimal memory ordering would be a better solution in the long term.
Yes, rte_rmb() probably had side effects on the performance. I will use 
a better solution to solve the problem in the V2 patch.
>>
>>>   		rxd = *rxdp;
>>>
>>>   		/*
>>> @@ -2122,6 +2124,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct
>>> rte_mbuf **rx_pkts, uint16_t nb_pkts,
>>>   		if (!(staterr & IXGBE_RXDADV_STAT_DD))
>>>   			break;
>>>
>>> +		rte_rmb();
>>>   		rxd = *rxdp;
>>>
>>>   		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
>>> --
>>> 2.31.1

Best regards,

Min