[dpdk-stable] [PATCH 17.11 0/2] net/ixgbe: busy polling fix backport

[dpdk-stable] [PATCH 17.11 0/2] net/ixgbe: busy polling fix backport

[Ilya Maximets]

Ilya Maximets (2):
  net/ixgbe: fix busy polling while fiber link update
  net/ixgbe: stop link setup alarm handler before device start

 drivers/net/ixgbe/ixgbe_ethdev.c | 64 +++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 10 deletions(-)

-- 
2.17.1

[dpdk-stable] [PATCH 17.11 1/2] net/ixgbe: fix busy polling while fiber link update

[Ilya Maximets]

[ backported from upstream commit 0408f47ba4d67f0cb96f5e907bc9336b6cbff0dd ]

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>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
Reviewed-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 46 +++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 5f7fb0f1d..be836beeb 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -254,6 +254,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);
@@ -2759,6 +2761,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);
 
@@ -3954,6 +3958,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 */
 static int
 ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
@@ -3966,9 +3989,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;
 
 	link.link_status = ETH_LINK_DOWN;
 	link.link_speed = 0;
@@ -3979,12 +4000,11 @@ 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) {
+		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+		if (link.link_status == old.link_status)
+			return -1;
+		return 0;
 	}
 
 	/* check if it needs to wait to complete, if lsc interrupt is enabled */
@@ -4007,12 +4027,16 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 
 	if (link_up == 0) {
 		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
-		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);
+		}
 		if (link.link_status == old.link_status)
 			return -1;
 		return 0;
 	}
-	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
+
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
 
@@ -5102,6 +5126,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(hw);
 
 	hw->adapter_stopped = 1;
-- 
2.17.1

[dpdk-stable] [PATCH 17.11 2/2] net/ixgbe: stop link setup alarm handler before device start

[Ilya Maximets]

[ backported from upstream commit 916193dda6fb400114c0ffc5615e74ca13d50720 ]

We should stop alarm handler before resetting the HW to
avoid concurrent device reconfiguration.

Fixes: 0408f47ba4d6 ("net/ixgbe: fix busy polling while fiber link update")
CC: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index be836beeb..f047db83c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2533,6 +2533,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
+	/* Stop the link setup handler before resetting the HW. */
+	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
+
 	/* disable uio/vfio intr/eventfd mapping */
 	rte_intr_disable(intr_handle);
 
@@ -2734,6 +2737,12 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 			    "please call hierarchy_commit() "
 			    "before starting the port");
 
+	/*
+	 * Update link status right before return, because it may
+	 * start link configuration process in a separate thread.
+	 */
+	ixgbe_dev_link_update(dev, 0);
+
 	return 0;
 
 error:
@@ -5042,6 +5051,9 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* Stop the link setup handler before resetting the HW. */
+	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
+
 	err = hw->mac.ops.reset_hw(hw);
 	if (err) {
 		PMD_INIT_LOG(ERR, "Unable to reset vf hardware (%d)", err);
@@ -5114,6 +5126,12 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
 	/* Re-enable interrupt for VF */
 	ixgbevf_intr_enable(hw);
 
+	/*
+	 * Update link status right before return, because it may
+	 * start link configuration process in a separate thread.
+	 */
+	ixgbevf_dev_link_update(dev, 0);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-stable] [PATCH 17.11 0/2] net/ixgbe: busy polling fix backport

[Yongseok Koh]

> On Jan 22, 2019, at 7:13 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
> 
> Ilya Maximets (2):
>  net/ixgbe: fix busy polling while fiber link update
>  net/ixgbe: stop link setup alarm handler before device start
> 
> drivers/net/ixgbe/ixgbe_ethdev.c | 64 +++++++++++++++++++++++++++-----
> 1 file changed, 54 insertions(+), 10 deletions(-)
> 
> -- 

Series applied to stable/17.11

Thanks,
Yongseok