On 12.09.2018 09:49, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ilya Maximets [mailto:[email protected]]
>> Sent: Monday, September 10, 2018 11:09 PM
>> To: Zhang, Qi Z <[email protected]>; [email protected]
>> Cc: Lu, Wenzhuo <[email protected]>; Ananyev, Konstantin
>> <[email protected]>; Laurent Hardy
>> <[email protected]>; Dai, Wei <[email protected]>;
>> [email protected]
>> 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:[email protected]] On Behalf Of Ilya Maximets
>>>> Sent: Friday, August 31, 2018 8:40 PM
>>>> To: [email protected]
>>>> Cc: Lu, Wenzhuo <[email protected]>; Ananyev, Konstantin
>>>> <[email protected]>; Laurent Hardy
>>>> <[email protected]>; Dai, Wei <[email protected]>; Ilya
>>>> Maximets <[email protected]>; [email protected]
>>>> 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: [email protected]
>>>>
>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>> ---
>>>> 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
>>>
