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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Sat, Apr 07, 2018 at 12:05:51PM +0300, Andrew Rybchenko wrote:
> 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.

Thing is, rss_action->conf was never supposed to be NULL in the first
place. Crashing on a NULL configuration has always been fine, but until
recently prevented validation with testpmd's broken implementation. This
problem was addressed in a prior series [1][2][3].

Since a value is now always provided, no need for a fallback.

[1] "app/testpmd: fix lack of flow action configuration"
    http://dpdk.org/ml/archives/dev/2018-April/095280.html
[2] "app/testpmd: fix RSS flow action configuration"
    http://dpdk.org/ml/archives/dev/2018-April/095281.html
[3] "app/testpmd: fix missing RSS fields in flow action"
    http://dpdk.org/ml/archives/dev/2018-April/095282.html

> > -	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.

Granted the existing documentation doesn't say much on that topic, but a 0
value for rss_hf does actually mean "no RSS" [4]:

 "The *rss_hf* field of the *rss_conf* structure indicates the different
  types of IPv4/IPv6 packets to which the RSS hashing must be applied.
  Supplying an *rss_hf* equal to zero disables the RSS feature."

Now since this action doesn't use struct rte_eth_rss_conf anymore, we could
define 0 as a PMD-specific behavior, which could be no RSS. It would make
the API easier to use for applications that don't care about the RSS
capabilities of each underlying adapter, 0 would just work everywhere as a
safe default.

[4] https://dpdk.org/doc/api/structrte__eth__rss__conf.html

> 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;

Looks like the previous code didn't provide a fallback when rss_hf was 0,
only when rss_conf itself was NULL. So this is not a new issue introduced by
this patch.

I will update documentation to define 0 as described above for the
convenience of application writers and leave the existing code in place.
PMD maintainers will be free to enhance it as they wish later.
Just remember testpmd now always provides a default value for it after
querying the device [2].

> >   	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;
> 
> <...>

-- 
Adrien Mazarguil
6WIND