diff mbox series

[v2] net/ixgbe: fix busy polling while fiber link update

Message ID 20181101160459.23586-1-i.maximets@samsung.com (mailing list archive)
State Accepted, archived
Delegated to: Qi Zhang
Headers
Series [v2] net/ixgbe: fix busy polling while fiber link update |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ilya Maximets Nov. 1, 2018, 4:04 p.m. UTC 
  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(-)

Comments

Qi Zhang Nov. 2, 2018, 1:49 p.m. UTC | #1 
> -----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
Anatoly Burakov Nov. 7, 2018, 3:52 p.m. UTC | #2 
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
Ilya Maximets Nov. 8, 2018, 10:27 a.m. UTC | #3 
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.
diff mbox series

Patch

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;