From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 881F542A12; Fri, 28 Apr 2023 08:27:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2AA7F4113C; Fri, 28 Apr 2023 08:27:17 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 55B41406B5 for ; Fri, 28 Apr 2023 08:27:15 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v2] net/ixgbe: add proper memory barriers for some Rx functions Date: Fri, 28 Apr 2023 08:27:11 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D878C7@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v2] net/ixgbe: add proper memory barriers for some Rx functions Thread-Index: AQHZdpSOGh/lb/NsM0CLv5nxkIU7VK9AGVsAgAArwQA= References: <20230424090532.367194-1-zhoumin@loongson.cn> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Zhang, Qi Z" , "Min Zhou" , "Yang, Qiming" , "Wu, Wenjun1" Cc: , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Zhang, Qi Z [mailto:qi.z.zhang@intel.com] > Sent: Friday, 28 April 2023 05.44 >=20 > > From: Min Zhou > > 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 =3D first_seg; lp->next !=3D rxm; lp =3D 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 =3D &rx_ring[rx_id]; > > #1 staterr =3D rte_le_to_cpu_32(rxdp->wb.upper.status_error); > > > > if (!(staterr & IXGBE_RXDADV_STAT_DD)) > > break; > > > > #2 rxd =3D *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 > > --- > > 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 =3D rxdp->wb.upper.status_error; > > if (!(staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > > break; > > + > > + rte_rmb(); >=20 > 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+volatil= e+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. >=20 >=20 > > rxd =3D *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 =3D *rxdp; > > > > PMD_RX_LOG(DEBUG, "port_id=3D%u queue_id=3D%u rx_id=3D%u " > > -- > > 2.31.1