[dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

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.

Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
CC: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 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

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya Maximets
> <i.maximets@samsung.com>; 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?
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.

Regards
Qi

> 
> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> CC: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  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

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Laurent Hardy
>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya Maximets
>> <i.maximets@samsung.com>; 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 <i.maximets@samsung.com>
>> ---
>>  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
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Monday, September 10, 2018 11:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Laurent Hardy
> >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >> Maximets <i.maximets@samsung.com>; 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.

Btw, since ixgbe support LSC, it is not necessary for "single thread periodically checks the link statuses", right?

> 
> >
> > Regards
> > Qi
> >
> >>
> >> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> >> CC: stable@dpdk.org
> >>
> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> ---
> >>  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
> >

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

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 <qi.z.zhang@intel.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Laurent Hardy
>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>> Maximets <i.maximets@samsung.com>; 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 <i.maximets@samsung.com>
>>>> ---
>>>>  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
>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Wednesday, September 12, 2018 4:05 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Laurent Hardy
> >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>> Maximets <i.maximets@samsung.com>; 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.

Ok, you are right, seems the concern I have is already exist , your patch does not introduce new issue.
So I have no objection if this will fix some issue.
But let's check if any ixgbe experts will comment.

Regards
Qi

> 
> >
> > 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 <i.maximets@samsung.com>
> >>>> ---
> >>>>  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
> >>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Wednesday, September 12, 2018 4:29 PM
> To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> 
> 
> > -----Original Message-----
> > From: Ilya Maximets [mailto:i.maximets@samsung.com]
> > Sent: Wednesday, September 12, 2018 4:05 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Laurent Hardy
> > <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> > 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > >> <konstantin.ananyev@intel.com>; Laurent Hardy
> > >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> > >> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > >>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> > >>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> > >>>> Maximets <i.maximets@samsung.com>; 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.
> 
> Ok, you are right, seems the concern I have is already exist , your patch does
> not introduce new issue.
> So I have no objection if this will fix some issue.
> But let's check if any ixgbe experts will comment.
> 
> Regards
> Qi
> 
> >
> > >
> > > 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 <i.maximets@samsung.com>

Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>

> > >>>> ---
> > >>>>  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
> > >>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

On 21.09.2018 17:25, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
>> Sent: Wednesday, September 12, 2018 4:29 PM
>> To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Laurent Hardy
>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
>> update
>>
>>
>>
>>> -----Original Message-----
>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>> Sent: Wednesday, September 12, 2018 4:05 PM
>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>>>>> Maximets <i.maximets@samsung.com>; 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.
>>
>> Ok, you are right, seems the concern I have is already exist , your patch does
>> not introduce new issue.
>> So I have no objection if this will fix some issue.
>> But let's check if any ixgbe experts will comment.
>>
>> Regards
>> Qi
>>
>>>
>>>>
>>>> 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 <i.maximets@samsung.com>
> 
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>

Hi.
Is there any chance for this to be merged in a near future?

Situation becomes even worse. Recently accepted commit
b2815c41bd0b ("net/ixgbe: wait longer for link after fiber MAC setup")
increases the time of the thread hang busy waiting up to
1.5 seconds per port on each link state check.

Best regards, Ilya Maximets.

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

HI,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
> Sent: Wednesday, October 3, 2018 3:43 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> stable@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> On 21.09.2018 17:25, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> >> Sent: Wednesday, September 12, 2018 4:29 PM
> >> To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Laurent Hardy
> >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >> stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while
> >> fiber link update
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>> Sent: Wednesday, September 12, 2018 4:05 PM
> >>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> >>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>>>>> Maximets <i.maximets@samsung.com>; 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.
> >>
> >> Ok, you are right, seems the concern I have is already exist , your
> >> patch does not introduce new issue.
> >> So I have no objection if this will fix some issue.
> >> But let's check if any ixgbe experts will comment.
> >>
> >> Regards
> >> Qi
> >>
> >>>
> >>>>
> >>>> 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 <i.maximets@samsung.com>
> >
> > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Hi.
> Is there any chance for this to be merged in a near future?
> 
> Situation becomes even worse. Recently accepted commit b2815c41bd0b
> ("net/ixgbe: wait longer for link after fiber MAC setup") increases the time of
> the thread hang busy waiting up to
> 1.5 seconds per port on each link state check.
> 
> Best regards, Ilya Maximets.

We are review this patch now, and feedback to you ASAP, SORRY.

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi,  Ilya Maximets AND laurent.hardy


> -----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 <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Laurent Hardy
> >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>> Maximets <i.maximets@samsung.com>; 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)
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.

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.

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 <i.maximets@samsung.com>
> >>>> ---
> >>>>  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
> >>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

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 <qi.z.zhang@intel.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Laurent Hardy
>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>>>> Maximets <i.maximets@samsung.com>; 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 <i.maximets@samsung.com>
>>>>>> ---
>>>>>>  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
>>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Laurent Hardy]

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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>>>>> Maximets <i.maximets@samsung.com>; 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 <i.maximets@samsung.com>
>>>>>>> ---
>>>>>>>   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

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi,  Laurent Hardy

> -----Original Message-----
> From: Laurent Hardy [mailto:laurent.hardy@6wind.com]
> Sent: Thursday, October 11, 2018 8:22 PM
> To: Ilya Maximets <i.maximets@samsung.com>; Zhao1, Wei
> <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>>>>> Maximets <i.maximets@samsung.com>; 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.


My question is whether we can change to:

ixgbe_setup_link(hw, speed, false);

in your patch, it is " ixgbe_setup_link(hw, speed, true);"
some user may  need flexible in wait for complete.

 
> >>
> >>>>>> Regards
> >>>>>> Qi
> >>>>>>
> >>>>>>> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> >>>>>>> CC: stable@dpdk.org
> >>>>>>>
> >>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >>>>>>> ---
> >>>>>>>   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

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Laurent Hardy]

Hi Wei,

On 10/12/2018 09:36 AM, Zhao1, Wei wrote:
>>>> 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.
> My question is whether we can change to:
>
> ixgbe_setup_link(hw, speed, false);
>
> in your patch, it is " ixgbe_setup_link(hw, speed, true);"
> some user may  need flexible in wait for complete.
>
>   
Yes, the parameter (autoneg_wait_to_complete) could be change to false.
I redo the test following test plan provided through 
http://patches.dpdk.org/comment/46253/, with flag set to false
and speed defined to 1Gb on switch side.
In this case both ports goes properly up (nic used is 82599ES 
10-Gigabit) with a correct link speed.

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi, Laurent Hardy

From: Laurent Hardy [mailto:laurent.hardy@6wind.com]
Sent: Monday, October 15, 2018 6:43 PM
To: Zhao1, Wei <wei.zhao1@intel.com>; Ilya Maximets <i.maximets@samsung.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update


Hi Wei,
On 10/12/2018 09:36 AM, Zhao1, Wei wrote:

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.



My question is whether we can change to:



ixgbe_setup_link(hw, speed, false);



in your patch, it is " ixgbe_setup_link(hw, speed, true);"

some user may  need flexible in wait for complete.




Yes, the parameter (autoneg_wait_to_complete) could be change to false.
I redo the test following test plan provided through http://patches.dpdk.org/comment/46253/, with flag set to false
and speed defined to 1Gb on switch side.
In this case both ports goes properly up (nic used is 82599ES 10-Gigabit) with a correct link speed.

Thank you for your test and feedback!

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi， 

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Thursday, October 11, 2018 6:27 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Laurent Hardy <laurent.hardy@6wind.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Laurent Hardy
> >> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>>>> Maximets <i.maximets@samsung.com>; 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?

rte_eth_link_get() ,  it will also call the function of ixgbe_setup_link().


> 
> >
> > 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").

Yes, you are right, link state check loop in function ixgbe_setup_mac_link_multispeed_fiber() are not blocked by bool autoneg_wait_to_complete,
It will cause about 1s wait when port is down.
And also, can we update code in function ixgbe_setup_mac_link_multispeed_fiber() to  block link state check loop using autoneg_wait_to_complete?
I am not sure. Because there is a comment for this loop check:
		/*
		 * Wait for the controller to acquire link.  Per IEEE 802.3ap,
		 * Section 73.10.2, we may have to wait up to 500ms if KR is
		 * attempted.  82599 uses the same timing for 10g SFI.
		 */
It seems we have to wait for at least 500ms for spec requirement before we check link after configuration.
If that is true, we can not do any change to these loop check.
But why not main thread take some action to stop periodic link sate check when it find it has been hang or link is down?
 


> 
> >
> > 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 <i.maximets@samsung.com>
> >>>>>> ---
> >>>>>>  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
> >>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

On 12.10.2018 12:19, Zhao1, Wei wrote:
> Hi， 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Thursday, October 11, 2018 6:27 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> Laurent Hardy <laurent.hardy@6wind.com>
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
>> update
>>
>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>>>>>> Maximets <i.maximets@samsung.com>; 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?
> 
> rte_eth_link_get() ,  it will also call the function of ixgbe_setup_link().
> 

rte_eth_link_get() does not call ixgbe_setup_link().
It only calls dev_ops->link_update().

> 
>>
>>>
>>> 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").
> 
> Yes, you are right, link state check loop in function ixgbe_setup_mac_link_multispeed_fiber() are not blocked by bool autoneg_wait_to_complete,
> It will cause about 1s wait when port is down.
> And also, can we update code in function ixgbe_setup_mac_link_multispeed_fiber() to  block link state check loop using autoneg_wait_to_complete?
> I am not sure. Because there is a comment for this loop check:
> 		/*
> 		 * Wait for the controller to acquire link.  Per IEEE 802.3ap,
> 		 * Section 73.10.2, we may have to wait up to 500ms if KR is
> 		 * attempted.  82599 uses the same timing for 10g SFI.
> 		 */
> It seems we have to wait for at least 500ms for spec requirement before we check link after configuration.
> If that is true, we can not do any change to these loop check.
> But why not main thread take some action to stop periodic link sate check when it find it has been hang or link is down?

To find that device is DOWN, thread will have to call this
function at least once for each port and wait a few seconds.
And how in that case we'll know that device is UP again?
As I already wrote in discussion for this patch, LSC is not
an option, because it takes at least 5 seconds to detect link
state change, which is way too much for many real world apps.

>  
> 
> 
>>
>>>
>>> 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 <i.maximets@samsung.com>
>>>>>>>> ---
>>>>>>>>  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
>>>>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi， Ilya Maximets

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Friday, October 12, 2018 6:15 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Laurent Hardy <laurent.hardy@6wind.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> On 12.10.2018 12:19, Zhao1, Wei wrote:
> > Hi，
> >
> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >> Sent: Thursday, October 11, 2018 6:27 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Laurent Hardy <laurent.hardy@6wind.com>
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while
> >> fiber link update
> >>
> >> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
> >>>>>>>> Maximets <i.maximets@samsung.com>; 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?
> >
> > rte_eth_link_get() ,  it will also call the function of ixgbe_setup_link().
> >
> 
> rte_eth_link_get() does not call ixgbe_setup_link().
> It only calls dev_ops->link_update().

No,  dev_ops->link_update call function ixgbe_dev_link_update()
-> ixgbe_dev_link_update_share() -> ixgbe_setup_link()



> 
> >
> >>
> >>>
> >>> 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").
> >
> > Yes, you are right, link state check loop in function
> > ixgbe_setup_mac_link_multispeed_fiber() are not blocked by bool
> autoneg_wait_to_complete, It will cause about 1s wait when port is down.
> > And also, can we update code in function
> ixgbe_setup_mac_link_multispeed_fiber() to  block link state check loop
> using autoneg_wait_to_complete?
> > I am not sure. Because there is a comment for this loop check:
> > 		/*
> > 		 * Wait for the controller to acquire link.  Per IEEE 802.3ap,
> > 		 * Section 73.10.2, we may have to wait up to 500ms if KR is
> > 		 * attempted.  82599 uses the same timing for 10g SFI.
> > 		 */
> > It seems we have to wait for at least 500ms for spec requirement before
> we check link after configuration.
> > If that is true, we can not do any change to these loop check.
> > But why not main thread take some action to stop periodic link sate check
> when it find it has been hang or link is down?
> 
> To find that device is DOWN, thread will have to call this function at least
> once for each port and wait a few seconds.
> And how in that case we'll know that device is UP again?
> As I already wrote in discussion for this patch, LSC is not an option, because it
> takes at least 5 seconds to detect link state change, which is way too much
> for many real world apps.
> 
> >
> >
> >
> >>
> >>>
> >>> 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 <i.maximets@samsung.com>
> >>>>>>>> ---
> >>>>>>>>  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
> >>>>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

On 15.10.2018 06:03, Zhao1, Wei wrote:
> Hi， Ilya Maximets
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Friday, October 12, 2018 6:15 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> Laurent Hardy <laurent.hardy@6wind.com>
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
>> update
>>
>> On 12.10.2018 12:19, Zhao1, Wei wrote:
>>> Hi，
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>> Sent: Thursday, October 11, 2018 6:27 PM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>; Laurent Hardy <laurent.hardy@6wind.com>
>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while
>>>> fiber link update
>>>>
>>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
>>>>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>>>>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
>>>>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>; Ilya
>>>>>>>>>> Maximets <i.maximets@samsung.com>; 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?
>>>
>>> rte_eth_link_get() ,  it will also call the function of ixgbe_setup_link().
>>>
>>
>> rte_eth_link_get() does not call ixgbe_setup_link().
>> It only calls dev_ops->link_update().
> 
> No,  dev_ops->link_update call function ixgbe_dev_link_update()
> -> ixgbe_dev_link_update_share() -> ixgbe_setup_link()

But with my patch, calling of ixgbe_setup_link() happens in a separate
(interrupt) thread. There is no direct call from ixgbe_dev_link_update_share().
All the calls from the interrupt thread are sequenced by implementation.

> 
> 
>>
>>>
>>>>
>>>>>
>>>>> 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").
>>>
>>> Yes, you are right, link state check loop in function
>>> ixgbe_setup_mac_link_multispeed_fiber() are not blocked by bool
>> autoneg_wait_to_complete, It will cause about 1s wait when port is down.
>>> And also, can we update code in function
>> ixgbe_setup_mac_link_multispeed_fiber() to  block link state check loop
>> using autoneg_wait_to_complete?
>>> I am not sure. Because there is a comment for this loop check:
>>> 		/*
>>> 		 * Wait for the controller to acquire link.  Per IEEE 802.3ap,
>>> 		 * Section 73.10.2, we may have to wait up to 500ms if KR is
>>> 		 * attempted.  82599 uses the same timing for 10g SFI.
>>> 		 */
>>> It seems we have to wait for at least 500ms for spec requirement before
>> we check link after configuration.
>>> If that is true, we can not do any change to these loop check.
>>> But why not main thread take some action to stop periodic link sate check
>> when it find it has been hang or link is down?
>>
>> To find that device is DOWN, thread will have to call this function at least
>> once for each port and wait a few seconds.
>> And how in that case we'll know that device is UP again?
>> As I already wrote in discussion for this patch, LSC is not an option, because it
>> takes at least 5 seconds to detect link state change, which is way too much
>> for many real world apps.
>>
>>>
>>>
>>>
>>>>
>>>>>
>>>>> 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 <i.maximets@samsung.com>
>>>>>>>>>> ---
>>>>>>>>>>  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
>>>>>>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhao1, Wei]

Hi, Ilya Maximets

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Monday, October 15, 2018 4:40 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Laurent Hardy <laurent.hardy@6wind.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link
> update
> 
> On 15.10.2018 06:03, Zhao1, Wei wrote:
> > Hi， Ilya Maximets
> >
> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >> Sent: Friday, October 12, 2018 6:15 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Laurent Hardy <laurent.hardy@6wind.com>
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while
> >> fiber link update
> >>
> >> On 12.10.2018 12:19, Zhao1, Wei wrote:
> >>> Hi，
> >>>
> >>>> -----Original Message-----
> >>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>>> Sent: Thursday, October 11, 2018 6:27 PM
> >>>> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Qi Z
> >>>> <qi.z.zhang@intel.com>; Laurent Hardy <laurent.hardy@6wind.com>
> >>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; stable@dpdk.org; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while
> >>>> fiber link update
> >>>>
> >>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>>>> 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 <qi.z.zhang@intel.com>; dev@dpdk.org
> >>>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> >>>>>>>> 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 <wenzhuo.lu@intel.com>; Ananyev,
> Konstantin
> >>>>>>>>>> <konstantin.ananyev@intel.com>; Laurent Hardy
> >>>>>>>>>> <laurent.hardy@6wind.com>; Dai, Wei <wei.dai@intel.com>;
> Ilya
> >>>>>>>>>> Maximets <i.maximets@samsung.com>; 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?
> >>>
> >>> rte_eth_link_get() ,  it will also call the function of ixgbe_setup_link().
> >>>
> >>
> >> rte_eth_link_get() does not call ixgbe_setup_link().
> >> It only calls dev_ops->link_update().
> >
> > No,  dev_ops->link_update call function ixgbe_dev_link_update()
> > -> ixgbe_dev_link_update_share() -> ixgbe_setup_link()
> 
> But with my patch, calling of ixgbe_setup_link() happens in a separate
> (interrupt) thread. There is no direct call from
> ixgbe_dev_link_update_share().
> All the calls from the interrupt thread are sequenced by implementation.

Yes, that is just the point that I worry about.
ixgbe_setup_link() process will hang 1s at least in other thread even if it is move out from the link status update which is being handle in user thread now.
I can not foresee what impact it will have to ixgbe PMD thread that long time, it may be a hidden danger. 
And the wait time of 1s of link setup can not eliminate also. The difference is which thread to block.
I agree with you of the exist of this hang time for user thread in link status update for set up link when port is down, 
but I can not support the code to move hang time to another thread with reason above.
Thank you for your explanation. 
I am willing to see if anyone can give an definite conclusion for this code change and ACK this patch.

Reviewed-by: Wei Zhao <wei.zhao1@intel.com>
 

> 
> >
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>> 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").
> >>>
> >>> Yes, you are right, link state check loop in function
> >>> ixgbe_setup_mac_link_multispeed_fiber() are not blocked by bool
> >> autoneg_wait_to_complete, It will cause about 1s wait when port is down.
> >>> And also, can we update code in function
> >> ixgbe_setup_mac_link_multispeed_fiber() to  block link state check
> >> loop using autoneg_wait_to_complete?
> >>> I am not sure. Because there is a comment for this loop check:
> >>> 		/*
> >>> 		 * Wait for the controller to acquire link.  Per IEEE 802.3ap,
> >>> 		 * Section 73.10.2, we may have to wait up to 500ms if KR is
> >>> 		 * attempted.  82599 uses the same timing for 10g SFI.
> >>> 		 */
> >>> It seems we have to wait for at least 500ms for spec requirement
> >>> before
> >> we check link after configuration.
> >>> If that is true, we can not do any change to these loop check.
> >>> But why not main thread take some action to stop periodic link sate
> >>> check
> >> when it find it has been hang or link is down?
> >>
> >> To find that device is DOWN, thread will have to call this function
> >> at least once for each port and wait a few seconds.
> >> And how in that case we'll know that device is UP again?
> >> As I already wrote in discussion for this patch, LSC is not an
> >> option, because it takes at least 5 seconds to detect link state
> >> change, which is way too much for many real world apps.
> >>
> >>>
> >>>
> >>>
> >>>>
> >>>>>
> >>>>> 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 <i.maximets@samsung.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  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
> >>>>>>>>>

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

Any more thoughts on that fix?
Or is there any chance for it to be accepted?

Best regards, Ilya Maximets.

On 31.08.2018 15:39, Ilya Maximets wrote:
> 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.
> 
> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> CC: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  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;
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Tuesday, October 30, 2018 5:21 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Wei Dai <wei.dai@intel.com>;
> stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [PATCH] net/ixgbe: fix busy polling while fiber link update
> 
> Any more thoughts on that fix?
> Or is there any chance for it to be accepted?

Would you rebase to dpdk-next-net-intel?

Thanks
Qi

> 
> Best regards, Ilya Maximets.
> 
> On 31.08.2018 15:39, Ilya Maximets wrote:
> > 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.
> >
> > Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> > CC: stable@dpdk.org
> >
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > ---
> >  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;
> >

Re: [dpdk-dev] [PATCH] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

On 01.11.2018 18:45, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Tuesday, October 30, 2018 5:21 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Laurent Hardy
>> <laurent.hardy@6wind.com>; Wei Dai <wei.dai@intel.com>;
>> stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [PATCH] net/ixgbe: fix busy polling while fiber link update
>>
>> Any more thoughts on that fix?
>> Or is there any chance for it to be accepted?
> 
> Would you rebase to dpdk-next-net-intel?

Done.

> Thanks
> Qi
> 
>>
>> Best regards, Ilya Maximets.
>>
>> On 31.08.2018 15:39, Ilya Maximets wrote:
>>> 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.
>>>
>>> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
>>> CC: stable@dpdk.org
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> ---
>>>  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;
>>>

[dpdk-dev] [PATCH v2] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

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.

Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
CC: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---

Version 2:
	* Minor rebase on dpdk-next-net-intel/master

 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 5a2c35143..c9e82d515 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -220,6 +220,8 @@ static int ixgbe_dev_interrupt_get_status(struct rte_eth_dev *dev);
 static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev);
 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);
@@ -2793,6 +2795,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);
 
@@ -3971,6 +3975,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,
@@ -3983,9 +4006,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;
@@ -3995,13 +4016,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)
@@ -4019,11 +4035,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

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix busy polling while fiber link update

[Zhang, Qi Z]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Thursday, November 1, 2018 11:05 AM
> To: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Laurent Hardy
> <laurent.hardy@6wind.com>; Wei Dai <wei.dai@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Ilya Maximets <i.maximets@samsung.com>;
> stable@dpdk.org
> Subject: [PATCH v2] 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.
> 
> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> CC: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix busy polling while fiber link update

[Burakov, Anatoly]

On 01-Nov-18 4:04 PM, Ilya Maximets wrote:
> 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.
> 
> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
> CC: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---

On my setup, this commit breaks ixgbe init in pktgen 3.5.7:

ixgbe_dev_start(): failure in ixgbe_dev_start(): -15
!PANIC!: rte_eth_dev_start: port=0, Input/output error
PANIC in pktgen_config_ports():
rte_eth_dev_start: port=0, Input/output error6: 
[build/DPDK/pktgen(_start+0x2a) [0x560880ec838a]]
5: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) 
[0x7fabc654cb97]]
4: [build/DPDK/pktgen(main+0xe77) [0x560880ec0357]]
3: [build/DPDK/pktgen(pktgen_config_ports+0x1cf0) [0x560880ef53e0]]
2: [build/DPDK/pktgen(__rte_panic+0xc5) [0x560880eb11b4]]
1: [build/DPDK/pktgen(rte_dump_stack+0x2e) [0x560880fde69e]]
Aborted

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

On 07.11.2018 18:52, Burakov, Anatoly wrote:
> On 01-Nov-18 4:04 PM, Ilya Maximets wrote:
>> 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.
>>
>> Fixes: c12d22f65b13 ("net/ixgbe: ensure link status is updated")
>> CC: stable@dpdk.org
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
> 
> On my setup, this commit breaks ixgbe init in pktgen 3.5.7:
> 
> ixgbe_dev_start(): failure in ixgbe_dev_start(): -15
> !PANIC!: rte_eth_dev_start: port=0, Input/output error
> PANIC in pktgen_config_ports():
> rte_eth_dev_start: port=0, Input/output error6: [build/DPDK/pktgen(_start+0x2a) [0x560880ec838a]]
> 5: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) [0x7fabc654cb97]]
> 4: [build/DPDK/pktgen(main+0xe77) [0x560880ec0357]]
> 3: [build/DPDK/pktgen(pktgen_config_ports+0x1cf0) [0x560880ef53e0]]
> 2: [build/DPDK/pktgen(__rte_panic+0xc5) [0x560880eb11b4]]
> 1: [build/DPDK/pktgen(rte_dump_stack+0x2e) [0x560880fde69e]]
> Aborted
> 

Hi Anatoly,
Thanks for the report.

Could you, please, try the following patch I prepared:
	http://patches.dpdk.org/patch/47939/
?

Best regards, Ilya Maximets.