From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id E5C721B4B1 for ; Thu, 11 Oct 2018 14:22:02 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id e4-v6so9471514wrs.0 for ; Thu, 11 Oct 2018 05:22:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=bPXhz94e+m5bSQ0bpYIibfQK+xo0AYYY1+cyBN+IrO4=; b=diGOnMhgt+dOoyhf4TDeMEG5f+FEPbhGQkW07ke56iuVnDrSEFAZbUvGb8Seh2u+a8 EyFL2QzQXx/gYbfOntMeQKvZ/R2HW2DwXK50PMxKzK9Gjf5Bhpnfv00FzxDXgUgQj3ob 5NQxOpajBDebikA9UL8b/35F14QZP3n7Qx3arIir4JfIWw2Gy1QajIGLn+Xk8xusPiqb t9VIMd6Y6v75UzvPZkj3PL+/ohU4GtjImVlvBsomEiTZy0F19aVwR1PnZqxU6+ZSNWcy AA2ArUWkyyOublwELrUjkllTO2MhdzyZp2chaQy+dFtNL6y5mDnYk98LN6unAkQWln3K AGVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=bPXhz94e+m5bSQ0bpYIibfQK+xo0AYYY1+cyBN+IrO4=; b=hc+fghFUgeNzVNWN1AOMKkQ4RUDSEP+WlV5//wkGr4zOBXFfGlq69Rx93MMJ3VnvRW tY7nI5yyanqoHzxw4vg9E/6AlHS6gwHhW0Gog5Ivvj4zb21ddiZ729ZUtkAPSf64Tjrn fmYfaVL1ZFYNaf+MPkNKbq5JshFyrbys7XLniRvi/vlZLwgIdz4maUfg7gP/S0OVvynU el/7mWSzwJsXn+VeOrBtfJw+bsw3OEAarP2gZJ6kt0GT7abF93lyLB+h0MNcPHjZu+Zl 6vrEHsq26nR1Cp9nyHOz2leFBeHtANvsmdJqpKJye+LQoYA1WQ7I2SQj5YI9HHdLG5yD 2ccg== X-Gm-Message-State: ABuFfoi1pR1rckfgptbLm0MLTmuQVZWvCwavnjvssE8z9V+QKAnU3KN+ NgHjmZRS5aGyM7kO0NcIgTj6FRLR+iM= X-Google-Smtp-Source: ACcGV61+nV88+zRZbEZHzYcMK4cG8J/XUce51owAw8jirJCq4VxJ8SqL4PbvUY1KpAMzjANRSclcZQ== X-Received: by 2002:a5d:47cb:: with SMTP id l11-v6mr1363848wrs.195.1539260521873; Thu, 11 Oct 2018 05:22:01 -0700 (PDT) Received: from [10.16.0.207] (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.googlemail.com with ESMTPSA id o126-v6sm19257119wmo.3.2018.10.11.05.22.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 11 Oct 2018 05:22:01 -0700 (PDT) To: Ilya Maximets , "Zhao1, Wei" , "Zhang, Qi Z" Cc: "Lu, Wenzhuo" , "Ananyev, Konstantin" , "stable@dpdk.org" , "dev@dpdk.org" 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> <20181011102428eucas1p2fe26b12282d2b456ddc2cb96ad7552f0~cht-bhDPv2507925079eucas1p2L@eucas1p2.samsung.com> From: Laurent Hardy Message-ID: <879a4ff6-b9a3-c8d8-9550-e252a053800a@6wind.com> Date: Thu, 11 Oct 2018 14:21:44 +0200 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: <20181011102428eucas1p2fe26b12282d2b456ddc2cb96ad7552f0~cht-bhDPv2507925079eucas1p2L@eucas1p2.samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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 12:22:03 -0000 Hi Wei, Please see comments below. On 10/11/2018 12:26 PM, Ilya Maximets wrote: > 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); >> The main issue which has lead to this patch has been reported through a test case with the autoneg enabled (which has been also reported in the pmd test provided along with the patch: http://patches.dpdk.org/comment/46253/). In this context, without the flag set the patch wasn't effective. >> >>>>>> 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