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.zh...@intel.com]
Sent: Friday, 28 April 2023 05.44
From: Min Zhou <zhou...@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 <zhou...@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
