From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 402F71B2B3 for ; Thu, 11 Oct 2018 12:24:30 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20181011102430euoutp0228111a2cdc418b64ba571cfe7a10a01e~chuAwM2e72294322943euoutp02P for ; Thu, 11 Oct 2018 10:24:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20181011102430euoutp0228111a2cdc418b64ba571cfe7a10a01e~chuAwM2e72294322943euoutp02P DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1539253470; bh=fYVrHKoQxrl/hFJLx/F4JV6+ZJTlIPTAoiRdtyYYA6Y=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=YTo45cAwb1biPfx4QQbtAuI/Bgl6s5nHCXcosLLiGEu08VDIIbov+sJw7aqRfASoz DEmfujpf1IgFzQYn/ZJ1KU5m5VVBK/kI3IrqSElzA/Cm7VKqnzS3WqeM0GMAVWJI9X Uv/lllvYwmchkLCDKSS+i/itNSS+Cya2eCcrivBY= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20181011102429eucas1p2728c14a8bba9ab8d8ef639117665e2c0~chuAUn0Rz2508425084eucas1p2P; Thu, 11 Oct 2018 10:24:29 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id AE.31.04806.DD42FBB5; Thu, 11 Oct 2018 11:24:29 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20181011102428eucas1p2fe26b12282d2b456ddc2cb96ad7552f0~cht-bhDPv2507925079eucas1p2L; Thu, 11 Oct 2018 10:24:28 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20181011102428eusmtrp12c1ac9a06977f4538922d8fac38ff290~cht-J2uM23273332733eusmtrp1M; Thu, 11 Oct 2018 10:24:28 +0000 (GMT) X-AuditID: cbfec7f5-34dff700000012c6-c4-5bbf24dd2b2d Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id F3.8D.04284.CD42FBB5; Thu, 11 Oct 2018 11:24:28 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20181011102427eusmtip1b7bfbec668c1db28c6ebcacdc0d39a9e~cht_lIThJ2055920559eusmtip14; Thu, 11 Oct 2018 10:24:27 +0000 (GMT) To: "Zhao1, Wei" , "Zhang, Qi Z" , Laurent Hardy Cc: "Lu, Wenzhuo" , "Ananyev, Konstantin" , "stable@dpdk.org" , "dev@dpdk.org" From: Ilya Maximets Date: Thu, 11 Oct 2018 13:26:58 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrDKsWRmVeSWpSXmKPExsWy7djP87p3VfZHG/y/zW/x7tN2Josr7T/Z Ld7/WcRicfnMdBaL6Rv62Sz+dfxht1jz9SKzxdYzfxkdODwu9t9h9Pi1YCmrx+I9L5k8+ras YgxgieKySUnNySxLLdK3S+DKeD/jP2vBkriKpqYpTA2MM/y6GDk5JARMJHY++crUxcjFISSw glHi9IO5zBDOF0aJw9t+s0A4nxklvj6eyw7T8nvNL1aIxHJGiQ33V0NVfWSUmH3qJRtIlbBA qMS7b5OBZnFwiAiUS+ybqwVSwyywjFFi4eGDjCA1bAI6EqdWHwGzWQRUJU4//gi2QVQgQuLI g4VgcV4BQYmTM5+wgNicAiES6w4+ZgWxmQXEJZq+rISy5SW2v50DdreEwDZ2ibsnjrJCNJdJ 9L0COQLkbBeJEw/usELYwhKvjm+BekdG4vTkHhYIu17ifstLRohBHYwS0w/9Y4JI2EtseX2O HeQbZgFNifW79CHCjhKv236CPSkhwCdx460gxD18EpO2TYcK80p0tAlBVKtI/D64HOoaKYmb 7z6zT2BUmoXky1lIPpuF5LNZCHsXMLKsYhRPLS3OTU8tNs5LLdcrTswtLs1L10vOz93ECExD p/8d/7qDcd+fpEOMAhyMSjy8P6T2RQuxJpYVV+YeYpTgYFYS4dWfARTiTUmsrEotyo8vKs1J LT7EKM3BoiTOu2zexmghgfTEktTs1NSC1CKYLBMHp1QDI9+zlMurJPNWprEyGeTLHbOxetrN wfBw/plm7pt3dTvlu3mWnZz7usXt8wIuOwWZYk/7FV//LLUIk53/Uz2++7Z76K7K/LRlrGdr V9rPK1iqtOTD+hS1I5PLVxioZvLf+3DLK3M+x/E8Ga/Wo/r/UyqWnXA0uvL+AV9SSaJIe0jT 6cA5G89+VGIpzkg01GIuKk4EAA0TPgs/AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrPIsWRmVeSWpSXmKPExsVy+t/xu7p3VPZHGyzYKmTx7tN2Josr7T/Z Ld7/WcRicfnMdBaL6Rv62Sz+dfxht1jz9SKzxdYzfxkdODwu9t9h9Pi1YCmrx+I9L5k8+ras YgxgidKzKcovLUlVyMgvLrFVija0MNIztLTQMzKx1DM0No+1MjJV0rezSUnNySxLLdK3S9DL eD/jP2vBkriKpqYpTA2MM/y6GDk5JARMJH6v+cXaxcjFISSwlFHi8Z7DrBAJKYkfvy5A2cIS f651sUEUvWeUOP4HIiEsECrx7ttk5i5GDg4RgXKJdy+VQGqYBZYxStyY0Ac1tYlVYu/h10wg DWwCOhKnVh9hBLF5Bewkdh78xAxiswioSpx+/JEdxBYViJBYvfwFK0SNoMTJmU9YQGxOgRCJ dQcfg8WZBdQl/sy7xAxhi0s0fVkJFZeX2P52DvMERqFZSNpnIWmZhaRlFpKWBYwsqxhFUkuL c9Nziw31ihNzi0vz0vWS83M3MQIjb9uxn5t3MF7aGHyIUYCDUYmH94fUvmgh1sSy4srcQ4wS HMxKIrz6M4BCvCmJlVWpRfnxRaU5qcWHGE2BnpvILCWanA9MCnkl8YamhuYWlobmxubGZhZK 4rznDSqjhATSE0tSs1NTC1KLYPqYODilGhizO0q3z38z7bVy4uQ6x2UW7vN//89IuWZpd2u2 U/aqaOP/YZVyZxWF+K6EmwV2LpKXfXCQu9XLQ7N04l67qGNM7OnP7t548W3Tz+UXlUMnn0g2 6n5T9Sfw4jvmxGZz/qPpwpGrX76duj1s9XOPDUrqarX21Uwnp219Xb3pd9OJatfY8PmvK94r sRRnJBpqMRcVJwIAmz3XltICAAA= Message-Id: <20181011102428eucas1p2fe26b12282d2b456ddc2cb96ad7552f0~cht-bhDPv2507925079eucas1p2L@eucas1p2.samsung.com> X-CMS-MailID: 20181011102428eucas1p2fe26b12282d2b456ddc2cb96ad7552f0 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180831123824eucas1p1cd2981c716c4764703e24c3eeb4d33c7 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180831123824eucas1p1cd2981c716c4764703e24c3eeb4d33c7 References: <20180831123824eucas1p1cd2981c716c4764703e24c3eeb4d33c7~P_GOOSRuf0867908679eucas1p1K@eucas1p1.samsung.com> <039ED4275CED7440929022BC67E706115327FA5E@SHSMSX103.ccr.corp.intel.com> <20180910150708eucas1p220c16857c4277b311ddc000b9337d9cb~TEk8KngQJ1365413654eucas1p2a@eucas1p2.samsung.com> <039ED4275CED7440929022BC67E7061153284304@SHSMSX103.ccr.corp.intel.com> <20180912080338eucas1p1bfdacb30aa969cd607ccf99f64d6bf80~TmFveK2Dy2157121571eucas1p1U@eucas1p1.samsung.com> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Oct 2018 10:24:31 -0000 On 11.10.2018 12:56, Zhao1, Wei wrote: > Hi, Ilya Maximets AND laurent.hardy Hi, thanks for sharing your thoughts. Comments inline. > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets >> Sent: Wednesday, September 12, 2018 4:05 PM >> To: Zhang, Qi Z ; dev@dpdk.org >> Cc: Lu, Wenzhuo ; Ananyev, Konstantin >> ; Laurent Hardy >> ; Dai, Wei ; >> stable@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link >> update >> >> On 12.09.2018 09:49, Zhang, Qi Z wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets [mailto:i.maximets@samsung.com] >>>> Sent: Monday, September 10, 2018 11:09 PM >>>> To: Zhang, Qi Z ; dev@dpdk.org >>>> Cc: Lu, Wenzhuo ; Ananyev, Konstantin >>>> ; Laurent Hardy >>>> ; Dai, Wei ; >>>> stable@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while >>>> fiber link update >>>> >>>> On 04.09.2018 09:08, Zhang, Qi Z wrote: >>>>> Hi Ilya: >>>>> >>>>>> -----Original Message----- >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets >>>>>> Sent: Friday, August 31, 2018 8:40 PM >>>>>> To: dev@dpdk.org >>>>>> Cc: Lu, Wenzhuo ; Ananyev, Konstantin >>>>>> ; Laurent Hardy >>>>>> ; Dai, Wei ; Ilya >>>>>> Maximets ; stable@dpdk.org >>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber >>>>>> link update >>>>>> >>>>>> If the multispeed fiber link is in DOWN state, ixgbe_setup_link >>>>>> could take around a second of busy polling. This is highly >>>>>> inconvenient for the case where single thread periodically checks the >> link statuses. >>>>>> For example, OVS main thread periodically updates the link statuses >>>>>> and hangs for a really long time busy waiting on ixgbe_setup_link() >>>>>> for a DOWN fiber ports. For case with 3 down ports it hangs for a 3 >>>>>> seconds and unable to do anything including packet processing. >>>>>> Fix that by shifting that workaround to a separate thread by alarm >>>>>> handler that will try to set up link if it is DOWN. >>>>> >>>>> Does that mean we will block the interrupt thread for 3 seconds? >>>> >>>> Three times for one second. Other work could be scheduled between. >>>> IMHO, it's much better than blocking usual caller for 3 seconds. >>>> >>>>> Also, can we guarantee there will not be any race condition if we >>>>> call >>>> ixgbe_setup_link at another thread, the base code API is not assumed >>>> to be thread-safe as I know. >>>> >>>> The only user of 'ixgbe_setup_link' is 'ixgbe_dev_start', but it >>>> could be called only if device stopped. 'ixgbe_dev_stop' cancels the alarm. >>>> Race with 'link_update' avoided by 'IXGBE_FLAG_NEED_LINK_CONFIG' >> flag. >>> >>> I guess, it' not only about when ixgb_setup_link race with itself, but also >> when it race with other APIs. >>> Also the concern is, even in current version, we can prove there is no issue, >> how can we guarantee we are safe for future base code update? It's not >> designed as thread-safe. >>> For my option, the change is risky. >> >> In current implementation interrupt handler already calls the >> 'ixgbe_dev_link_update' which subsequently calls 'ixgbe_setup_link' >> in our case if LSC interrupts enabled. So, my change makes the driver even >> safer by moving 'ixgbe_setup_link' to the same interrupt thread. >> Otherwise two threads (interrupts handler and the link status checking >> thread) could call 'ixgbe_setup_link' simultaneously. >> >>> >>> Btw, since ixgbe support LSC, it is not necessary for "single thread >> periodically checks the link statuses", right? >> >> In current implementation it will take at least 5 seconds (4 + 1) for the >> interrupt handler to detect DOWN link state for ixgbe multispeed fiber. This >> is too much for many real world cases. > > I have reviewed this patch, now I agree with you of the point that when port is down, " main thread > periodically updates the link statuses and hangs for a really long time busy waiting on ixgbe_setup_link() for a DOWN fiber ports ". > This is introduced by a patch in the following: > SHA-1: c12d22f65b132c56db7b4fdbfd5ddce27d1e9572 > * net/ixgbe: ensure link status is updated > > Because in this patch, ixgbe_setup_link() is called with input parameter autoneg_wait_to_complete=1, this will cause loop check and sleep delay. > At least 82599 seems has this delay.(BTW, whivh type of NIC are you use? X550 or 82599) I have 82599. > Your solution is add a eal_alarm_set for ixgbe_setup_link in the thread of PMD driver, and do the set up work in that thread, is that right? > And main thread avoid hang by the flag of IXGBE_FLAG_NEED_LINK_CONFIG. > I think this is a good idea for this problem, but it may cause problem for other legacy user of ixgbe pmd, because their legacy code, > which use main thread to check link state and setup_link when port is down, and they are not aware of it is done by other thread if add your patch. What are these applications? I see no public API for setup_link function. It's internal to driver and should not be used externally. Am I missing something? > > And is that ok if we change code in ixgbe_dev_link_update_share() to > > ixgbe_dev_link_update_share() > { > > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > wait = 0; > > if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) { > speed = hw->phy.autoneg_advertised; > if (!speed) > ixgbe_get_link_capabilities(hw, &speed, &autoneg); > ixgbe_setup_link(hw, speed, wait); > } > } > > Then, your application can call rte_eth_link_get_nowait () to make wait_to_complete=0 when doing periodic link state check, > Which will not cause loop check and sleep delay. Legacy code of other user call rte_eth_link_get() will not be affected also. > But, I am NOT confident ,whether this will introduce new problem when set up link without wait! > So, this is just a discussion topic. Unfortunately this will not help. Take a look to the function 'ixgbe_setup_mac_link_multispeed_fiber()', which is the main problematic function here. 'wait_to_complete' here used only as argument for ixgbe_setup_mac_link(), and the busy waiting loops are outside of it. Regardless of the 'wait_to_complete' value, this function will busy poll the link for 1040 ms trying to setup 10GB speed and 140ms more trying to setup 1GB speed. After that, it will call itself recursively and wait again... Looks like I miscalculated last time. Right now it'll be more than 2 seconds for each down port since following patch merged: 8fc1f32fa615 ("net/ixgbe: wait longer for link after fiber MAC setup"). > > Hi, laurent.hardy > You are the author for the patch (* net/ixgbe: ensure link status is updated), why do you implement code that way? > Is that must that set up link with wait? > > ixgbe_setup_link(hw, speed, true); > > > >> >>> >>>> >>>>> >>>>> Regards >>>>> Qi >>>>> >>>>>> >>>>>> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated") >>>>>> CC: stable@dpdk.org >>>>>> >>>>>> Signed-off-by: Ilya Maximets >>>>>> --- >>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 43 >>>>>> ++++++++++++++++++++++++-------- >>>>>> 1 file changed, 32 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> index 26b192737..a33b9a6e8 100644 >>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> @@ -221,6 +221,8 @@ static int ixgbe_dev_interrupt_action(struct >>>>>> rte_eth_dev *dev, >>>>>> struct rte_intr_handle *handle); static >> void >>>>>> ixgbe_dev_interrupt_handler(void *param); static void >>>>>> ixgbe_dev_interrupt_delayed_handler(void *param); >>>>>> +static void ixgbe_dev_setup_link_alarm_handler(void *param); >>>>>> + >>>>>> static int ixgbe_add_rar(struct rte_eth_dev *dev, struct >>>>>> ether_addr *mac_addr, >>>>>> uint32_t index, uint32_t pool); static void >>>>>> ixgbe_remove_rar(struct rte_eth_dev *dev, uint32_t index); @@ >>>>>> -2791,6 +2793,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev) >>>>>> >>>>>> PMD_INIT_FUNC_TRACE(); >>>>>> >>>>>> + rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); >>>>>> + >>>>>> /* disable interrupts */ >>>>>> ixgbe_disable_intr(hw); >>>>>> >>>>>> @@ -3969,6 +3973,25 @@ ixgbevf_check_link(struct ixgbe_hw *hw, >>>>>> ixgbe_link_speed *speed, >>>>>> return ret_val; >>>>>> } >>>>>> >>>>>> +static void >>>>>> +ixgbe_dev_setup_link_alarm_handler(void *param) { >>>>>> + struct rte_eth_dev *dev = (struct rte_eth_dev *)param; >>>>>> + struct ixgbe_hw *hw = >>>>>> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>>>>> + struct ixgbe_interrupt *intr = >>>>>> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>>>>> + u32 speed; >>>>>> + bool autoneg = false; >>>>>> + >>>>>> + speed = hw->phy.autoneg_advertised; >>>>>> + if (!speed) >>>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>>> + >>>>>> + ixgbe_setup_link(hw, speed, true); >>>>>> + >>>>>> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; } >>>>>> + >>>>>> /* return 0 means link status changed, -1 means not changed */ >>>>>> int ixgbe_dev_link_update_share(struct rte_eth_dev *dev, @@ - >> 3981,9 >>>>>> +4004,7 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, >>>>>> IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>>>>> int link_up; >>>>>> int diag; >>>>>> - u32 speed = 0; >>>>>> int wait = 1; >>>>>> - bool autoneg = false; >>>>>> >>>>>> memset(&link, 0, sizeof(link)); >>>>>> link.link_status = ETH_LINK_DOWN; @@ -3993,13 +4014,8 @@ >>>>>> ixgbe_dev_link_update_share(struct >>>> rte_eth_dev >>>>>> *dev, >>>>>> >>>>>> hw->mac.get_link_status = true; >>>>>> >>>>>> - if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>>> - ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) { >>>>>> - speed = hw->phy.autoneg_advertised; >>>>>> - if (!speed) >>>>>> - ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>>> - ixgbe_setup_link(hw, speed, true); >>>>>> - } >>>>>> + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) >>>>>> + return rte_eth_linkstatus_set(dev, &link); >>>>>> >>>>>> /* check if it needs to wait to complete, if lsc interrupt is enabled */ >>>>>> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != >>>>>> 0) @@ >>>>>> -4017,11 +4033,14 @@ ixgbe_dev_link_update_share(struct >> rte_eth_dev >>>> *dev, >>>>>> } >>>>>> >>>>>> if (link_up == 0) { >>>>>> - intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>> + if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) >> { >>>>>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>> + rte_eal_alarm_set(10, >>>>>> + ixgbe_dev_setup_link_alarm_handler, dev); >>>>>> + } >>>>>> return rte_eth_linkstatus_set(dev, &link); >>>>>> } >>>>>> >>>>>> - intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>> link.link_status = ETH_LINK_UP; >>>>>> link.link_duplex = ETH_LINK_FULL_DUPLEX; >>>>>> >>>>>> @@ -5128,6 +5147,8 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev) >>>>>> >>>>>> PMD_INIT_FUNC_TRACE(); >>>>>> >>>>>> + rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); >>>>>> + >>>>>> ixgbevf_intr_disable(dev); >>>>>> >>>>>> hw->adapter_stopped = 1; >>>>>> -- >>>>>> 2.17.1 >>>>>