From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilya Maximets Subject: Re: [PATCH] net/ixgbe: fix busy polling while fiber link update Date: Mon, 10 Sep 2018 18:08:49 +0300 Message-ID: <20180910150708eucas1p220c16857c4277b311ddc000b9337d9cb~TEk8KngQJ1365413654eucas1p2a@eucas1p2.samsung.com> References: <20180831123824eucas1p1cd2981c716c4764703e24c3eeb4d33c7~P_GOOSRuf0867908679eucas1p1K@eucas1p1.samsung.com> <039ED4275CED7440929022BC67E706115327FA5E@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Cc: "Lu, Wenzhuo" , "Ananyev, Konstantin" , Laurent Hardy , "Dai, Wei" , "stable@dpdk.org" To: "Zhang, Qi Z" , "dev@dpdk.org" Return-path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 2458E4C90 for ; Mon, 10 Sep 2018 17:07:11 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20180910150710euoutp01abe55715b3c45eced51ebced2e28fdee~TEk9kqiKq1060910609euoutp01q for ; Mon, 10 Sep 2018 15:07:10 +0000 (GMT) In-Reply-To: <039ED4275CED7440929022BC67E706115327FA5E@SHSMSX103.ccr.corp.intel.com> Content-Language: en-GB List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. > > 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 >