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 18FBB42A31; Mon, 1 May 2023 15:29:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A114D40EE3; Mon, 1 May 2023 15:29:24 +0200 (CEST) Received: from forward500b.mail.yandex.net (forward500b.mail.yandex.net [178.154.239.144]) by mails.dpdk.org (Postfix) with ESMTP id 0ED024021E for ; Mon, 1 May 2023 15:29:23 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-36.iva.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-36.iva.yp-c.yandex.net [IPv6:2a02:6b8:c0c:a716:0:640:8b36:0]) by forward500b.mail.yandex.net (Yandex) with ESMTP id 721285E837; Mon, 1 May 2023 16:29:22 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-36.iva.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id JTMkWleDUuQ0-K5LhdPCt; Mon, 01 May 2023 16:29:21 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1682947761; bh=SXI3RM0OLoneIYC2ezwOzpxQ501TRWXCxfR2UiBYe8M=; h=From:Subject:In-Reply-To:Cc:Date:References:To:Message-ID; b=f8/VVE7jIFHIGNNQxe+WmVIv3znf3xUGG0pQXEasre6bNnohG4eUZjXed0B7Xs/dw 6zgkDVMumhjDwtLupDMvm6UXxHdd0DQ/A8mKc3KaqMqBzaXN8YVy/8l/cXhpG1OwBb yDRzES1viOMPZ81wTPGlbxfMUwSQRAALSscp8Ckc= Authentication-Results: mail-nwsmtp-smtp-production-main-36.iva.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <20671e5e-8e86-4bc3-2d95-4cec014b0539@yandex.ru> Date: Mon, 1 May 2023 14:29:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 To: zhoumin@loongson.cn Cc: dev@dpdk.org, maobibo@loongson.cn, qiming.yang@intel.com, wenjun1.wu@intel.com, "ruifeng.wang@arm.com >> Ruifeng Wang" , drc@linux.vnet.ibm.com References: <20230424090532.367194-1-zhoumin@loongson.cn> Subject: Re: [PATCH v2] net/ixgbe: add proper memory barriers for some Rx functions Content-Language: en-US From: Konstantin Ananyev In-Reply-To: <20230424090532.367194-1-zhoumin@loongson.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > 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 > --- > 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(); > rxd = *rxdp; Indeed, looks like a problem to me on systems with relaxed MO. Strange that it was never hit on arm or ppc - cc-ing ARM/PPC maintainers. About a fix - looks right, but a bit excessive to me - as I understand all we need here is to prevent re-ordering by CPU itself. So rte_smp_rmb() seems enough here. Or might be just: staterr = __atomic_load_n(&rxdp->wb.upper.status_error, __ATOMIC_ACQUIRE); > /* > @@ -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