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 7141D41D84; Sun, 26 Feb 2023 18:22:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0739F4021D; Sun, 26 Feb 2023 18:22:17 +0100 (CET) Received: from forward500a.mail.yandex.net (forward500a.mail.yandex.net [178.154.239.80]) by mails.dpdk.org (Postfix) with ESMTP id 679FA40156 for ; Sun, 26 Feb 2023 18:22:15 +0100 (CET) Received: from vla5-f17bbf3532fc.qloud-c.yandex.net (vla5-f17bbf3532fc.qloud-c.yandex.net [IPv6:2a02:6b8:c18:3588:0:640:f17b:bf35]) by forward500a.mail.yandex.net (Yandex) with ESMTP id CFDC95E85F; Sun, 26 Feb 2023 20:22:14 +0300 (MSK) Received: by vla5-f17bbf3532fc.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id BMW7pWZbYa61-rRLTNLMa; Sun, 26 Feb 2023 20:22:13 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1677432134; bh=91t3xmYHzgccqCOerKR/yVRJfjRr72jYJvYnyWoHkWk=; h=In-Reply-To:Cc:From:Date:References:To:Subject:Message-ID; b=CwKrKEk65cwoWodmSMD3LbWTVQPz4ahwd8k5qdykOCF9lMpXp8+Y2cSCMYnYC1TNu 0Ssn1729g7Iy5aoG2RFyV4X1vHlpncM9DE7xa6VHM6vhZV7czfYlI5B860z5QpU/Vk 0eWfdPaTPcu0c8IlW9wgknllTtP1xutMIYdiw+hU= Authentication-Results: vla5-f17bbf3532fc.qloud-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <3dc3b3d3-c80f-a361-8780-b1b3e48d843e@yandex.ru> Date: Sun, 26 Feb 2023 17:22:11 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup Content-Language: en-US To: dev@dpdk.org, fengchengwen , 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> From: Konstantin Ananyev Cc: fengchengwen , Konstantin Ananyev , Honnappa Nagarahalli , Stephen Hemminger , Ruifeng Wang , "Ajit Khaparde (ajit.khaparde@broadcom.com)" In-Reply-To: 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 >>>>>>>>>> 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. >> >> 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; >>>>>>> } >>>>>>> >>>>>>> >>>>>>> . >>>>>>> >>>>