From patchwork Wed Nov 1 03:32:40 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jiawen Wu X-Patchwork-Id: 133696 X-Patchwork-Delegate: ferruh.yigit@amd.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2C7364316B; Wed, 1 Nov 2023 04:21:23 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7AEE14113D; Wed, 1 Nov 2023 04:21:16 +0100 (CET) Received: from smtpbg153.qq.com (smtpbg153.qq.com [13.245.218.24]) by mails.dpdk.org (Postfix) with ESMTP id F2313402CF; Wed, 1 Nov 2023 04:21:11 +0100 (CET) X-QQ-mid: bizesmtp86t1698808861tru1c8xr Received: from wxdbg.localdomain.com ( [125.120.145.41]) by bizesmtp.qq.com (ESMTP) with id ; Wed, 01 Nov 2023 11:20:53 +0800 (CST) X-QQ-SSF: 01400000000000K0Z000000A0000000 X-QQ-FEAT: +oIWmpEafD9ubEGEdnz2rT/UdIRe9aHRvT4MxlKG0xqyRxISdIBPTTithGHqi +i2c269jI09GV9lmDhuOHsIL3Jws8cBU/tX3OS66DbwHuQPGzq2WFFSi9IhGkB12Dk89mVp d14ljNwmZjeK7z/Uj8N/nrUHb1irPlgyTg9wO0ZbfmKMJzlxOkxihFSFs76WOUAizLtjA4r /Y+zVWI134DhuFFqLViuL9f8Tz/C7SDL0TPKN3FIwMG3hi9xXjz/iyhoCGIbTboiCYUnSiK qpfmjbUZNrSi0W+t56FlacDxU3vmQ+p3jNwx4kwPEmWufE8/7u3eQCsHj8adkTG4EFtq/87 qaaXcuQV55Bq8QZJq+yiMF0br58xn8UJ/Jtr6PMjbBvqFiPNuxBd1menw9vPA== X-QQ-GoodBg: 2 X-BIZMAIL-ID: 12200627065141232647 From: Jiawen Wu To: dev@dpdk.org Cc: Jiawen Wu , stable@dpdk.org Subject: [PATCH v2 1/2] net/txgbe: add proper memory barriers in Rx Date: Wed, 1 Nov 2023 11:32:40 +0800 Message-Id: <20231101033241.623190-1-jiawenwu@trustnetic.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20231030105144.595502-1-jiawenwu@trustnetic.com> References: <20231030105144.595502-1-jiawenwu@trustnetic.com> MIME-Version: 1.0 X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:trustnetic.com:qybglogicsvrgz:qybglogicsvrgz5a-1 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 Refer to commit 85e46c532bc7 ("net/ixgbe: add proper memory barriers in Rx"). Fix the same issue as ixgbe. Segmentation fault has been observed while running the txgbe_recv_pkts_lro() function to receive packets on the Loongson 3A5000 processor. It's caused by the out-of-order execution of CPU. So add a proper memory barrier to ensure the read ordering be correct. We also did the same thing in the txgbe_recv_pkts() function to make the rxd data be valid even though we did not find segmentation fault in this function. Fixes: 0e484278c85f ("net/txgbe: support Rx") Cc: stable@dpdk.org Signed-off-by: Jiawen Wu --- drivers/net/txgbe/txgbe_rxtx.c | 49 +++++++++++++++------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c index 834ada886a..1cd4b25965 100644 --- a/drivers/net/txgbe/txgbe_rxtx.c +++ b/drivers/net/txgbe/txgbe_rxtx.c @@ -1226,7 +1226,7 @@ txgbe_rx_scan_hw_ring(struct txgbe_rx_queue *rxq) for (j = 0; j < LOOK_AHEAD; j++) s[j] = rte_le_to_cpu_32(rxdp[j].qw1.lo.status); - rte_atomic_thread_fence(__ATOMIC_ACQUIRE); + rte_atomic_thread_fence(rte_memory_order_acquire); /* Compute how many status bits were set */ for (nb_dd = 0; nb_dd < LOOK_AHEAD && @@ -1476,11 +1476,22 @@ txgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, * of accesses cannot be reordered by the compiler. If they were * not volatile, they could be reordered which could lead to * using invalid descriptor fields when read from rxd. + * + * Meanwhile, to prevent the CPU from executing out of order, we + * need to use a proper memory barrier to ensure the memory + * ordering below. */ rxdp = &rx_ring[rx_id]; staterr = rxdp->qw1.lo.status; if (!(staterr & rte_cpu_to_le_32(TXGBE_RXD_STAT_DD))) break; + + /* + * Use acquire fence to ensure that status_error which includes + * DD bit is loaded before loading of other descriptor words. + */ + rte_atomic_thread_fence(rte_memory_order_acquire); + rxd = *rxdp; /* @@ -1726,32 +1737,10 @@ txgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts, next_desc: /* - * The code in this whole file uses the volatile pointer to - * ensure the read ordering of the status and the rest of the - * descriptor fields (on the compiler level only!!!). This is so - * UGLY - why not to just use the compiler barrier instead? DPDK - * even has the rte_compiler_barrier() for that. - * - * But most importantly this is just wrong because this doesn't - * ensure memory ordering in a general case at all. For - * instance, DPDK is supposed to work on Power CPUs where - * compiler barrier may just not be enough! - * - * I tried to write only this function properly to have a - * starting point (as a part of an LRO/RSC series) but the - * compiler cursed at me when I tried to cast away the - * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm - * keeping it the way it is for now. - * - * The code in this file is broken in so many other places and - * will just not work on a big endian CPU anyway therefore the - * lines below will have to be revisited together with the rest - * of the txgbe PMD. - * - * TODO: - * - Get rid of "volatile" and let the compiler do its job. - * - Use the proper memory barrier (rte_rmb()) to ensure the - * memory ordering below. + * "Volatile" only prevents caching of the variable marked + * volatile. Most important, "volatile" cannot prevent the CPU + * from executing out of order. So, it is necessary to use a + * proper memory barrier to ensure the memory ordering below. */ rxdp = &rx_ring[rx_id]; staterr = rte_le_to_cpu_32(rxdp->qw1.lo.status); @@ -1759,6 +1748,12 @@ txgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts, if (!(staterr & TXGBE_RXD_STAT_DD)) break; + /* + * Use acquire fence to ensure that status_error which includes + * DD bit is loaded before loading of other descriptor words. + */ + rte_atomic_thread_fence(rte_memory_order_acquire); + rxd = *rxdp; PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "