Re: [dpdk-dev] [PATCH v8 2/3] ethdev: extend RSS offload types

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/1/19 5:36 PM, Simei Su wrote:
> This patch reserves several bits as input set selection from the
> high end of the 64 bits. It is combined with exisiting ETH_RSS_*
> to represent RSS types.
>
> Signed-off-by: Simei Su <simei.su@intel.com>
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 22 ++++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev.h | 14 ++++++++++++++
>   2 files changed, 36 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index af82360..5e5a974 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1269,6 +1269,17 @@ struct rte_eth_dev *
>   		goto rollback;
>   	}
>   
> +	/* simplified the SRC/DST_ONLY RSS offload modificaiton */
> +	if (dev_conf->rx_adv_conf.rss_conf.rss_hf & ETH_RSS_L3_SRC_ONLY &&
> +		dev_conf->rx_adv_conf.rss_conf.rss_hf & ETH_RSS_L3_DST_ONLY)

I'm afraid some compiler versions could bark about missing parenthesis here.
Consider to use something like:
  (rss_conf->rss_hf & (ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY) ==
   (ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY))
May be parenthesis around & is not required.

> +			dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &=
> +				~(ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY);
> +
> +	if (dev_conf->rx_adv_conf.rss_conf.rss_hf & ETH_RSS_L4_SRC_ONLY &&
> +		dev_conf->rx_adv_conf.rss_conf.rss_hf & ETH_RSS_L4_DST_ONLY)
> +			dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &=
> +				~(ETH_RSS_L4_SRC_ONLY | ETH_RSS_L4_DST_ONLY);
> +

I think it is wrong to duplicate the logic twice (here and below).
Helper static function should be used to avoid it.

>   	/* Check that device supports requested rss hash functions. */
>   	if ((dev_info.flow_type_rss_offloads |
>   	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> @@ -3112,6 +3123,17 @@ struct rte_eth_dev *
>   	if (ret != 0)
>   		return ret;
>   
> +	/* simplified the SRC/DST_ONLY RSS offload modificaiton */
> +	if (rss_conf->rss_hf & ETH_RSS_L3_SRC_ONLY &&
> +		rss_conf->rss_hf & ETH_RSS_L3_DST_ONLY)
> +			rss_conf->rss_hf &=
> +				~(ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY);
> +
> +	if (rss_conf->rss_hf & ETH_RSS_L4_SRC_ONLY &&
> +		rss_conf->rss_hf & ETH_RSS_L4_DST_ONLY)
> +			rss_conf->rss_hf &=
> +				~(ETH_RSS_L4_SRC_ONLY | ETH_RSS_L4_DST_ONLY);
> +
>   	dev = &rte_eth_devices[port_id];
>   	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
>   	    dev_info.flow_type_rss_offloads) {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 7722f70..6d61b84 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -505,6 +505,20 @@ struct rte_eth_rss_conf {
>   #define ETH_RSS_GENEVE             (1ULL << 20)
>   #define ETH_RSS_NVGRE              (1ULL << 21)
>   
> +/*
> + * We use the following macros to combine with above ETH_RSS_* for
> + * more specific input set selection. These bits are defined starting
> + * from the high end of the 64 bits.
> + * Note: If we use above ETH_RSS_* without SRC/DST_ONLY, it represents
> + * both SRC and DST are taken into account. If SRC_ONLY and DST_ONLY of
> + * the same level be used simultaneously, it is the same case as none of
> + * them are added.
> + */
> +#define ETH_RSS_L3_SRC_ONLY        (1ULL << 63)
> +#define ETH_RSS_L3_DST_ONLY        (1ULL << 62)
> +#define ETH_RSS_L4_SRC_ONLY        (1ULL << 61)
> +#define ETH_RSS_L4_DST_ONLY        (1ULL << 60)
> +
>   #define ETH_RSS_IP ( \
>   	ETH_RSS_IPV4 | \
>   	ETH_RSS_FRAG_IPV4 | \