[dpdk-dev,v6,1/2] net/bond: fixes for link properties management

Message ID 20170705185429.13591-1-declan.doherty@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

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

Commit Message

Doherty, Declan July 5, 2017, 6:54 p.m. UTC
From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch fixes the management of link properties in the bonded device.

In all mode except mode 4 a bonded device link will default to reporting
the link as full duplex and auto-neg. The link speed for a bond port is
calculated on it's active slaves and the particular mode it is running
in. The bonding link speed is reported based on the transmit link as in
some modes link speed between egress/ingress is not symmetrical.

- round-robin, balance, 802.3ad, TLB and ALB modes all report the link
  speed as the sum of the speed of each active slave.
- active backup link speed is reported as the speed of the current
  primary slave
- broadcast is reported as the minimum of value of the active slaves
  link speeds.

In mode 4 (link aggregation 802.3ad) the properties of the first slave
added to the bonded device are slave and subsequent slaves are verified
to have the same properties.

Finally in the bond_ethdev_lsc_event_callback function the link
properties of the device are updated after any change to the number of
active slaves.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v6:
- fix unused variable warning
v5:
- add back in change to drivers/net/bonding/rte_eth_bond_8023ad_private.h lost
in rebase 
v4:

- rebased
v3:

- reworked link management changes and split into separate patch

 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   3 +
 drivers/net/bonding/rte_eth_bond_api.c            |   6 +-
 drivers/net/bonding/rte_eth_bond_pmd.c            | 176 +++++++++++++---------
 drivers/net/bonding/rte_eth_bond_private.h        |   8 +-
 4 files changed, 116 insertions(+), 77 deletions(-)
  

Comments

Ferruh Yigit July 6, 2017, 9:33 a.m. UTC | #1
On 7/5/2017 7:54 PM, Declan Doherty wrote:
> From: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> 
> This patch fixes the management of link properties in the bonded device.
> 
> In all mode except mode 4 a bonded device link will default to reporting
> the link as full duplex and auto-neg. The link speed for a bond port is
> calculated on it's active slaves and the particular mode it is running
> in. The bonding link speed is reported based on the transmit link as in
> some modes link speed between egress/ingress is not symmetrical.
> 
> - round-robin, balance, 802.3ad, TLB and ALB modes all report the link
>   speed as the sum of the speed of each active slave.
> - active backup link speed is reported as the speed of the current
>   primary slave
> - broadcast is reported as the minimum of value of the active slaves
>   link speeds.
> 
> In mode 4 (link aggregation 802.3ad) the properties of the first slave
> added to the bonded device are slave and subsequent slaves are verified
> to have the same properties.
> 
> Finally in the bond_ethdev_lsc_event_callback function the link
> properties of the device are updated after any change to the number of
> active slaves.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>


Series applied to dpdk-next-net/master, thanks.

(Signed-off from Tomasz, exists in older versions, added back)
  

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index c16dba8..802551d 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -180,6 +180,9 @@  struct mode8023ad_private {
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	uint8_t external_sm;
 
+	struct rte_eth_link slave_link;
+	/***< slave link properties */
+
 	/**
 	 * Configuration of dedicated hardware queues for control plane
 	 * traffic
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..82c4499 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -297,6 +297,9 @@  __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa &= dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads &= dev_info.flow_type_rss_offloads;
 
+		link_properties_valid(bonded_eth_dev,
+				&slave_eth_dev->data->dev_link);
+
 		/* RETA size is GCD of all slaves RETA sizes, so, if all sizes will be
 		 * the power of 2, the lower one is GCD
 		 */
@@ -439,9 +442,6 @@  __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	}
 
 	if (internals->active_slave_count < 1) {
-		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
-
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 60569c6..d3cbc9d 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1386,39 +1386,44 @@  bond_ethdev_tx_burst_broadcast(void *queue, struct rte_mbuf **bufs,
 }
 
 void
-link_properties_set(struct rte_eth_dev *bonded_eth_dev,
-		struct rte_eth_link *slave_dev_link)
+link_properties_set(struct rte_eth_dev *ethdev, struct rte_eth_link *slave_link)
 {
-	struct rte_eth_link *bonded_dev_link = &bonded_eth_dev->data->dev_link;
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	if (slave_dev_link->link_status &&
-		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		/**
+		 * If in mode 4 then save the link properties of the first
+		 * slave, all subsequent slaves must match these properties
+		 */
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-		internals->link_props_set = 1;
+		bond_link->link_autoneg = slave_link->link_autoneg;
+		bond_link->link_duplex = slave_link->link_duplex;
+		bond_link->link_speed = slave_link->link_speed;
+	} else {
+		/**
+		 * In any other mode the link properties are set to default
+		 * values of AUTONEG/DUPLEX
+		 */
+		ethdev->data->dev_link.link_autoneg = ETH_LINK_AUTONEG;
+		ethdev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	}
 }
 
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev)
+int
+link_properties_valid(struct rte_eth_dev *ethdev,
+		struct rte_eth_link *slave_link)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	memset(&(bonded_eth_dev->data->dev_link), 0,
-			sizeof(bonded_eth_dev->data->dev_link));
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-	internals->link_props_set = 0;
-}
-
-int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
-		struct rte_eth_link *slave_dev_link)
-{
-	if (bonded_dev_link->link_duplex != slave_dev_link->link_duplex ||
-		bonded_dev_link->link_speed !=  slave_dev_link->link_speed)
-		return -1;
+		if (bond_link->link_duplex != slave_link->link_duplex ||
+			bond_link->link_autoneg != slave_link->link_autoneg ||
+			bond_link->link_speed != slave_link->link_speed)
+			return -1;
+	}
 
 	return 0;
 }
@@ -2270,36 +2275,90 @@  bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 }
 
 static int
-bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
-		int wait_to_complete)
+bond_ethdev_link_update(struct rte_eth_dev *ethdev, int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	void (*link_update)(uint8_t port_id, struct rte_eth_link *eth_link);
 
-	if (!bonded_eth_dev->data->dev_started ||
-		internals->active_slave_count == 0) {
-		bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	struct bond_dev_private *bond_ctx;
+	struct rte_eth_link slave_link;
+
+	uint32_t idx;
+
+	bond_ctx = ethdev->data->dev_private;
+
+	ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+	if (ethdev->data->dev_started == 0 ||
+			bond_ctx->active_slave_count == 0) {
+		ethdev->data->dev_link.link_status = ETH_LINK_DOWN;
 		return 0;
-	} else {
-		struct rte_eth_dev *slave_eth_dev;
-		int i, link_up = 0;
+	}
 
-		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+	ethdev->data->dev_link.link_status = ETH_LINK_UP;
 
-			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
-					wait_to_complete);
-			if (slave_eth_dev->data->dev_link.link_status == ETH_LINK_UP) {
-				link_up = 1;
-				break;
-			}
+	if (wait_to_complete)
+		link_update = rte_eth_link_get;
+	else
+		link_update = rte_eth_link_get_nowait;
+
+	switch (bond_ctx->mode) {
+	case BONDING_MODE_BROADCAST:
+		/**
+		 * Setting link speed to UINT32_MAX to ensure we pick up the
+		 * value of the first active slave
+		 */
+		ethdev->data->dev_link.link_speed = UINT32_MAX;
+
+		/**
+		 * link speed is minimum value of all the slaves link speed as
+		 * packet loss will occur on this slave if transmission at rates
+		 * greater than this are attempted
+		 */
+		for (idx = 1; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[0],	&slave_link);
+
+			if (slave_link.link_speed <
+					ethdev->data->dev_link.link_speed)
+				ethdev->data->dev_link.link_speed =
+						slave_link.link_speed;
 		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		link_update(bond_ctx->current_primary_port, &slave_link);
+
+		ethdev->data->dev_link.link_speed = slave_link.link_speed;
+		break;
+	case BONDING_MODE_8023AD:
+		ethdev->data->dev_link.link_autoneg =
+				bond_ctx->mode4.slave_link.link_autoneg;
+		ethdev->data->dev_link.link_duplex =
+				bond_ctx->mode4.slave_link.link_duplex;
+		/* fall through to update link speed */
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/**
+		 * In theses mode the maximum theoretical link speed is the sum
+		 * of all the slaves
+		 */
+		ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+		for (idx = 0; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[idx], &slave_link);
 
-		bonded_eth_dev->data->dev_link.link_status = link_up;
+			ethdev->data->dev_link.link_speed +=
+					slave_link.link_speed;
+		}
 	}
 
+
 	return 0;
 }
 
+
 static void
 bond_ethdev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -2410,7 +2469,7 @@  int
 bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		void *param, void *ret_param __rte_unused)
 {
-	struct rte_eth_dev *bonded_eth_dev, *slave_eth_dev;
+	struct rte_eth_dev *bonded_eth_dev;
 	struct bond_dev_private *internals;
 	struct rte_eth_link link;
 	int rc = -1;
@@ -2423,7 +2482,6 @@  bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		return rc;
 
 	bonded_eth_dev = &rte_eth_devices[*(uint8_t *)param];
-	slave_eth_dev = &rte_eth_devices[port_id];
 
 	if (check_for_bonded_ethdev(bonded_eth_dev))
 		return rc;
@@ -2462,20 +2520,6 @@  bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 			lsc_flag = 1;
 
 			mac_address_slaves_update(bonded_eth_dev);
-
-			/* Inherit eth dev link properties from first active slave */
-			link_properties_set(bonded_eth_dev,
-					&(slave_eth_dev->data->dev_link));
-		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
-					(~RTE_ETH_DEV_BONDED_SLAVE);
-				RTE_LOG(ERR, PMD,
-					"port %u invalid speed/duplex\n",
-					port_id);
-				return rc;
-			}
 		}
 
 		activate_slave(bonded_eth_dev, port_id);
@@ -2491,15 +2535,6 @@  bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		/* Remove from active slave list */
 		deactivate_slave(bonded_eth_dev, port_id);
 
-		/* No active slaves, change link status to down and reset other
-		 * link properties */
-		if (internals->active_slave_count < 1) {
-			lsc_flag = 1;
-			bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
-			link_properties_reset(bonded_eth_dev);
-		}
-
 		/* Update primary id, take first active slave from list or if none
 		 * available set to -1 */
 		if (port_id == internals->current_primary_port) {
@@ -2511,6 +2546,12 @@  bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		}
 	}
 
+	/**
+	 * Update bonded device link properties after any change to active
+	 * slaves
+	 */
+	bond_ethdev_link_update(bonded_eth_dev, 0);
+
 	if (lsc_flag) {
 		/* Cancel any possible outstanding interrupts if delays are enabled */
 		if (internals->link_up_delay_ms > 0 ||
@@ -2714,7 +2755,6 @@  bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	internals->balance_xmit_policy = BALANCE_XMIT_POLICY_LAYER2;
 	internals->xmit_hash = xmit_l2_hash;
 	internals->user_defined_mac = 0;
-	internals->link_props_set = 0;
 
 	internals->link_status_polling_enabled = 0;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 53470f6..7295192 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -132,8 +132,7 @@  struct bond_dev_private {
 	/**< Flag for whether MAC address is user defined or not */
 	uint8_t promiscuous_en;
 	/**< Enabled/disable promiscuous mode on bonding device */
-	uint8_t link_props_set;
-	/**< flag to denote if the link properties are set */
+
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
@@ -216,11 +215,8 @@  activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
 void
 link_properties_set(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev);
-
 int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
+link_properties_valid(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
 
 int