[dpdk-dev] [PATCH] net/mlx5: fix flow creation on port start

[dpdk-dev] [PATCH] net/mlx5: fix flow creation on port start

[Shahaf Shuler]

While the PMD avoids from creating hash RXQ with no hash fields and
array of queues after the port was allready started, it lacks such
protection when re-creating the flows after the port restarts.

This may lead to inconsist behaviour for flows depending if they were
created before or after the port start.

Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 5f49bf5ff..b0a196a4b 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2290,22 +2290,27 @@ priv_flow_start(struct priv *priv, struct mlx5_flows *list)
 			continue;
 		}
 		for (i = 0; i != hash_rxq_init_n; ++i) {
+			uint64_t hash_fields = hash_rxq_init[i].hash_fields;
 			if (!flow->frxq[i].ibv_attr)
 				continue;
 			flow->frxq[i].hrxq =
 				mlx5_priv_hrxq_get(priv, flow->rss_conf.rss_key,
 						   flow->rss_conf.rss_key_len,
-						   hash_rxq_init[i].hash_fields,
+						   hash_fields,
 						   (*flow->queues),
-						   flow->queues_n);
+						   hash_fields ?
+						   flow->queues_n :
+						   1);
 			if (flow->frxq[i].hrxq)
 				goto flow_create;
 			flow->frxq[i].hrxq =
 				mlx5_priv_hrxq_new(priv, flow->rss_conf.rss_key,
 						   flow->rss_conf.rss_key_len,
-						   hash_rxq_init[i].hash_fields,
+						   hash_fields,
 						   (*flow->queues),
-						   flow->queues_n);
+						   hash_fields ?
+						   flow->queues_n :
+						   1);
 			if (!flow->frxq[i].hrxq) {
 				DEBUG("Flow %p cannot be applied",
 				      (void *)flow);
-- 
2.12.0

Re: [dpdk-dev] [PATCH] net/mlx5: fix flow creation on port start

[Nélio Laranjeiro]

Hi Shahaf,

On Sun, Nov 05, 2017 at 05:44:49PM +0200, Shahaf Shuler wrote:
> While the PMD avoids from creating hash RXQ with no hash fields and
> array of queues after the port was allready started, it lacks such
> protection when re-creating the flows after the port restarts.
> 
> This may lead to inconsist behaviour for flows depending if they were
> created before or after the port start.
> 
> Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 5f49bf5ff..b0a196a4b 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2290,22 +2290,27 @@ priv_flow_start(struct priv *priv, struct mlx5_flows *list)
>  			continue;
>  		}
>  		for (i = 0; i != hash_rxq_init_n; ++i) {
> +			uint64_t hash_fields = hash_rxq_init[i].hash_fields;
>  			if (!flow->frxq[i].ibv_attr)
>  				continue;
>  			flow->frxq[i].hrxq =
>  				mlx5_priv_hrxq_get(priv, flow->rss_conf.rss_key,
>  						   flow->rss_conf.rss_key_len,
> -						   hash_rxq_init[i].hash_fields,
> +						   hash_fields,
>  						   (*flow->queues),
> -						   flow->queues_n);
> +						   hash_fields ?
> +						   flow->queues_n :
> +						   1);
>  			if (flow->frxq[i].hrxq)
>  				goto flow_create;
>  			flow->frxq[i].hrxq =
>  				mlx5_priv_hrxq_new(priv, flow->rss_conf.rss_key,
>  						   flow->rss_conf.rss_key_len,
> -						   hash_rxq_init[i].hash_fields,
> +						   hash_fields,
>  						   (*flow->queues),
> -						   flow->queues_n);
> +						   hash_fields ?
> +						   flow->queues_n :
> +						   1);
>  			if (!flow->frxq[i].hrxq) {
>  				DEBUG("Flow %p cannot be applied",
>  				      (void *)flow);
> -- 
> 2.12.0
 
I would suggest to modify directly the mlx5_priv_hrxq_get() and
mlx5_priv_hrxq_new() to handle this and update the comment of both
functions, as the two places where those are called are tricking the
queue numbers the same way, it can be directly handled by the function
itself.

Thanks,

-- 
Nélio Laranjeiro
6WIND

[dpdk-dev] [PATCH v2] net/mlx5: fix flow creation on port start

[Shahaf Shuler]

While the PMD avoids from creating hash RXQ with no hash fields and
array of queues after the port was allready started, it lacks such
protection when re-creating the flows after the port restarts.

This may lead to inconsist behaviour for flows depending if they were
created before or after the port start.

Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
On v2:
 - Fixing the issue from the hrxq_get and hrxq_new functions.
---
 drivers/net/mlx5/mlx5_flow.c | 4 ++--
 drivers/net/mlx5/mlx5_rxq.c  | 8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 5f49bf5ff..bcb06f84a 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -1781,7 +1781,7 @@ priv_flow_create_action_queue_rss(struct priv *priv,
 					   parser->rss_conf.rss_key_len,
 					   hash_fields,
 					   parser->queues,
-					   hash_fields ? parser->queues_n : 1);
+					   parser->queues_n);
 		if (flow->frxq[i].hrxq)
 			continue;
 		flow->frxq[i].hrxq =
@@ -1790,7 +1790,7 @@ priv_flow_create_action_queue_rss(struct priv *priv,
 					   parser->rss_conf.rss_key_len,
 					   hash_fields,
 					   parser->queues,
-					   hash_fields ? parser->queues_n : 1);
+					   parser->queues_n);
 		if (!flow->frxq[i].hrxq) {
 			rte_flow_error_set(error, ENOMEM,
 					   RTE_FLOW_ERROR_TYPE_HANDLE,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index a1f382b1f..18c13b3c7 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1265,7 +1265,8 @@ mlx5_priv_ind_table_ibv_verify(struct priv *priv)
  * @param hash_fields
  *   Verbs protocol hash field to make the RSS on.
  * @param queues
- *   Queues entering in hash queue.
+ *   Queues entering in hash queue. In case of empty hash_fields only the
+ *   first queue index will be taken for the indirection table.
  * @param queues_n
  *   Number of queues.
  *
@@ -1280,6 +1281,7 @@ mlx5_priv_hrxq_new(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
 	struct mlx5_ind_table_ibv *ind_tbl;
 	struct ibv_qp *qp;
 
+	queues_n = hash_fields ? queues_n : 1;
 	ind_tbl = mlx5_priv_ind_table_ibv_get(priv, queues, queues_n);
 	if (!ind_tbl)
 		ind_tbl = mlx5_priv_ind_table_ibv_new(priv, queues, queues_n);
@@ -1332,7 +1334,8 @@ mlx5_priv_hrxq_new(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
  * @param rss_conf
  *   RSS configuration for the Rx hash queue.
  * @param queues
- *   Queues entering in hash queue.
+ *   Queues entering in hash queue. In case of empty hash_fields only the
+ *   first queue index will be taken for the indirection table.
  * @param queues_n
  *   Number of queues.
  *
@@ -1345,6 +1348,7 @@ mlx5_priv_hrxq_get(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
 {
 	struct mlx5_hrxq *hrxq;
 
+	queues_n = hash_fields ? queues_n : 1;
 	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
 		struct mlx5_ind_table_ibv *ind_tbl;
 
-- 
2.12.0

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix flow creation on port start

[Nélio Laranjeiro]

On Mon, Nov 06, 2017 at 04:00:25PM +0200, Shahaf Shuler wrote:
> While the PMD avoids from creating hash RXQ with no hash fields and
> array of queues after the port was allready started, it lacks such
> protection when re-creating the flows after the port restarts.
> 
> This may lead to inconsist behaviour for flows depending if they were
> created before or after the port start.
> 
> Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> On v2:
>  - Fixing the issue from the hrxq_get and hrxq_new functions.
> ---
>  drivers/net/mlx5/mlx5_flow.c | 4 ++--
>  drivers/net/mlx5/mlx5_rxq.c  | 8 ++++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 5f49bf5ff..bcb06f84a 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1781,7 +1781,7 @@ priv_flow_create_action_queue_rss(struct priv *priv,
>  					   parser->rss_conf.rss_key_len,
>  					   hash_fields,
>  					   parser->queues,
> -					   hash_fields ? parser->queues_n : 1);
> +					   parser->queues_n);
>  		if (flow->frxq[i].hrxq)
>  			continue;
>  		flow->frxq[i].hrxq =
> @@ -1790,7 +1790,7 @@ priv_flow_create_action_queue_rss(struct priv *priv,
>  					   parser->rss_conf.rss_key_len,
>  					   hash_fields,
>  					   parser->queues,
> -					   hash_fields ? parser->queues_n : 1);
> +					   parser->queues_n);
>  		if (!flow->frxq[i].hrxq) {
>  			rte_flow_error_set(error, ENOMEM,
>  					   RTE_FLOW_ERROR_TYPE_HANDLE,
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index a1f382b1f..18c13b3c7 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1265,7 +1265,8 @@ mlx5_priv_ind_table_ibv_verify(struct priv *priv)
>   * @param hash_fields
>   *   Verbs protocol hash field to make the RSS on.
>   * @param queues
> - *   Queues entering in hash queue.
> + *   Queues entering in hash queue. In case of empty hash_fields only the
> + *   first queue index will be taken for the indirection table.
>   * @param queues_n
>   *   Number of queues.
>   *
> @@ -1280,6 +1281,7 @@ mlx5_priv_hrxq_new(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
>  	struct mlx5_ind_table_ibv *ind_tbl;
>  	struct ibv_qp *qp;
>  
> +	queues_n = hash_fields ? queues_n : 1;
>  	ind_tbl = mlx5_priv_ind_table_ibv_get(priv, queues, queues_n);
>  	if (!ind_tbl)
>  		ind_tbl = mlx5_priv_ind_table_ibv_new(priv, queues, queues_n);
> @@ -1332,7 +1334,8 @@ mlx5_priv_hrxq_new(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
>   * @param rss_conf
>   *   RSS configuration for the Rx hash queue.
>   * @param queues
> - *   Queues entering in hash queue.
> + *   Queues entering in hash queue. In case of empty hash_fields only the
> + *   first queue index will be taken for the indirection table.
>   * @param queues_n
>   *   Number of queues.
>   *
> @@ -1345,6 +1348,7 @@ mlx5_priv_hrxq_get(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
>  {
>  	struct mlx5_hrxq *hrxq;
>  
> +	queues_n = hash_fields ? queues_n : 1;
>  	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
>  		struct mlx5_ind_table_ibv *ind_tbl;
>  
> -- 
> 2.12.0
 

Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix flow creation on port start

[Ferruh Yigit]

On 11/6/2017 6:32 AM, Nélio Laranjeiro wrote:
> On Mon, Nov 06, 2017 at 04:00:25PM +0200, Shahaf Shuler wrote:
>> While the PMD avoids from creating hash RXQ with no hash fields and
>> array of queues after the port was allready started, it lacks such
>> protection when re-creating the flows after the port restarts.
>>
>> This may lead to inconsist behaviour for flows depending if they were
>> created before or after the port start.
>>
>> Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
>> Cc: nelio.laranjeiro@6wind.com
>>
>> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

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