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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/15/19 3:56 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. This patch also checks the simultaneous
> use of SRC_ONLY and DST_ONLY of the same level.
>
> 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>

One nit below
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

> ---
>   lib/librte_ethdev/rte_ethdev.c |  5 +++++
>   lib/librte_ethdev/rte_ethdev.h | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 7caaa0b..3f989df 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1269,6 +1269,9 @@ struct rte_eth_dev *
>   		goto rollback;
>   	}
>   
> +	dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf =
> +		rte_eth_rss_hf_refine(dev_conf->rx_adv_conf.rss_conf.rss_hf);
> +
>   	/* 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 +3115,8 @@ struct rte_eth_dev *
>   	if (ret != 0)
>   		return ret;
>   
> +	rss_conf->rss_hf = rte_eth_rss_hf_refine(rss_conf->rss_hf);
> +
>   	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 40aa6a3..57fe78b 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -507,6 +507,20 @@ struct rte_eth_rss_conf {
>   #define ETH_RSS_NVGRE              (1ULL << 21)
>   #define ETH_RSS_GTPU               (1ULL << 23)
>   
> +/*
> + * 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 are 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 | \
> @@ -4036,6 +4050,27 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t port_id,
>   void *
>   rte_eth_dev_get_sec_ctx(uint16_t port_id);
>   
> +/**
> + * For input set change of hash filter, if SRC_ONLY and DST_ONLY of
> + * the same level are used simultaneously, it is the same case as
> + * none of them are added.
> + *
> + * @param rss_hf
> + *   RSS types with SRC/DST_ONLY.
> + * @return
> + *   RSS types.
> + */
> +static inline uint64_t
> +rte_eth_rss_hf_refine(uint64_t rss_hf)
> +{
> +	if ((rss_hf & ETH_RSS_L3_SRC_ONLY) && (rss_hf & ETH_RSS_L3_DST_ONLY))
> +		rss_hf &= ~(ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY);
> +
> +	if ((rss_hf & ETH_RSS_L4_SRC_ONLY) && (rss_hf & ETH_RSS_L4_DST_ONLY))
> +		rss_hf &= ~(ETH_RSS_L4_SRC_ONLY | ETH_RSS_L4_DST_ONLY);
> +
> +	return rss_hf;
> +}

May I suggest to put the function just after ETH_RSS defines.

>   #include <rte_ethdev_core.h>
>