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 7458A41D34; Wed, 22 Feb 2023 02:07:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0867040697; Wed, 22 Feb 2023 02:07:09 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 1018740693; Wed, 22 Feb 2023 02:07:05 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4PLyc037MZznWhF; Wed, 22 Feb 2023 09:04:32 +0800 (CST) Received: from [10.67.100.224] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.17; Wed, 22 Feb 2023 09:07:02 +0800 Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup To: Stephen Hemminger , Ruifeng Wang CC: Ashok Kaladi , "jerinj@marvell.com" , "thomas@monjalon.net" , Honnappa Nagarahalli , "dev@dpdk.org" , "s.v.naga.harish.k@intel.com" , "erik.g.carrillo@intel.com" , "abhinandan.gujjar@intel.com" , "stable@dpdk.org" , nd References: <20230220060839.1267349-1-ashok.k.kaladi@intel.com> <20230220060839.1267349-2-ashok.k.kaladi@intel.com> <4786db4b-63dc-5329-522d-77eb58d4cff4@huawei.com> <20230221090053.14d653bf@hermes.local> From: fengchengwen Message-ID: Date: Wed, 22 Feb 2023 09:07:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20230221090053.14d653bf@hermes.local> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected 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 On 2023/2/22 1:00, Stephen Hemminger wrote: > On Tue, 21 Feb 2023 07:24:19 +0000 > Ruifeng Wang wrote: > >>> -----Original Message----- >>> From: fengchengwen >>> Sent: Monday, February 20, 2023 2:58 PM >>> To: Ashok Kaladi ; jerinj@marvell.com; thomas@monjalon.net >>> Cc: dev@dpdk.org; s.v.naga.harish.k@intel.com; erik.g.carrillo@intel.com; >>> abhinandan.gujjar@intel.com; stable@dpdk.org; Ruifeng Wang >>> Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup >>> >>> On 2023/2/20 14:08, Ashok Kaladi wrote: >>>> If ethdev enqueue or dequeue function is called during >>>> eth_dev_fp_ops_setup(), it may get pre-empted after setting the >>>> function pointers, but before setting the pointer to port data. >>>> In this case the newly registered enqueue/dequeue function will use >>>> dummy port data and end up in seg fault. >>>> >>>> This patch moves the updation of each data pointers before updating >>>> corresponding function pointers. >>>> >>>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate >>>> structure") >>>> Cc: stable@dpdk.org > > Why is something calling enqueue/dequeue when device is not fully started. > A correctly written application would not call rx/tx burst until after > ethdev start had finished. Please refer the eb0d471a894 (ethdev: add proactive error handling mode), when driver recover itself, the application may still invoke enqueue/dequeue API. > > Would something like this work better? > > Note: there is another bug in current code. The check for link state interrupt > and link_ops could return -ENOTSUP and leave device in indeterminate state. > The check should be done before calling PMD. > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 0266cc82acb6..d6c163ed85e7 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -1582,6 +1582,14 @@ rte_eth_dev_start(uint16_t port_id) > return 0; > } > > + if (dev->data->dev_conf.intr_conf.lsc == 0 && > + dev->dev_ops->link_update == NULL) { > + RTE_ETHDEV_LOG(INFO, > + "Device with port_id=%"PRIu16" link update not supported\n", > + port_id); > + return -ENOTSUP; > + } > + > ret = rte_eth_dev_info_get(port_id, &dev_info); > if (ret != 0) > return ret; > @@ -1591,9 +1599,7 @@ rte_eth_dev_start(uint16_t port_id) > eth_dev_mac_restore(dev, &dev_info); > > diag = (*dev->dev_ops->dev_start)(dev); > - if (diag == 0) > - dev->data->dev_started = 1; > - else > + if (diag != 0) > return eth_err(port_id, diag); > > ret = eth_dev_config_restore(dev, &dev_info, port_id); > @@ -1611,16 +1617,18 @@ rte_eth_dev_start(uint16_t port_id) > return ret; > } > > - if (dev->data->dev_conf.intr_conf.lsc == 0) { > - if (*dev->dev_ops->link_update == NULL) > - return -ENOTSUP; > - (*dev->dev_ops->link_update)(dev, 0); > - } > - > /* expose selection of PMD fast-path functions */ > eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev); > > + /* ensure state is set before marking device ready */ > + rte_smp_wmb(); > + > rte_ethdev_trace_start(port_id); > + > + /* Update current link state */ > + if (dev->data->dev_conf.intr_conf.lsc == 0) > + (*dev->dev_ops->link_update)(dev, 0); > + > return 0; > } > > > . >