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 9A7201041 for ; Wed, 12 Sep 2018 10:03:40 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20180912080339euoutp02b8b12a817e7f486f06543bdc4702c116~TmFwxfV2Z0648206482euoutp02M for ; Wed, 12 Sep 2018 08:03:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20180912080339euoutp02b8b12a817e7f486f06543bdc4702c116~TmFwxfV2Z0648206482euoutp02M DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1536739419; bh=GgsyAozzfZLx/NzmhtiAyVIJ/N3xGKCHjKqKVSzPObo=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=X9I4X10Sf764W1dFLQ0zPOCceg6x7VnhtYGJ4XbscNvbpUDlJpddKuNu9CkXkhG8n HDuIncuFTLzSMye1G0AHzU/Q7hlltaanX1V2PIRwgZiVbHb27Q8Youy6aKfm1bQbZF 8uDH1kwTqbGJ2TMae8f25TAafHC0KxdZOpYKX1wA= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20180912080339eucas1p231e2117e4a748d520238dbcd91d10eb3~TmFwVj8jR2855528555eucas1p2A; Wed, 12 Sep 2018 08:03:39 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 18.F4.04294.A58C89B5; Wed, 12 Sep 2018 09:03:38 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20180912080338eucas1p1bfdacb30aa969cd607ccf99f64d6bf80~TmFveK2Dy2157121571eucas1p1U; Wed, 12 Sep 2018 08:03:38 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20180912080337eusmtrp256721385a0c38c4badfc90e25858ac98~TmFvNLhId1989119891eusmtrp2j; Wed, 12 Sep 2018 08:03:37 +0000 (GMT) X-AuditID: cbfec7f4-84fff700000010c6-5a-5b98c85a9538 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 19.0C.04284.958C89B5; Wed, 12 Sep 2018 09:03:37 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20180912080337eusmtip29e826bd8da288f118fefcdff4baf6341~TmFurg2mI0531805318eusmtip2l; Wed, 12 Sep 2018 08:03:37 +0000 (GMT) To: "Zhang, Qi Z" , "dev@dpdk.org" Cc: "Lu, Wenzhuo" , "Ananyev, Konstantin" , Laurent Hardy , "Dai, Wei" , "stable@dpdk.org" From: Ilya Maximets Date: Wed, 12 Sep 2018 11:05:18 +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: <039ED4275CED7440929022BC67E7061153284304@SHSMSX103.ccr.corp.intel.com> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJKsWRmVeSWpSXmKPExsWy7djPc7pRJ2ZEG1xcq2fx7tN2Josr7T/Z Ld7/WcRicfnMdBaL6Rv62Sz+dfxhtzjwqoXFYuuZv4wOHB4X++8wevxasJTVY/Gel0wefVtW MQawRHHZpKTmZJalFunbJXBlTDg9hbVgp01F+0yhBsbVRl2MnBwSAiYSbbN3M3UxcnEICaxg lNj/+Sg7hPOFUeLU8yNQmc+MEm8f72eDaTm+9iUTiC0ksJxR4tX2Ogj7I6PEoYv+ILawQKjE u2+TmUFsEQF3ieetDawgNrPABUaJ7idgNWwCOhKnVh9hBLFZBFQlfm99DFYvKhAhceTBQrA4 r4CgxMmZT1hAbE6BEIn+/t+MEHPEJZq+rISaKS+x/e0cZpBDJQS2sUvcvLWBFaK5TOL7rvNM EEe7SKx/u4UFwhaWeHV8CzuELSPxf+d8qJp6ifstLxkhBnUwSkw/9A8qYS+x5fU5oAYOoG2a Eut36UOEHSVet/1kBglLCPBJ3HgrCHEPn8SkbdOhwrwSHW1CENUqEr8PLmeGsKUkbr77zD6B UWkWki9nIflsFpLPZiHsXcDIsopRPLW0ODc9tdgoL7Vcrzgxt7g0L10vOT93EyMwAZ3+d/zL DsZdf5IOMQpwMCrx8K7YNz1aiDWxrLgy9xCjBAezkgiv4OoZ0UK8KYmVValF+fFFpTmpxYcY pTlYlMR5+bTSooUE0hNLUrNTUwtSi2CyTBycUg2ME5f2Z8xMOJ16+8zKpf1aOocv3RU6pb3F wf9W3v0ToQE2FtOdjvZFBfgKx1VNO/Hct/XUj8W3J31WnLvh8/eZeySs/5nZKx2sXDb/vgLr m6mssVfq527I2CqXv2PeC/fNx87enX/xN7dp2RWj1NqrNQyuWTc3qfzn+7G5I/LV/+Kq49YP OTX3LVRiKc5INNRiLipOBACP03+0PAMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrPIsWRmVeSWpSXmKPExsVy+t/xe7qRJ2ZEG5x9Ymzx7tN2Josr7T/Z Ld7/WcRicfnMdBaL6Rv62Sz+dfxhtzjwqoXFYuuZv4wOHB4X++8wevxasJTVY/Gel0wefVtW MQawROnZFOWXlqQqZOQXl9gqRRtaGOkZWlroGZlY6hkam8daGZkq6dvZpKTmZJalFunbJehl TDg9hbVgp01F+0yhBsbVRl2MnBwSAiYSx9e+ZOpi5OIQEljKKNG4dw0TREJK4sevC6wQtrDE n2tdbBBF7xkl3j6czgiSEBYIlXj3bTIziC0i4C7xvLWBFaSIWeASo8Sy/m52iI7TzBLbHz9j AaliE9CROLX6CFg3r4CdxKs9G8G6WQRUJX5vfQxmiwpESKxe/oIVokZQ4uTMJ2C9nAIhEv39 v8F6mQXUJf7Mu8QMYYtLNH1ZyQphy0tsfzuHeQKj0Cwk7bOQtMxC0jILScsCRpZVjCKppcW5 6bnFhnrFibnFpXnpesn5uZsYgZG37djPzTsYL20MPsQowMGoxMO7Yt/0aCHWxLLiytxDjBIc zEoivIKrZ0QL8aYkVlalFuXHF5XmpBYfYjQFem4is5Rocj4wKeSVxBuaGppbWBqaG5sbm1ko ifOeN6iMEhJITyxJzU5NLUgtgulj4uCUamBUOjO3WPtv8ySLG30lCR135P2mhWw8JfGjwVhy l8XR7VrJ99ou3lzGmuHVcD6FIf3hi7crWSWjPavMap1duj5d67+xJr1PcKuilbWC6oozFsaf jqZbPX/889j6A1OnpEw9wc/zbMH72ky+CJ9lCh6Tj1/+zX9y9yHR69POs607dqbtV8aq+pPX lFiKMxINtZiLihMBaQHXrtICAAA= Message-Id: <20180912080338eucas1p1bfdacb30aa969cd607ccf99f64d6bf80~TmFveK2Dy2157121571eucas1p1U@eucas1p1.samsung.com> X-CMS-MailID: 20180912080338eucas1p1bfdacb30aa969cd607ccf99f64d6bf80 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> 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: Wed, 12 Sep 2018 08:03:40 -0000 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. > >> >>> >>> 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 >>>