[dpdk-dev] bonding: fix wrong slaves capacity check

Message ID 20170727120027.30416-1-tomaszx.kulasek@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 success Compilation OK

Commit Message

Tomasz Kulasek July 27, 2017, noon UTC
  For fortville NIC bond_ethdev_8023ad_flow_verify fails when action queue
index indicates unavailable queue before slaves configuration.

This fix verifies flow settings for queue 0, which is always available,
and checks if slaves max queue number capacity meets requirements.

Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
  

Comments

Radu Nicolau Sept. 4, 2017, 1:01 p.m. UTC | #1
On 7/27/2017 1:00 PM, Tomasz Kulasek wrote:
> For fortville NIC bond_ethdev_8023ad_flow_verify fails when action queue
> index indicates unavailable queue before slaves configuration.
>
> This fix verifies flow settings for queue 0, which is always available,
> and checks if slaves max queue number capacity meets requirements.
>
> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Reviewed-by:  Radu Nicolau <radu.nicolau@intel.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 8f9a860..ab5ebe0 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -175,12 +175,13 @@ const struct rte_flow_attr flow_attr_8023ad = {
>   int
>   bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
>   		uint8_t slave_port) {
> +	struct rte_eth_dev_info slave_info;
>   	struct rte_flow_error error;
>   	struct bond_dev_private *internals = (struct bond_dev_private *)
>   			(bond_dev->data->dev_private);
>   
> -	struct rte_flow_action_queue lacp_queue_conf = {
> -		.index = internals->mode4.dedicated_queues.rx_qid,
> +	const struct rte_flow_action_queue lacp_queue_conf = {
> +		.index = 0,
>   	};
>   
>   	const struct rte_flow_action actions[] = {
> @@ -195,8 +196,22 @@ bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
>   
>   	int ret = rte_flow_validate(slave_port, &flow_attr_8023ad,
>   			flow_item_8023ad, actions, &error);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_verify: %s "
> +				"(slave_port=%d queue_id=%d)",
> +				error.message, slave_port,
> +				internals->mode4.dedicated_queues.rx_qid);
> +		return -1;
> +	}
> +
> +	rte_eth_dev_info_get(slave_port, &slave_info);
> +	if ((slave_info.max_rx_queues < bond_dev->data->nb_rx_queues) ||
> +			(slave_info.max_tx_queues < bond_dev->data->nb_tx_queues)) {
> +		RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_verify: Slave %d"
> +				" capabilities doesn't allow to allocate "
> +				"additional queues", slave_port);
>   		return -1;
> +	}
>   
>   	return 0;
>   }
> @@ -206,7 +221,7 @@ bond_8023ad_slow_pkt_hw_filter_supported(uint8_t port_id) {
>   	struct rte_eth_dev *bond_dev = &rte_eth_devices[port_id];
>   	struct bond_dev_private *internals = (struct bond_dev_private *)
>   			(bond_dev->data->dev_private);
> -	struct rte_eth_dev_info bond_info, slave_info;
> +	struct rte_eth_dev_info bond_info;
>   	uint8_t idx;
>   
>   	/* Verify if all slaves in bonding supports flow director and */
> @@ -217,9 +232,6 @@ bond_8023ad_slow_pkt_hw_filter_supported(uint8_t port_id) {
>   		internals->mode4.dedicated_queues.tx_qid = bond_info.nb_tx_queues;
>   
>   		for (idx = 0; idx < internals->slave_count; idx++) {
> -			rte_eth_dev_info_get(internals->slaves[idx].port_id,
> -					&slave_info);
> -
>   			if (bond_ethdev_8023ad_flow_verify(bond_dev,
>   					internals->slaves[idx].port_id) != 0)
>   				return -1;
  
Doherty, Declan Sept. 18, 2017, 10:24 a.m. UTC | #2
On 27/07/2017 1:00 PM, Tomasz Kulasek wrote:
> For fortville NIC bond_ethdev_8023ad_flow_verify fails when action queue
> index indicates unavailable queue before slaves configuration.
> 
> This fix verifies flow settings for queue 0, which is always available,
> and checks if slaves max queue number capacity meets requirements.
> 
> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
> 
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
...
> 

Acked-by: Declan Doherty <declan.doherty@intel.com>
  
Ferruh Yigit Sept. 18, 2017, 6:37 p.m. UTC | #3
On 9/18/2017 11:24 AM, Doherty, Declan wrote:
> On 27/07/2017 1:00 PM, Tomasz Kulasek wrote:
>> For fortville NIC bond_ethdev_8023ad_flow_verify fails when action queue
>> index indicates unavailable queue before slaves configuration.
>>
>> This fix verifies flow settings for queue 0, which is always available,
>> and checks if slaves max queue number capacity meets requirements.
>>
>> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
>>
>> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>

> Reviewed-by: Radu Nicolau <radu.nicolau@intel.com>

> Acked-by: Declan Doherty <declan.doherty@intel.com>

Applied to dpdk-next-net/master, thanks.

(Checkpatch warnings fixed while applying, can you please confirm the
commit in next-net)
  

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 8f9a860..ab5ebe0 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -175,12 +175,13 @@  const struct rte_flow_attr flow_attr_8023ad = {
 int
 bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
 		uint8_t slave_port) {
+	struct rte_eth_dev_info slave_info;
 	struct rte_flow_error error;
 	struct bond_dev_private *internals = (struct bond_dev_private *)
 			(bond_dev->data->dev_private);
 
-	struct rte_flow_action_queue lacp_queue_conf = {
-		.index = internals->mode4.dedicated_queues.rx_qid,
+	const struct rte_flow_action_queue lacp_queue_conf = {
+		.index = 0,
 	};
 
 	const struct rte_flow_action actions[] = {
@@ -195,8 +196,22 @@  bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
 
 	int ret = rte_flow_validate(slave_port, &flow_attr_8023ad,
 			flow_item_8023ad, actions, &error);
-	if (ret < 0)
+	if (ret < 0) {
+		RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_verify: %s "
+				"(slave_port=%d queue_id=%d)",
+				error.message, slave_port,
+				internals->mode4.dedicated_queues.rx_qid);
+		return -1;
+	}
+
+	rte_eth_dev_info_get(slave_port, &slave_info);
+	if ((slave_info.max_rx_queues < bond_dev->data->nb_rx_queues) ||
+			(slave_info.max_tx_queues < bond_dev->data->nb_tx_queues)) {
+		RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_verify: Slave %d"
+				" capabilities doesn't allow to allocate "
+				"additional queues", slave_port);
 		return -1;
+	}
 
 	return 0;
 }
@@ -206,7 +221,7 @@  bond_8023ad_slow_pkt_hw_filter_supported(uint8_t port_id) {
 	struct rte_eth_dev *bond_dev = &rte_eth_devices[port_id];
 	struct bond_dev_private *internals = (struct bond_dev_private *)
 			(bond_dev->data->dev_private);
-	struct rte_eth_dev_info bond_info, slave_info;
+	struct rte_eth_dev_info bond_info;
 	uint8_t idx;
 
 	/* Verify if all slaves in bonding supports flow director and */
@@ -217,9 +232,6 @@  bond_8023ad_slow_pkt_hw_filter_supported(uint8_t port_id) {
 		internals->mode4.dedicated_queues.tx_qid = bond_info.nb_tx_queues;
 
 		for (idx = 0; idx < internals->slave_count; idx++) {
-			rte_eth_dev_info_get(internals->slaves[idx].port_id,
-					&slave_info);
-
 			if (bond_ethdev_8023ad_flow_verify(bond_dev,
 					internals->slaves[idx].port_id) != 0)
 				return -1;