Re: [dpdk-dev] [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 04/06/2018 04:25 PM, Adrien Mazarguil wrote:
> Since its inception, the rte_flow RSS action has been relying in part on
> external struct rte_eth_rss_conf for compatibility with the legacy RSS API.
> This structure lacks parameters such as the hash algorithm to use, and more
> recently, a method to tell which layer RSS should be performed on [1].
>
> Given struct rte_eth_rss_conf will never be flexible enough to represent a
> complete RSS configuration (e.g. RETA table), this patch supersedes it by
> extending the rte_flow RSS action directly.
>
> A subsequent patch will add a field to use a non-default RSS hash
> algorithm. To that end, a field named "types" replaces the field formerly
> known as "rss_hf" and standing for "RSS hash functions" as it was
> confusing. Actual RSS hash function types are defined by enum
> rte_eth_hash_function.
> This patch updates all PMDs and example applications accordingly.
>
> It breaks ABI compatibility for the following public functions:
>
> - rte_flow_copy()
> - rte_flow_create()
> - rte_flow_query()
> - rte_flow_validate()
>
> [1] commit 676b605182a5 ("doc: announce ethdev API change for RSS
>      configuration")
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Xueming Li <xuemingl@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>
> Cc: Beilei Xing <beilei.xing@intel.com>
> Cc: Qi Zhang <qi.z.zhang@intel.com>
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Cc: Yongseok Koh <yskoh@mellanox.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Pascal Mazon <pascal.mazon@6wind.com>
> Cc: Radu Nicolau <radu.nicolau@intel.com>
> Cc: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>   app/test-pmd/cmdline_flow.c        |  59 +++++-----
>   app/test-pmd/config.c              |  39 +++----
>   doc/guides/prog_guide/rte_flow.rst |  22 ++--
>   drivers/net/e1000/e1000_ethdev.h   |  13 ++-
>   drivers/net/e1000/igb_ethdev.c     |   4 +-
>   drivers/net/e1000/igb_flow.c       |  31 ++---
>   drivers/net/e1000/igb_rxtx.c       |  51 +++++++--
>   drivers/net/i40e/i40e_ethdev.c     |  53 +++++++--
>   drivers/net/i40e/i40e_ethdev.h     |  15 ++-
>   drivers/net/i40e/i40e_flow.c       |  47 ++++----
>   drivers/net/ixgbe/ixgbe_ethdev.c   |   4 +-
>   drivers/net/ixgbe/ixgbe_ethdev.h   |  13 ++-
>   drivers/net/ixgbe/ixgbe_flow.c     |  30 ++---
>   drivers/net/ixgbe/ixgbe_rxtx.c     |  51 +++++++--
>   drivers/net/mlx4/mlx4.c            |   2 +-
>   drivers/net/mlx4/mlx4_flow.c       |  61 +++++-----
>   drivers/net/mlx4/mlx4_flow.h       |   2 +-
>   drivers/net/mlx4/mlx4_rxq.c        |   2 +-
>   drivers/net/mlx4/mlx4_rxtx.h       |   2 +-
>   drivers/net/mlx5/mlx5_flow.c       | 193 +++++++++++++++-----------------
>   drivers/net/mlx5/mlx5_rxq.c        |  22 ++--
>   drivers/net/mlx5/mlx5_rxtx.h       |  26 +++--
>   drivers/net/sfc/sfc_flow.c         |  21 ++--
>   drivers/net/tap/tap_flow.c         |   8 +-
>   examples/ipsec-secgw/ipsec.c       |  10 +-
>   lib/librte_ether/rte_flow.c        |  39 +++----
>   lib/librte_ether/rte_flow.h        |   6 +-
>   27 files changed, 473 insertions(+), 353 deletions(-)

<...>

> diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c
> index 056405515..1a2c0299c 100644
> --- a/drivers/net/sfc/sfc_flow.c
> +++ b/drivers/net/sfc/sfc_flow.c
> @@ -1234,13 +1234,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
>   	struct sfc_rxq *rxq;
>   	unsigned int rxq_hw_index_min;
>   	unsigned int rxq_hw_index_max;
> -	const struct rte_eth_rss_conf *rss_conf = rss->rss_conf;
> -	uint64_t rss_hf;
> -	uint8_t *rss_key = NULL;
> +	const uint8_t *rss_key;
>   	struct sfc_flow_rss *sfc_rss_conf = &flow->rss_conf;
>   	unsigned int i;
>   
> -	if (rss->num == 0)
> +	if (rss->queue_num == 0)
>   		return -EINVAL;
>   
>   	rxq_sw_index = sa->rxq_count - 1;
> @@ -1248,7 +1246,7 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
>   	rxq_hw_index_min = rxq->hw_index;
>   	rxq_hw_index_max = 0;
>   
> -	for (i = 0; i < rss->num; ++i) {
> +	for (i = 0; i < rss->queue_num; ++i) {
>   		rxq_sw_index = rss->queue[i];
>   
>   		if (rxq_sw_index >= sa->rxq_count)
> @@ -1263,15 +1261,14 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
>   			rxq_hw_index_max = rxq->hw_index;
>   	}
>   
> -	rss_hf = (rss_conf != NULL) ? rss_conf->rss_hf : SFC_RSS_OFFLOADS;

Here we had a fallback to default rss_hf (now types) if rss_conf is 
unspecified.

> -	if ((rss_hf & ~SFC_RSS_OFFLOADS) != 0)
> +	if ((rss->types & ~SFC_RSS_OFFLOADS) != 0)
>   		return -EINVAL;
>   
> -	if (rss_conf != NULL) {
> -		if (rss_conf->rss_key_len != sizeof(sa->rss_key))
> +	if (rss->key_len) {
> +		if (rss->key_len != sizeof(sa->rss_key))
>   			return -EINVAL;
>   
> -		rss_key = rss_conf->rss_key;
> +		rss_key = rss->key;
>   	} else {
>   		rss_key = sa->rss_key;
>   	}
> @@ -1280,11 +1277,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
>   
>   	sfc_rss_conf->rxq_hw_index_min = rxq_hw_index_min;
>   	sfc_rss_conf->rxq_hw_index_max = rxq_hw_index_max;
> -	sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss_hf);
> +	sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types);

Now types go directly to mapping function and unspecified types (0)
will result in 0 rss_hash_types. Of course, it is a question how to treat
types==0. It is possible to say that it no RSS, but it does not make sense.
So, real options are device defaults (regardless configured on device level)
or device config (rx_adv.conf.rss_conf.rss_hf). I would prefer the later.
Please, document the intended behaviour in rte_flow.rst.

If the later is chosen, above we'll have a bug since fallback to fixed 
default.
Just use sa->rss_hash_types as fallback. Something like:
if (rss->types)
     sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types);
else
     sfc_rss_conf->rss_hash_types =sa->rss_hash_types;

>   	rte_memcpy(sfc_rss_conf->rss_key, rss_key, sizeof(sa->rss_key));
>   
>   	for (i = 0; i < RTE_DIM(sfc_rss_conf->rss_tbl); ++i) {
> -		unsigned int rxq_sw_index = rss->queue[i % rss->num];
> +		unsigned int rxq_sw_index = rss->queue[i % rss->queue_num];
>   		struct sfc_rxq *rxq = sa->rxq_info[rxq_sw_index].rxq;
>   
>   		sfc_rss_conf->rss_tbl[i] = rxq->hw_index - rxq_hw_index_min;

<...>