[dpdk-dev,v1] net/mlx4: report on supported RSS hash functions

Message ID 1525794195-25082-1-git-send-email-ophirmu@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers

Checks

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

Commit Message

Ophir Munk May 8, 2018, 3:43 p.m. UTC
  Report on mlx4 supported RSS functions as part of dev_infos_get
callback.
Previous to this commit RSS support was reported as none. Since the
introduction of [1] it is required that all RSS configurations will be
verified.

[1] commit 8863a1fbfc66 ("ethdev: add supported hash function check")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/mlx4/mlx4_ethdev.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
  

Comments

Shahaf Shuler May 9, 2018, 9:38 a.m. UTC | #1
Hi Ophir,

Tuesday, May 8, 2018 6:43 PM, Ophir Munk:
> Subject: [PATCH v1] net/mlx4: report on supported RSS hash functions
> 
> Report on mlx4 supported RSS functions as part of dev_infos_get callback.
> Previous to this commit RSS support was reported as none. Since the
> introduction of [1] it is required that all RSS configurations will be verified.
> 
> [1] commit 8863a1fbfc66 ("ethdev: add supported hash function check")
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
>  drivers/net/mlx4/mlx4_ethdev.c | 51
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..2a1533c 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -545,6 +545,55 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr)  }
> 
>  /**
> + * Convert verbs RSS types to their DPDK equivalents.
> + *
> + * This function returns a group of RSS dpdk types given their
> +equivalent group
> + * of verbs types.
> + * For example both source IPv4 and destination IPv4 verbs types are
> +converted
> + * into their equivalent RSS group types. If each of these verbs types
> +existed
> + * exclusively - no conversion would take place.
> + *
> + * @param types
> + *   RSS hash types in verbs format
> + *
> + * @return
> + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
> + */
> +static uint64_t
> +mlx4_ibv_to_dpdk_rss_types(uint64_t types) {
> +	enum { IPV4, IPV6, TCP, UDP, };
> +	const uint64_t in[] = {
> +		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> IBV_RX_HASH_DST_IPV4,
> +		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> IBV_RX_HASH_DST_IPV6,
> +		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> IBV_RX_HASH_DST_PORT_TCP,
> +		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> IBV_RX_HASH_DST_PORT_UDP,
> +	};
> +	const uint64_t out[RTE_DIM(in)] = {
> +		[IPV4] = (ETH_RSS_IPV4 |
> +			  ETH_RSS_FRAG_IPV4 |
> +			  ETH_RSS_NONFRAG_IPV4_OTHER),
> +		[IPV6] = (ETH_RSS_IPV6 |
> +			  ETH_RSS_FRAG_IPV6 |
> +			  ETH_RSS_NONFRAG_IPV6_OTHER |
> +			  ETH_RSS_IPV6_EX),
> +		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> +			 ETH_RSS_NONFRAG_IPV6_TCP |
> +			 ETH_RSS_IPV6_TCP_EX),
> +		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> +			 ETH_RSS_NONFRAG_IPV6_UDP |
> +			 ETH_RSS_IPV6_UDP_EX),
> +	};

Since above are constants, why not defining a global array of structs containing the ibv_hash and the equivalent dpdk_hash, instead of recreating it for each call? 
There is a similar concept on mlx5_flow.c:

/* Initialization data for hash RX queues. */              
 const struct hash_rxq_init hash_rxq_init[] = {             
         [HASH_RXQ_TCPV4] = {                               
                 .hash_fields = (IBV_RX_HASH_SRC_IPV4 |     
                                 IBV_RX_HASH_DST_IPV4 |     
                                 IBV_RX_HASH_SRC_PORT_TCP | 
                                 IBV_RX_HASH_DST_PORT_TCP), 
                 .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,   
                 .flow_priority = 0,                        
                 .ip_version = MLX5_IPV4,                   
         },                                                 

> +	uint64_t conv = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i != RTE_DIM(in); ++i)
> +		if ((types & in[i]) == in[i])
> +			conv |= out[i];
> +	return conv;
> +}
> +
> +/**
>   * DPDK callback to get information about the device.
>   *
>   * @param dev
> @@ -587,6 +636,8 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  			ETH_LINK_SPEED_20G |
>  			ETH_LINK_SPEED_40G |
>  			ETH_LINK_SPEED_56G;
> +	info->flow_type_rss_offloads = mlx4_ibv_to_dpdk_rss_types(
> +			priv->hw_rss_sup);
>  }
> 
>  /**
> --
> 2.7.4
  
Ophir Munk May 9, 2018, 11:54 a.m. UTC | #2
Hi Shahaf,

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Wednesday, May 09, 2018 12:39 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash functions
> 
> Hi Ophir,
> 
> Tuesday, May 8, 2018 6:43 PM, Ophir Munk:
> > Subject: [PATCH v1] net/mlx4: report on supported RSS hash functions
> >
> > Report on mlx4 supported RSS functions as part of dev_infos_get callback.
> > Previous to this commit RSS support was reported as none. Since the
> > introduction of [1] it is required that all RSS configurations will be verified.
> >
> > [1] commit 8863a1fbfc66 ("ethdev: add supported hash function check")
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> >  drivers/net/mlx4/mlx4_ethdev.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> > b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..2a1533c 100644
> > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > @@ -545,6 +545,55 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev,
> struct
> > ether_addr *mac_addr)  }
> >
> >  /**
> > + * Convert verbs RSS types to their DPDK equivalents.
> > + *
> > + * This function returns a group of RSS dpdk types given their
> > +equivalent group
> > + * of verbs types.
> > + * For example both source IPv4 and destination IPv4 verbs types are
> > +converted
> > + * into their equivalent RSS group types. If each of these verbs
> > +types existed
> > + * exclusively - no conversion would take place.
> > + *
> > + * @param types
> > + *   RSS hash types in verbs format
> > + *
> > + * @return
> > + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
> > + */
> > +static uint64_t
> > +mlx4_ibv_to_dpdk_rss_types(uint64_t types) {
> > +	enum { IPV4, IPV6, TCP, UDP, };
> > +	const uint64_t in[] = {
> > +		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> > IBV_RX_HASH_DST_IPV4,
> > +		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> > IBV_RX_HASH_DST_IPV6,
> > +		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> > IBV_RX_HASH_DST_PORT_TCP,
> > +		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> > IBV_RX_HASH_DST_PORT_UDP,
> > +	};
> > +	const uint64_t out[RTE_DIM(in)] = {
> > +		[IPV4] = (ETH_RSS_IPV4 |
> > +			  ETH_RSS_FRAG_IPV4 |
> > +			  ETH_RSS_NONFRAG_IPV4_OTHER),
> > +		[IPV6] = (ETH_RSS_IPV6 |
> > +			  ETH_RSS_FRAG_IPV6 |
> > +			  ETH_RSS_NONFRAG_IPV6_OTHER |
> > +			  ETH_RSS_IPV6_EX),
> > +		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> > +			 ETH_RSS_NONFRAG_IPV6_TCP |
> > +			 ETH_RSS_IPV6_TCP_EX),
> > +		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> > +			 ETH_RSS_NONFRAG_IPV6_UDP |
> > +			 ETH_RSS_IPV6_UDP_EX),
> > +	};
> 
> Since above are constants, why not defining a global array of structs
> containing the ibv_hash and the equivalent dpdk_hash, instead of recreating
> it for each call?

The array is recreated because it should have been declared as "static const" rather than just "const". I prefer this fix.
Alternatively it could have been defined globally outside of the function as suggested which would have avoided recreation as well.
All of those claims are confirmed by inspecting the assembly code of the above alternatives.

> There is a similar concept on mlx5_flow.c:
> 
> /* Initialization data for hash RX queues. */
>  const struct hash_rxq_init hash_rxq_init[] = {
>          [HASH_RXQ_TCPV4] = {
>                  .hash_fields = (IBV_RX_HASH_SRC_IPV4 |
>                                  IBV_RX_HASH_DST_IPV4 |
>                                  IBV_RX_HASH_SRC_PORT_TCP |
>                                  IBV_RX_HASH_DST_PORT_TCP),
>                  .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,
>                  .flow_priority = 0,
>                  .ip_version = MLX5_IPV4,
>          },
> 

I was inspired from "the reversed" function in mlx4_flow.c which defines constants inside the function as well, see [1].
To fix the constants recreation both functions should be handled together.

I suggest:
1. Moving mlx4_ibv_to_dpdk_rss_types() function as is from mlx4_ether.c to mlx4_flow.c so it will be adjacent to mlx4_conv_rss_types() function.
2. Sending a new patch that avoids constants recreation in the 2 functions. 

[1]
uint64_t
mlx4_conv_rss_types(struct priv *priv, uint64_t types)
{
	enum { IPV4, IPV6, TCP, UDP, };
	const uint64_t in[] = {
		[IPV4] = (ETH_RSS_IPV4 |
			  ETH_RSS_FRAG_IPV4 |
			  ETH_RSS_NONFRAG_IPV4_TCP |
			  ETH_RSS_NONFRAG_IPV4_UDP |
			  ETH_RSS_NONFRAG_IPV4_OTHER),
		[IPV6] = (ETH_RSS_IPV6 |
			  ETH_RSS_FRAG_IPV6 |
			  ETH_RSS_NONFRAG_IPV6_TCP |
			  ETH_RSS_NONFRAG_IPV6_UDP |
			  ETH_RSS_NONFRAG_IPV6_OTHER |
			  ETH_RSS_IPV6_EX |
			  ETH_RSS_IPV6_TCP_EX |
			  ETH_RSS_IPV6_UDP_EX),
		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
			 ETH_RSS_NONFRAG_IPV6_TCP |
			 ETH_RSS_IPV6_TCP_EX),
		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
			 ETH_RSS_NONFRAG_IPV6_UDP |
			 ETH_RSS_IPV6_UDP_EX),
	};
	const uint64_t out[RTE_DIM(in)] = {
		[IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4,
		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6,
		[TCP] = IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP,
		[UDP] = IBV_RX_HASH_SRC_PORT_UDP | IBV_RX_HASH_DST_PORT_UDP,
	};
	uint64_t seen = 0;
	uint64_t conv = 0;
	unsigned int i;

	if (types == (uint64_t)-1)
		return priv->hw_rss_sup;
	for (i = 0; i != RTE_DIM(in); ++i)
		if (types & in[i]) {
			seen |= types & in[i];
			conv |= out[i];
		}
	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
		return conv;
	rte_errno = ENOTSUP;
	return (uint64_t)-1;
}

> > +	uint64_t conv = 0;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != RTE_DIM(in); ++i)
> > +		if ((types & in[i]) == in[i])
> > +			conv |= out[i];
> > +	return conv;
> > +}
> > +
> > +/**
> >   * DPDK callback to get information about the device.
> >   *
> >   * @param dev
> > @@ -587,6 +636,8 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev,
> struct
> > rte_eth_dev_info *info)
> >  			ETH_LINK_SPEED_20G |
> >  			ETH_LINK_SPEED_40G |
> >  			ETH_LINK_SPEED_56G;
> > +	info->flow_type_rss_offloads = mlx4_ibv_to_dpdk_rss_types(
> > +			priv->hw_rss_sup);
> >  }
> >
> >  /**
> > --
> > 2.7.4
  
Shahaf Shuler May 9, 2018, 2:01 p.m. UTC | #3
Wednesday, May 9, 2018 2:54 PM, Ophir Munk:
> Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash functions
> 
> Hi Shahaf,
> 
> > -----Original Message-----
> > From: Shahaf Shuler
> > Sent: Wednesday, May 09, 2018 12:39 PM
> > To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Adrien
> Mazarguil
> > <adrien.mazarguil@6wind.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > <olgas@mellanox.com>
> > Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash
> > functions
> >
> > Hi Ophir,
> >
> > Tuesday, May 8, 2018 6:43 PM, Ophir Munk:
> > > Subject: [PATCH v1] net/mlx4: report on supported RSS hash functions
> > >
> > > Report on mlx4 supported RSS functions as part of dev_infos_get
> callback.
> > > Previous to this commit RSS support was reported as none. Since the
> > > introduction of [1] it is required that all RSS configurations will be verified.
> > >
> > > [1] commit 8863a1fbfc66 ("ethdev: add supported hash function
> > > check")
> > >
> > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > > ---
> > >  drivers/net/mlx4/mlx4_ethdev.c | 51
> > > ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >
> > > diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> > > b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..2a1533c 100644
> > > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > > @@ -545,6 +545,55 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev,
> > struct
> > > ether_addr *mac_addr)  }
> > >
> > >  /**
> > > + * Convert verbs RSS types to their DPDK equivalents.
> > > + *
> > > + * This function returns a group of RSS dpdk types given their
> > > +equivalent group
> > > + * of verbs types.
> > > + * For example both source IPv4 and destination IPv4 verbs types
> > > +are converted
> > > + * into their equivalent RSS group types. If each of these verbs
> > > +types existed
> > > + * exclusively - no conversion would take place.
> > > + *
> > > + * @param types
> > > + *   RSS hash types in verbs format
> > > + *
> > > + * @return
> > > + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
> > > + */
> > > +static uint64_t
> > > +mlx4_ibv_to_dpdk_rss_types(uint64_t types) {
> > > +	enum { IPV4, IPV6, TCP, UDP, };
> > > +	const uint64_t in[] = {
> > > +		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> > > IBV_RX_HASH_DST_IPV4,
> > > +		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> > > IBV_RX_HASH_DST_IPV6,
> > > +		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> > > IBV_RX_HASH_DST_PORT_TCP,
> > > +		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> > > IBV_RX_HASH_DST_PORT_UDP,
> > > +	};
> > > +	const uint64_t out[RTE_DIM(in)] = {
> > > +		[IPV4] = (ETH_RSS_IPV4 |
> > > +			  ETH_RSS_FRAG_IPV4 |
> > > +			  ETH_RSS_NONFRAG_IPV4_OTHER),
> > > +		[IPV6] = (ETH_RSS_IPV6 |
> > > +			  ETH_RSS_FRAG_IPV6 |
> > > +			  ETH_RSS_NONFRAG_IPV6_OTHER |
> > > +			  ETH_RSS_IPV6_EX),
> > > +		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> > > +			 ETH_RSS_NONFRAG_IPV6_TCP |
> > > +			 ETH_RSS_IPV6_TCP_EX),
> > > +		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> > > +			 ETH_RSS_NONFRAG_IPV6_UDP |
> > > +			 ETH_RSS_IPV6_UDP_EX),
> > > +	};
> >
> > Since above are constants, why not defining a global array of structs
> > containing the ibv_hash and the equivalent dpdk_hash, instead of
> > recreating it for each call?
> 
> The array is recreated because it should have been declared as "static const"
> rather than just "const". I prefer this fix.
> Alternatively it could have been defined globally outside of the function as
> suggested which would have avoided recreation as well.
> All of those claims are confirmed by inspecting the assembly code of the
> above alternatives.
> 
> > There is a similar concept on mlx5_flow.c:
> >
> > /* Initialization data for hash RX queues. */  const struct
> > hash_rxq_init hash_rxq_init[] = {
> >          [HASH_RXQ_TCPV4] = {
> >                  .hash_fields = (IBV_RX_HASH_SRC_IPV4 |
> >                                  IBV_RX_HASH_DST_IPV4 |
> >                                  IBV_RX_HASH_SRC_PORT_TCP |
> >                                  IBV_RX_HASH_DST_PORT_TCP),
> >                  .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,
> >                  .flow_priority = 0,
> >                  .ip_version = MLX5_IPV4,
> >          },
> >
> 
> I was inspired from "the reversed" function in mlx4_flow.c which defines
> constants inside the function as well, see [1].
> To fix the constants recreation both functions should be handled together.
> 
> I suggest:
> 1. Moving mlx4_ibv_to_dpdk_rss_types() function as is from mlx4_ether.c to
> mlx4_flow.c so it will be adjacent to mlx4_conv_rss_types() function.
> 2. Sending a new patch that avoids constants recreation in the 2 functions.

OK. Those can be 2 patches from the same series. 

> 
> [1]
> uint64_t
> mlx4_conv_rss_types(struct priv *priv, uint64_t types) {
> 	enum { IPV4, IPV6, TCP, UDP, };
> 	const uint64_t in[] = {
> 		[IPV4] = (ETH_RSS_IPV4 |
> 			  ETH_RSS_FRAG_IPV4 |
> 			  ETH_RSS_NONFRAG_IPV4_TCP |
> 			  ETH_RSS_NONFRAG_IPV4_UDP |
> 			  ETH_RSS_NONFRAG_IPV4_OTHER),
> 		[IPV6] = (ETH_RSS_IPV6 |
> 			  ETH_RSS_FRAG_IPV6 |
> 			  ETH_RSS_NONFRAG_IPV6_TCP |
> 			  ETH_RSS_NONFRAG_IPV6_UDP |
> 			  ETH_RSS_NONFRAG_IPV6_OTHER |
> 			  ETH_RSS_IPV6_EX |
> 			  ETH_RSS_IPV6_TCP_EX |
> 			  ETH_RSS_IPV6_UDP_EX),
> 		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> 			 ETH_RSS_NONFRAG_IPV6_TCP |
> 			 ETH_RSS_IPV6_TCP_EX),
> 		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> 			 ETH_RSS_NONFRAG_IPV6_UDP |
> 			 ETH_RSS_IPV6_UDP_EX),
> 	};
> 	const uint64_t out[RTE_DIM(in)] = {
> 		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> IBV_RX_HASH_DST_IPV4,
> 		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> IBV_RX_HASH_DST_IPV6,
> 		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> IBV_RX_HASH_DST_PORT_TCP,
> 		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> IBV_RX_HASH_DST_PORT_UDP,
> 	};
> 	uint64_t seen = 0;
> 	uint64_t conv = 0;
> 	unsigned int i;
> 
> 	if (types == (uint64_t)-1)
> 		return priv->hw_rss_sup;
> 	for (i = 0; i != RTE_DIM(in); ++i)
> 		if (types & in[i]) {
> 			seen |= types & in[i];
> 			conv |= out[i];
> 		}
> 	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
> 		return conv;
> 	rte_errno = ENOTSUP;
> 	return (uint64_t)-1;
> }
> 
> > > +	uint64_t conv = 0;
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i != RTE_DIM(in); ++i)
> > > +		if ((types & in[i]) == in[i])
> > > +			conv |= out[i];
> > > +	return conv;
> > > +}
> > > +
> > > +/**
> > >   * DPDK callback to get information about the device.
> > >   *
> > >   * @param dev
> > > @@ -587,6 +636,8 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev,
> > struct
> > > rte_eth_dev_info *info)
> > >  			ETH_LINK_SPEED_20G |
> > >  			ETH_LINK_SPEED_40G |
> > >  			ETH_LINK_SPEED_56G;
> > > +	info->flow_type_rss_offloads = mlx4_ibv_to_dpdk_rss_types(
> > > +			priv->hw_rss_sup);
> > >  }
> > >
> > >  /**
> > > --
> > > 2.7.4
  
Ophir Munk May 9, 2018, 10:42 p.m. UTC | #4
Hi,
PATCH v2 was sent

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Wednesday, May 09, 2018 5:01 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash functions
> 
> Wednesday, May 9, 2018 2:54 PM, Ophir Munk:
> > Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash
> > functions
> >
> > Hi Shahaf,
> >
> > > -----Original Message-----
> > > From: Shahaf Shuler
> > > Sent: Wednesday, May 09, 2018 12:39 PM
> > > To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Adrien
> > Mazarguil
> > > <adrien.mazarguil@6wind.com>
> > > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > > <olgas@mellanox.com>
> > > Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash
> > > functions
> > >
> > > Hi Ophir,
> > >
> > > Tuesday, May 8, 2018 6:43 PM, Ophir Munk:
> > > > Subject: [PATCH v1] net/mlx4: report on supported RSS hash
> > > > functions
> > > >
> > > > Report on mlx4 supported RSS functions as part of dev_infos_get
> > callback.
> > > > Previous to this commit RSS support was reported as none. Since
> > > > the introduction of [1] it is required that all RSS configurations will be
> verified.
> > > >
> > > > [1] commit 8863a1fbfc66 ("ethdev: add supported hash function
> > > > check")
> > > >
> > > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > > > ---
> > > >  drivers/net/mlx4/mlx4_ethdev.c | 51
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 51 insertions(+)
> > > >
> > > > diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> > > > b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..2a1533c 100644
> > > > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > > > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > > > @@ -545,6 +545,55 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev,
> > > struct
> > > > ether_addr *mac_addr)  }
> > > >
> > > >  /**
> > > > + * Convert verbs RSS types to their DPDK equivalents.
> > > > + *
> > > > + * This function returns a group of RSS dpdk types given their
> > > > +equivalent group
> > > > + * of verbs types.
> > > > + * For example both source IPv4 and destination IPv4 verbs types
> > > > +are converted
> > > > + * into their equivalent RSS group types. If each of these verbs
> > > > +types existed
> > > > + * exclusively - no conversion would take place.
> > > > + *
> > > > + * @param types
> > > > + *   RSS hash types in verbs format
> > > > + *
> > > > + * @return
> > > > + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
> > > > + */
> > > > +static uint64_t
> > > > +mlx4_ibv_to_dpdk_rss_types(uint64_t types) {
> > > > +	enum { IPV4, IPV6, TCP, UDP, };
> > > > +	const uint64_t in[] = {
> > > > +		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> > > > IBV_RX_HASH_DST_IPV4,
> > > > +		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> > > > IBV_RX_HASH_DST_IPV6,
> > > > +		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> > > > IBV_RX_HASH_DST_PORT_TCP,
> > > > +		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> > > > IBV_RX_HASH_DST_PORT_UDP,
> > > > +	};
> > > > +	const uint64_t out[RTE_DIM(in)] = {
> > > > +		[IPV4] = (ETH_RSS_IPV4 |
> > > > +			  ETH_RSS_FRAG_IPV4 |
> > > > +			  ETH_RSS_NONFRAG_IPV4_OTHER),
> > > > +		[IPV6] = (ETH_RSS_IPV6 |
> > > > +			  ETH_RSS_FRAG_IPV6 |
> > > > +			  ETH_RSS_NONFRAG_IPV6_OTHER |
> > > > +			  ETH_RSS_IPV6_EX),
> > > > +		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> > > > +			 ETH_RSS_NONFRAG_IPV6_TCP |
> > > > +			 ETH_RSS_IPV6_TCP_EX),
> > > > +		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> > > > +			 ETH_RSS_NONFRAG_IPV6_UDP |
> > > > +			 ETH_RSS_IPV6_UDP_EX),
> > > > +	};
> > >
> > > Since above are constants, why not defining a global array of
> > > structs containing the ibv_hash and the equivalent dpdk_hash,
> > > instead of recreating it for each call?
> >
> > The array is recreated because it should have been declared as "static
> const"
> > rather than just "const". I prefer this fix.
> > Alternatively it could have been defined globally outside of the
> > function as suggested which would have avoided recreation as well.
> > All of those claims are confirmed by inspecting the assembly code of
> > the above alternatives.
> >
> > > There is a similar concept on mlx5_flow.c:
> > >
> > > /* Initialization data for hash RX queues. */  const struct
> > > hash_rxq_init hash_rxq_init[] = {
> > >          [HASH_RXQ_TCPV4] = {
> > >                  .hash_fields = (IBV_RX_HASH_SRC_IPV4 |
> > >                                  IBV_RX_HASH_DST_IPV4 |
> > >                                  IBV_RX_HASH_SRC_PORT_TCP |
> > >                                  IBV_RX_HASH_DST_PORT_TCP),
> > >                  .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,
> > >                  .flow_priority = 0,
> > >                  .ip_version = MLX5_IPV4,
> > >          },
> > >
> >
> > I was inspired from "the reversed" function in mlx4_flow.c which
> > defines constants inside the function as well, see [1].
> > To fix the constants recreation both functions should be handled together.
> >
> > I suggest:
> > 1. Moving mlx4_ibv_to_dpdk_rss_types() function as is from
> > mlx4_ether.c to mlx4_flow.c so it will be adjacent to
> mlx4_conv_rss_types() function.
> > 2. Sending a new patch that avoids constants recreation in the 2 functions.
> 
> OK. Those can be 2 patches from the same series.
> 
> >
> > [1]
> > uint64_t
> > mlx4_conv_rss_types(struct priv *priv, uint64_t types) {
> > 	enum { IPV4, IPV6, TCP, UDP, };
> > 	const uint64_t in[] = {
> > 		[IPV4] = (ETH_RSS_IPV4 |
> > 			  ETH_RSS_FRAG_IPV4 |
> > 			  ETH_RSS_NONFRAG_IPV4_TCP |
> > 			  ETH_RSS_NONFRAG_IPV4_UDP |
> > 			  ETH_RSS_NONFRAG_IPV4_OTHER),
> > 		[IPV6] = (ETH_RSS_IPV6 |
> > 			  ETH_RSS_FRAG_IPV6 |
> > 			  ETH_RSS_NONFRAG_IPV6_TCP |
> > 			  ETH_RSS_NONFRAG_IPV6_UDP |
> > 			  ETH_RSS_NONFRAG_IPV6_OTHER |
> > 			  ETH_RSS_IPV6_EX |
> > 			  ETH_RSS_IPV6_TCP_EX |
> > 			  ETH_RSS_IPV6_UDP_EX),
> > 		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> > 			 ETH_RSS_NONFRAG_IPV6_TCP |
> > 			 ETH_RSS_IPV6_TCP_EX),
> > 		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> > 			 ETH_RSS_NONFRAG_IPV6_UDP |
> > 			 ETH_RSS_IPV6_UDP_EX),
> > 	};
> > 	const uint64_t out[RTE_DIM(in)] = {
> > 		[IPV4] = IBV_RX_HASH_SRC_IPV4 |
> > IBV_RX_HASH_DST_IPV4,
> > 		[IPV6] = IBV_RX_HASH_SRC_IPV6 |
> > IBV_RX_HASH_DST_IPV6,
> > 		[TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> > IBV_RX_HASH_DST_PORT_TCP,
> > 		[UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> > IBV_RX_HASH_DST_PORT_UDP,
> > 	};
> > 	uint64_t seen = 0;
> > 	uint64_t conv = 0;
> > 	unsigned int i;
> >
> > 	if (types == (uint64_t)-1)
> > 		return priv->hw_rss_sup;
> > 	for (i = 0; i != RTE_DIM(in); ++i)
> > 		if (types & in[i]) {
> > 			seen |= types & in[i];
> > 			conv |= out[i];
> > 		}
> > 	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
> > 		return conv;
> > 	rte_errno = ENOTSUP;
> > 	return (uint64_t)-1;
> > }
> >
> > > > +	uint64_t conv = 0;
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i != RTE_DIM(in); ++i)
> > > > +		if ((types & in[i]) == in[i])
> > > > +			conv |= out[i];
> > > > +	return conv;
> > > > +}
> > > > +
> > > > +/**
> > > >   * DPDK callback to get information about the device.
> > > >   *
> > > >   * @param dev
> > > > @@ -587,6 +636,8 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev,
> > > struct
> > > > rte_eth_dev_info *info)
> > > >  			ETH_LINK_SPEED_20G |
> > > >  			ETH_LINK_SPEED_40G |
> > > >  			ETH_LINK_SPEED_56G;
> > > > +	info->flow_type_rss_offloads = mlx4_ibv_to_dpdk_rss_types(
> > > > +			priv->hw_rss_sup);
> > > >  }
> > > >
> > > >  /**
> > > > --
> > > > 2.7.4
  

Patch

diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
index 9a76670..2a1533c 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -545,6 +545,55 @@  mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 }
 
 /**
+ * Convert verbs RSS types to their DPDK equivalents.
+ *
+ * This function returns a group of RSS dpdk types given their equivalent group
+ * of verbs types.
+ * For example both source IPv4 and destination IPv4 verbs types are converted
+ * into their equivalent RSS group types. If each of these verbs types existed
+ * exclusively - no conversion would take place.
+ *
+ * @param types
+ *   RSS hash types in verbs format
+ *
+ * @return
+ *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
+ */
+static uint64_t
+mlx4_ibv_to_dpdk_rss_types(uint64_t types)
+{
+	enum { IPV4, IPV6, TCP, UDP, };
+	const uint64_t in[] = {
+		[IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4,
+		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6,
+		[TCP] = IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP,
+		[UDP] = IBV_RX_HASH_SRC_PORT_UDP | IBV_RX_HASH_DST_PORT_UDP,
+	};
+	const uint64_t out[RTE_DIM(in)] = {
+		[IPV4] = (ETH_RSS_IPV4 |
+			  ETH_RSS_FRAG_IPV4 |
+			  ETH_RSS_NONFRAG_IPV4_OTHER),
+		[IPV6] = (ETH_RSS_IPV6 |
+			  ETH_RSS_FRAG_IPV6 |
+			  ETH_RSS_NONFRAG_IPV6_OTHER |
+			  ETH_RSS_IPV6_EX),
+		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
+			 ETH_RSS_NONFRAG_IPV6_TCP |
+			 ETH_RSS_IPV6_TCP_EX),
+		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
+			 ETH_RSS_NONFRAG_IPV6_UDP |
+			 ETH_RSS_IPV6_UDP_EX),
+	};
+	uint64_t conv = 0;
+	unsigned int i;
+
+	for (i = 0; i != RTE_DIM(in); ++i)
+		if ((types & in[i]) == in[i])
+			conv |= out[i];
+	return conv;
+}
+
+/**
  * DPDK callback to get information about the device.
  *
  * @param dev
@@ -587,6 +636,8 @@  mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 			ETH_LINK_SPEED_20G |
 			ETH_LINK_SPEED_40G |
 			ETH_LINK_SPEED_56G;
+	info->flow_type_rss_offloads = mlx4_ibv_to_dpdk_rss_types(
+			priv->hw_rss_sup);
 }
 
 /**