[dpdk-dev] net/mlx5: fix adjust priority for drop queue

  Drop queue should also adjust their priority according the most specific
layer in the pattern they are matching to avoid dropping all the traffic.

Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

On Tue, Oct 24, 2017 at 01:33:19PM +0200, Nelio Laranjeiro wrote:
> Drop queue should also adjust their priority according the most specific
> layer in the pattern they are matching to avoid dropping all the traffic.
> 
> Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 26cf593af..549ae6916 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1185,6 +1185,14 @@ priv_flow_convert(struct priv *priv,
>  		parser->queue[HASH_RXQ_ETH].ibv_attr->priority =
>  			attr->priority +
>  			hash_rxq_init[parser->layer].flow_priority;
> +	} else if (parser->drop) {

Isn't this condition redundant?

And the following could be more consistent (and readable) with the similar code
above. But I'd leave it as your choice.

	if (parser->drop) {
	...
	} else if (parser->queues_n == 1) {
	...
	} else {
	...
	}

> +		/*
> +		 * Drop queue priority needs to be adjusted to
> +		 * their most specific layer priority.
> +		 */
> +		parser->drop_q.ibv_attr->priority =
> +			attr->priority +
> +			hash_rxq_init[parser->layer].flow_priority;
>  	}
>  exit_free:
>  	/* Only verification is expected, all resources should be released. */
> -- 


Thanks,
Yongseok
  

On Tue, Oct 24, 2017 at 10:55:03AM -0700, Yongseok Koh wrote:
> On Tue, Oct 24, 2017 at 01:33:19PM +0200, Nelio Laranjeiro wrote:
> > Drop queue should also adjust their priority according the most specific
> > layer in the pattern they are matching to avoid dropping all the traffic.
> > 
> > Fixes: 8086cf08b2f0 ("net/mlx5: handle RSS hash configuration in RSS flow")
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> > index 26cf593af..549ae6916 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -1185,6 +1185,14 @@ priv_flow_convert(struct priv *priv,
> >  		parser->queue[HASH_RXQ_ETH].ibv_attr->priority =
> >  			attr->priority +
> >  			hash_rxq_init[parser->layer].flow_priority;
> > +	} else if (parser->drop) {
> 
> Isn't this condition redundant?

I reach a point that all this "drop" is redundant and could use the
Ethernet entry to store it, but in RC phases making a such huge change
is not easy.

> And the following could be more consistent (and readable) with the similar code
> above. But I'd leave it as your choice.
> 
> 	if (parser->drop) {
> 	...
> 	} else if (parser->queues_n == 1) {
> 	...
> 	} else {
> 	...
> 	}

Agreed.

Thanks,