[dpdk-dev] net/mlx5: change tunnel flow priority

  Packet matching inner and outer flow rules are catch by the first one
added in the device as both flows are configured with the same priority.
To avoid such situation, the inner flow can have an higher priority than
the outer ones as their pattern matching will not collide.

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

A few commit log rewording suggestions for clarity and a couple of
additional comments below, otherwise patch looks fine.

On Thu, Feb 15, 2018 at 10:23:29AM +0100, Nelio Laranjeiro wrote:
> Packet matching inner and outer flow rules are catch by the first one

catch => caught

> added in the device as both flows are configured with the same priority.
> To avoid such situation, the inner flow can have an higher priority than

in the device => to the device
can => must?

> the outer ones as their pattern matching will not collide.

not => otherwise

> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 72 +++++++++++++++++++++++++++-----------------
>  1 file changed, 45 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 26002c4b9..6223f1f94 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -118,7 +118,7 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_SRC_PORT_TCP |
>  				IBV_RX_HASH_DST_PORT_TCP),
>  		.dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,
> -		.flow_priority = 0,
> +		.flow_priority = 1,
>  		.ip_version = MLX5_IPV4,
>  	},
>  	[HASH_RXQ_UDPV4] = {
> @@ -127,7 +127,7 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_SRC_PORT_UDP |
>  				IBV_RX_HASH_DST_PORT_UDP),
>  		.dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_UDP,
> -		.flow_priority = 0,
> +		.flow_priority = 1,
>  		.ip_version = MLX5_IPV4,
>  	},
>  	[HASH_RXQ_IPV4] = {
> @@ -135,7 +135,7 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_DST_IPV4),
>  		.dpdk_rss_hf = (ETH_RSS_IPV4 |
>  				ETH_RSS_FRAG_IPV4),
> -		.flow_priority = 1,
> +		.flow_priority = 2,
>  		.ip_version = MLX5_IPV4,
>  	},
>  	[HASH_RXQ_TCPV6] = {
> @@ -144,7 +144,7 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_SRC_PORT_TCP |
>  				IBV_RX_HASH_DST_PORT_TCP),
>  		.dpdk_rss_hf = ETH_RSS_NONFRAG_IPV6_TCP,
> -		.flow_priority = 0,
> +		.flow_priority = 1,
>  		.ip_version = MLX5_IPV6,
>  	},
>  	[HASH_RXQ_UDPV6] = {
> @@ -153,7 +153,7 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_SRC_PORT_UDP |
>  				IBV_RX_HASH_DST_PORT_UDP),
>  		.dpdk_rss_hf = ETH_RSS_NONFRAG_IPV6_UDP,
> -		.flow_priority = 0,
> +		.flow_priority = 1,
>  		.ip_version = MLX5_IPV6,
>  	},
>  	[HASH_RXQ_IPV6] = {
> @@ -161,13 +161,13 @@ const struct hash_rxq_init hash_rxq_init[] = {
>  				IBV_RX_HASH_DST_IPV6),
>  		.dpdk_rss_hf = (ETH_RSS_IPV6 |
>  				ETH_RSS_FRAG_IPV6),
> -		.flow_priority = 1,
> +		.flow_priority = 2,
>  		.ip_version = MLX5_IPV6,
>  	},
>  	[HASH_RXQ_ETH] = {
>  		.hash_fields = 0,
>  		.dpdk_rss_hf = 0,
> -		.flow_priority = 2,
> +		.flow_priority = 3,
>  	},
>  };
>  
> @@ -860,8 +860,6 @@ priv_flow_convert_items_validate(struct priv *priv,
>   *
>   * @param priv
>   *   Pointer to private structure.
> - * @param[in] priority
> - *   Flow priority.
>   * @param[in] size
>   *   Amount of byte to allocate.
>   * @param[out] error
> @@ -872,7 +870,6 @@ priv_flow_convert_items_validate(struct priv *priv,
>   */
>  static struct ibv_flow_attr*
>  priv_flow_convert_allocate(struct priv *priv,
> -			   unsigned int priority,
>  			   unsigned int size,
>  			   struct rte_flow_error *error)
>  {
> @@ -887,11 +884,44 @@ priv_flow_convert_allocate(struct priv *priv,
>  				   "cannot allocate verbs spec attributes.");
>  		return NULL;
>  	}
> -	ibv_attr->priority = priority;
>  	return ibv_attr;
>  }
>  
>  /**
> + * Make Inner packet matching with an higher priority from the non Inner
> + * matching.

Inner => inner

> + *
> + * @param priv
> + *   Pointer to private structure.
> + * @param[in, out] parser
> + *   Internal parser structure.
> + * @param attr
> + *   User flow attribute.
> + */
> +static void
> +priv_flow_update_priority(struct priv *priv, struct mlx5_flow_parse *parser,
> +			  const struct rte_flow_attr *attr)
> +{
> +	unsigned int i;
> +
> +	(void)priv;

I suggest dropping "priv" from everywhere in this function including its
name. It's a static helper not even exposed, no need to be consistent with
other priv* functions for the sake of consistency if the parameter is
unnecessary.

> +	if (parser->drop) {
> +		parser->queue[HASH_RXQ_ETH].ibv_attr->priority =
> +			attr->priority +
> +			hash_rxq_init[HASH_RXQ_ETH].flow_priority;
> +		return;
> +	}
> +	for (i = 0; i != hash_rxq_init_n; ++i) {
> +		if (parser->queue[i].ibv_attr) {
> +			parser->queue[i].ibv_attr->priority =
> +				attr->priority +
> +				hash_rxq_init[i].flow_priority -
> +				(parser->inner ? 1 : 0);
> +		}
> +	}
> +}

On a personal note, I noticed how complex (convoluted?) mlx5 flow rule
handling code turned lately due to all the added features, unforeseen issues
and the resulting workarounds, partly caused by the limited number of
available priorities on the Verbs side. I think we're nearing another major
rewrite.

> +
> +/**
>   * Finalise verbs flow attributes.
>   *
>   * @param priv
> @@ -1059,23 +1089,16 @@ priv_flow_convert(struct priv *priv,
>  	 * Allocate the memory space to store verbs specifications.
>  	 */
>  	if (parser->drop) {
> -		unsigned int priority =
> -			attr->priority +
> -			hash_rxq_init[HASH_RXQ_ETH].flow_priority;
>  		unsigned int offset = parser->queue[HASH_RXQ_ETH].offset;
>  
>  		parser->queue[HASH_RXQ_ETH].ibv_attr =
> -			priv_flow_convert_allocate(priv, priority,
> -						   offset, error);
> +			priv_flow_convert_allocate(priv, offset, error);
>  		if (!parser->queue[HASH_RXQ_ETH].ibv_attr)
>  			return ENOMEM;
>  		parser->queue[HASH_RXQ_ETH].offset =
>  			sizeof(struct ibv_flow_attr);
>  	} else {
>  		for (i = 0; i != hash_rxq_init_n; ++i) {
> -			unsigned int priority =
> -				attr->priority +
> -				hash_rxq_init[i].flow_priority;
>  			unsigned int offset;
>  
>  			if (!(parser->rss_conf.rss_hf &
> @@ -1084,8 +1107,7 @@ priv_flow_convert(struct priv *priv,
>  				continue;
>  			offset = parser->queue[i].offset;
>  			parser->queue[i].ibv_attr =
> -				priv_flow_convert_allocate(priv, priority,
> -							   offset, error);
> +				priv_flow_convert_allocate(priv, offset, error);
>  			if (!parser->queue[i].ibv_attr)
>  				goto exit_enomem;
>  			parser->queue[i].offset = sizeof(struct ibv_flow_attr);
> @@ -1120,13 +1142,9 @@ priv_flow_convert(struct priv *priv,
>  	 * Last step. Complete missing specification to reach the RSS
>  	 * configuration.
>  	 */
> -	if (!parser->drop) {
> +	if (!parser->drop)
>  		priv_flow_convert_finalise(priv, parser);
> -	} else {
> -		parser->queue[HASH_RXQ_ETH].ibv_attr->priority =
> -			attr->priority +
> -			hash_rxq_init[parser->layer].flow_priority;
> -	}
> +	priv_flow_update_priority(priv, parser, attr);
>  exit_free:
>  	/* Only verification is expected, all resources should be released. */
>  	if (!parser->create) {
> -- 
> 2.11.0
>
  

This series apply on top of [1]

Changes in v2:
 - rebase on top of series [2] which drops priv locks.

[1] https://dpdk.org/dev/patchwork/patch/36058/
[2] https://dpdk.org/dev/patchwork/patch/35650/

Nelio Laranjeiro (4):
  net/mlx5: change tunnel flow priority
  net/mlx5: fix flow single queue
  net/mlx5: improve flow error explanation
  net/mlx5: refuse empty VLAN flow specification

 drivers/net/mlx5/mlx5_flow.c | 164 +++++++++++++++++++++++++++----------------
 1 file changed, 102 insertions(+), 62 deletions(-)
  

Tuesday, March 13, 2018 4:18 PM, Nelio Laranjeiro:
> This series apply on top of [1]
> 
> Changes in v2:
>  - rebase on top of series [2] which drops priv locks.
> 
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdp
> dk.org%2Fdev%2Fpatchwork%2Fpatch%2F36058%2F&data=02%7C01%7Csha
> hafs%40mellanox.com%7C6258651c419f49d449e808d588ed5ce6%7Ca652971c
> 7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636565475410509385&sdata=Y4sR
> XTP1eW42hNx75KcZ1eA8hG4MC4vwIKbvWrE1whs%3D&reserved=0
> [2]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdp
> dk.org%2Fdev%2Fpatchwork%2Fpatch%2F35650%2F&data=02%7C01%7Csha
> hafs%40mellanox.com%7C6258651c419f49d449e808d588ed5ce6%7Ca652971c
> 7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636565475410509385&sdata=4IT
> MZRuizi25TN3f6EFv970RgZj5CzKN5CcvY21PsOE%3D&reserved=0
> 
> Nelio Laranjeiro (4):
>   net/mlx5: change tunnel flow priority
>   net/mlx5: fix flow single queue
>   net/mlx5: improve flow error explanation
>   net/mlx5: refuse empty VLAN flow specification
> 
>  drivers/net/mlx5/mlx5_flow.c | 164 +++++++++++++++++++++++++++------
> ----------
>  1 file changed, 102 insertions(+), 62 deletions(-)

Series applied to next-net-mlx, thanks. 

> 
> --
> 2.11.0