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 8B23142A5D; Thu, 4 May 2023 15:16:44 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1894E41144; Thu, 4 May 2023 15:16:44 +0200 (CEST) Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by mails.dpdk.org (Postfix) with ESMTP id 4B31A410DC for ; Thu, 4 May 2023 15:16:41 +0200 (CEST) Received: from loongson.cn (unknown [10.20.42.90]) by gateway (Coremail) with SMTP id _____8AxFuk3sFNktqYEAA--.7596S3; Thu, 04 May 2023 21:16:39 +0800 (CST) Received: from [10.20.42.90] (unknown [10.20.42.90]) by localhost.localdomain (Coremail) with SMTP id AQAAf8BxFLE3sFNkXPtJAA--.5827S3; Thu, 04 May 2023 21:16:39 +0800 (CST) Subject: Re: [PATCH v2] net/ixgbe: add proper memory barriers for some Rx functions To: Konstantin Ananyev Cc: dev@dpdk.org, maobibo@loongson.cn, qiming.yang@intel.com, wenjun1.wu@intel.com, ruifeng.wang@arm.com, drc@linux.vnet.ibm.com References: <20230424090532.367194-1-zhoumin@loongson.cn> <20671e5e-8e86-4bc3-2d95-4cec014b0539@yandex.ru> From: zhoumin Message-ID: <43fb4e62-63b5-7076-327e-dc30cd561fff@loongson.cn> Date: Thu, 4 May 2023 21:16:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20671e5e-8e86-4bc3-2d95-4cec014b0539@yandex.ru> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID: AQAAf8BxFLE3sFNkXPtJAA--.5827S3 X-CM-SenderInfo: 52kr3ztlq6z05rqj20fqof0/ X-Coremail-Antispam: 1Uk129KBjvJXoWxWF48Kr4UKr4kJF4DCry8Zrb_yoWrWFW5pF 4kJFn0kry5Gr4kGw1Iqw18Gr9xAr4rJ3WUXr95A3WkKr4DXw1YqF4jqr1jgF1DJr48Jw1j qr1UX39xuF4DAFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUj1kv1TuYvTs0mT0YCTnIWj qI5I8CrVACY4xI64kE6c02F40Ex7xfYxn0WfASr-VFAUDa7-sFnT9fnUUIcSsGvfJTRUUU b3kYFVCjjxCrM7AC8VAFwI0_Jr0_Gr1l1xkIjI8I6I8E6xAIw20EY4v20xvaj40_Wr0E3s 1l1IIY67AEw4v_Jrv_JF1l8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxSw2x7M28EF7xv wVC0I7IYx2IY67AKxVW5JVW7JwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxVW8JVWxJwA2z4 x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwVC2z280aVCY1x0267AKxVW0oVCq3wAS0I0E 0xvYzxvE52x082IY62kv0487Mc804VCY07AIYIkI8VC2zVCFFI0UMc02F40EFcxC0VAKzV Aqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S 6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxk0xIA0c2IEe2xFo4CEbIxvr21l42xK82 IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1l4IxYO2xFxVAFwI0_JF0_Jw1lx2Iq xVAqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r 126r1DMIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY 6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67 AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x 07jUsqXUUUUU= 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 Hi Konstantin, Thanks for your  comments. On 2023/5/1 下午9:29, Konstantin Ananyev wrote: >> 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. The LoongArch architecture uses the Weak Consistency model which can cause the problem, especially in scenario with many cores, such as Loongson 3C5000 with four NUMA node, which has 64 cores. I cannot reproduce it on Loongson 3C5000 with one NUMA node, which just has 16 cores. > 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. Yes, thanks for cc-ing. > So rte_smp_rmb() seems enough here. > Or might be just: > staterr = __atomic_load_n(&rxdp->wb.upper.status_error, > __ATOMIC_ACQUIRE); > Does __atomic_load_n() work on Windows if we use it to solve this problem ? > >>          /* >> @@ -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