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 1FAF841DEE; Mon, 6 Mar 2023 02:57:13 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C171D40ED6; Mon, 6 Mar 2023 02:57:12 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id E736A410F6 for ; Mon, 6 Mar 2023 02:57:10 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.55]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4PVM7g2g6zzSkPW; Mon, 6 Mar 2023 09:54:07 +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.21; Mon, 6 Mar 2023 09:57:08 +0800 Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup To: Ferruh Yigit , Konstantin Ananyev , , Konstantin Ananyev , Honnappa Nagarahalli , Stephen Hemminger , Ruifeng Wang , "Ajit Khaparde (ajit.khaparde@broadcom.com)" 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> <3cd97a71-b32f-b33b-dce1-46fabad182f6@huawei.com> <54fbf4e55cd44477b1e956f98a7a3c50@huawei.com> <3dc3b3d3-c80f-a361-8780-b1b3e48d843e@yandex.ru> <52296fe2-d9f6-1a24-e577-e5271a69a053@amd.com> From: fengchengwen Message-ID: <3829a532-63b3-5d49-77d0-d749e44ca902@huawei.com> Date: Mon, 6 Mar 2023 09:57:08 +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: <52296fe2-d9f6-1a24-e577-e5271a69a053@amd.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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/3/4 1:19, Ferruh Yigit wrote: > On 2/26/2023 5:22 PM, Konstantin Ananyev 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. >>>>>>> >>>>>>> Right now DPDK ethdev layer *does not* provide synchronization >>>>>>> mechanisms between data-path and control-path functions. >>>>>>> That was a deliberate deisgn choice. If we want to change that >>>>>>> rule, then I >>>>>>> suppose we need a community consensus for it. >>>>>>> I think that if the driver wants to provide some sort of error >>>>>>> recovery >>>>>>> procedure, then it has to provide some synchronization mechanism >>>>>>> inside it >>>>>>> between data-path and control-path functions. >>>>>>> Actually looking at eb0d471a894 (ethdev: add proactive error handling >>>>>>> mode), and following patches I wonder how it creeped in? >>>>>>> It seems we just introduced a loophole for race condition with this >>>>>>> approach... >>>>> >>>>> Could you try to describe the specific scenario of loophole ? >>>> >>>> Ok, as I understand the existing mechanism: >>>> >>>> When PMD wants to start a recovery it has to: >>>>   - invoke  rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING); >>>>     That supposed to call user provided callback. After callback is >>>> finished PMD assumes >>>>     that user is aware that recovery is about to start and should >>>> make some precautions. >>>> - when recovery is finished it invokes another callback: >>>>    RTE_ETH_EVENT_RECOVERY_(SUCCESS/FAILED). After that user either >>>> can continue to >>>>    use port or have to treat is as faulty. >>>> >>>> The idea is ok in principle, but there is a problem. >>>> >>>> lib/ethdev/rte_ethdev.h: >>>>             /** Port recovering from a hardware or firmware error. >>>>           * If PMD supports proactive error recovery, >>>>           * it should trigger this event to notify application >>>>           * that it detected an error and the recovery is being started. >>>> >>>> <<< !!!!! >>>>           * Upon receiving the event, the application should not >>>> invoke any control path API >>>>           * (such as rte_eth_dev_configure/rte_eth_dev_stop...) until >>>> receiving >>>>           * RTE_ETH_EVENT_RECOVERY_SUCCESS or >>>> RTE_ETH_EVENT_RECOVERY_FAILED event. >>>>           * The PMD will set the data path pointers to dummy functions, >>>>           * and re-set the data path pointers to non-dummy functions >>>>           * before reporting RTE_ETH_EVENT_RECOVERY_SUCCESS event. >>>> <<< !!!!! >>>> >>>> That part is just wrong I believe. >>>> It should be: >>>> Upon receiving the event, the application should not invoke any *both >>>> control and data-path* API >>>> until receiving  RTE_ETH_EVENT_RECOVERY_SUCCESS or >>>> RTE_ETH_EVENT_RECOVERY_FAILED event. >>>> Resetting data path pointers to dummy functions by PMD *before* invoking >>>> rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING); >>>> introduces a race-condition with data-path threads, as such thread >>>> could already be inside RX/TX function >>>> or can already read RX/TX function/data pointers and be about to use >>>> them. >>> >>> Current practices: the PMDs already add some delay after set Rx/Tx >>> callback to dummy, and plus the DPDK >>> worker thread is busypolling, the probability of occurence in reality >>> is zero. But in theoretically exist >>> the above race-condition. >> >> >> Adding delay might make a problem a bit less reproducible, >> but it doesn't fix it. >> The bug is still there. >> >> >>> >>>> And right now rte_ethdev layer doesn't provide any mechanism to check >>>> it or wait when they'll finish, etc. >>> >>> Yes >>> >>>> >>>> So, probably the simplest way to fix it with existing DPDK design: >>>> - user level callback  RTE_ETH_EVENT_ERR_RECOVERING should return >>>> only after it ensures that *all* >>>>    application threads (and processes) stopped using either control >>>> or data-path functions for that port >>> >>> Agree >>> >>>>    (yes it means that application that wants to use this feature has >>>> to provide its own synchronization mechanism >>>>    around data-path functions (RX/TX) that it is going to use). >>>> - after that PMD is safe to reset rte_eth_fp_ops[] values to dummy ones. >>>> >>>> And message to all PMD developers: >>>> *please stop updating rte_eth_fp_ops[] on your own*. >>>> That's a bad practice and it is not supposed to do things that way. >>>> There is a special API provided for these purposes: >>>> eth_dev_fp_ops_reset(), eth_dev_fp_ops_setup(), so use it. >>> >>> This two function is in private.h, so it should be expose to public >>> header file. >> >> You mean we need to move these functions declarations into ethdev_driver.h? >> If so, then yes, I think we probably do. >> >> > > > What about making slightly different version available to drivers, which > only updates function pointers, but not 'fpo->rxq' / 'fpo->txq'. > > This way driver can switch to between dummy and real burst function > without worrying Rx/Tx queue validity. > > @Chengwen, @Ruifeng, can this solve the issue for relaxed memory > ordering systems? For the problem described in this commit, I think it's OK for solve the RMO. > > > >>>> >>>> BTW,  I don't see any implementation for RTE_ETH_EVENT_ERR_RECOVERING >>>> within >>>> either testpmd or any other example apps. >>>> Am I missing something? >>> >>> Currently it just promote the event. >> >> >> Ok, can I suggest then to add a proper usage for into in testpmd? >> It looks really strange that we add new feature into ethdev (and 2 PMDs), >> but didn't provide any way for users to test it. >> >>> >>>> If not, then probably it could be a good starting point - let's >>>> incorporate it inside testpmd >>>> (new forwarding engine probably) so everyone can test/try it. >>>> >>>>           * It means that the application cannot send or receive any >>>> packets >>>>           * during this period. >>>>           * @note Before the PMD reports the recovery result, >>>>           * the PMD may report the RTE_ETH_EVENT_ERR_RECOVERING event >>>> again, >>>>           * because a larger error may occur during the recovery. >>>>           */ >>>>          RTE_ETH_EVENT_ERR_RECOVERING, >>>> >>>>>>> It probably needs to be either deprecated or reworked. >>>>>> Looking at the commit, it does not say anything about the data >>>>>> plane functions which probably means, the error recovery is >>>>> happening within the data plane thread. What happens to other data >>>>> plane threads that are polling the same port on which the error >>>>> recovery is happening? >>>>> >>>>> The commit log says: "the PMD sets the data path pointers to dummy >>>>> functions". >>>>> >>>>> So the data plane threads will receive non-packet and send zero with >>>>> port which in error recovery. >>>>> >>>>>> >>>>>> Also, the commit log says that while the error recovery is under >>>>>> progress, the application should not call any control plane APIs. Does >>>>> that mean, the application has to check for error condition every >>>>> time it calls a control plane API? >>>>> >>>>> If application has not register event (RTE_ETH_EVENT_ERR_RECOVERING) >>>>> callback, it could calls control plane API, but it will return >>>>> failed. >>>>> If application has register above callback, it can wait for recovery >>>>> result, or direct call without wait but this will return failed. >>>>> >>>>>> >>>>>> The commit message also says that "PMD makes sure the control path >>>>>> operations failed with retcode -EBUSY". It does not say how it >>>>> does this. But, any communication from the PMD thread to control >>>>> plane thread may introduce race conditions if not done correctly. >>>>> >>>>> First there are no PMD thread, do you mean eal-intr-thread ? >>>>> >>>>> As for this question, you can see PMDs which already implement it, >>>>> they both provides mutual exclusion protection. >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> 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; >>>>>>>>>   } >>>>>>>>> >>>>>>>>> >>>>>>>>> . >>>>>>>>> >>>>>> >> > > . >