diff mbox series

[v11,2/3] ethdev: extend RSS offload types

Message ID 1570604240-97643-3-git-send-email-simei.su@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series extend RSS offload types |

Checks

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

Commit Message

Simei Su Oct. 9, 2019, 6:57 a.m. UTC 
  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 |  5 +++++
 lib/librte_ethdev/rte_ethdev.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Andrew Rybchenko Oct. 9, 2019, 7:18 a.m. UTC | #1 
On 10/9/19 9:57 AM, 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 |  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 af82360..69f4133 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 =
> +		strip_out_src_dst_only(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 = strip_out_src_dst_only(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 7722f70..ef59ed5 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 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 | \
> @@ -4034,6 +4048,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);
>   
> +/**
> + * 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
> +strip_out_src_dst_only(uint64_t rss_hf)

Inline function in public header without corresponding prefix is a bad idea.
Please, move it to C file and I think that inline should be removed.
Let the compiler do its job.

> +{
> +	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;
> +}
>   
>   #include <rte_ethdev_core.h>
>
Simei Su Oct. 9, 2019, 7:42 a.m. UTC | #2 
Hi, Andrew

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Wednesday, October 9, 2019 3:18 PM
> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload types
> 
> On 10/9/19 9:57 AM, 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 |  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 af82360..69f4133 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 =
> > +		strip_out_src_dst_only(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 = strip_out_src_dst_only(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 7722f70..ef59ed5 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 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 | \
> > @@ -4034,6 +4048,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);
> >
> > +/**
> > + * 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
> > +strip_out_src_dst_only(uint64_t rss_hf)
> 
> Inline function in public header without corresponding prefix is a bad idea.
> Please, move it to C file and I think that inline should be removed.
> Let the compiler do its job.
> 

  Because I also need to check simultaneous use of SRC/DST_ONLY in PMD driver. 
  In order to call strip_out_src_dst_only() function directly in driver,
  I put it in header file and declare it as inline function.

> > +{
> > +	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;
> > +}
> >
> >   #include <rte_ethdev_core.h>
> >
Andrew Rybchenko Oct. 9, 2019, 7:55 a.m. UTC | #3 
On 10/9/19 10:42 AM, Su, Simei wrote:
> Hi, Andrew
>
>> -----Original Message-----
>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>> Sent: Wednesday, October 9, 2019 3:18 PM
>> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye,
>> Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload types
>>
>> On 10/9/19 9:57 AM, 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>

[snip]

>>> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 7722f70..ef59ed5 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -4034,6 +4048,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);
>>>
>>> +/**
>>> + * 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
>>> +strip_out_src_dst_only(uint64_t rss_hf)
>> Inline function in public header without corresponding prefix is a bad idea.
>> Please, move it to C file and I think that inline should be removed.
>> Let the compiler do its job.
>    Because I also need to check simultaneous use of SRC/DST_ONLY in PMD driver.
>    In order to call strip_out_src_dst_only() function directly in driver,
>    I put it in header file and declare it as inline function.

At bare minimum it should have rte_eth_dev_ prefix.
Also from the name it is not clear that it is about RSS etc.
Not sure why you need it in driver as well, hopefully I'll see.

Andrew.
Simei Su Oct. 9, 2019, 9:08 a.m. UTC | #4 
Hi, Andrew

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Wednesday, October 9, 2019 3:55 PM
> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload types
> 
> On 10/9/19 10:42 AM, Su, Simei wrote:
> > Hi, Andrew
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >> Sent: Wednesday, October 9, 2019 3:18 PM
> >> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit,
> >> Ferruh <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload
> >> types
> >>
> >> On 10/9/19 9:57 AM, 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>
> 
> [snip]
> 
> >>> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >>> index 7722f70..ef59ed5 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -4034,6 +4048,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);
> >>>
> >>> +/**
> >>> + * 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
> >>> +strip_out_src_dst_only(uint64_t rss_hf)
> >> Inline function in public header without corresponding prefix is a bad idea.
> >> Please, move it to C file and I think that inline should be removed.
> >> Let the compiler do its job.
> >    Because I also need to check simultaneous use of SRC/DST_ONLY in PMD
> driver.
> >    In order to call strip_out_src_dst_only() function directly in driver,
> >    I put it in header file and declare it as inline function.
> 
> At bare minimum it should have rte_eth_dev_ prefix.
> Also from the name it is not clear that it is about RSS etc.
> Not sure why you need it in driver as well, hopefully I'll see.
> 
> Andrew.

I want to reuse it in hash filter, so just put it in public header but ignore rte_eth_dev_prefix and function name.
I reconsider it and think put it in C file is much better.
I will send the next version based on your advice.

As for need it in driver, because when creating a flow, PMD driver should check its RSS types of its own side. It has nothing to do with etherdev.

Br
Simei
Qi Zhang Oct. 9, 2019, 9:32 a.m. UTC | #5 
> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Wednesday, October 9, 2019 3:55 PM
> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload types
> 
> On 10/9/19 10:42 AM, Su, Simei wrote:
> > Hi, Andrew
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >> Sent: Wednesday, October 9, 2019 3:18 PM
> >> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit,
> >> Ferruh <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload
> >> types
> >>
> >> On 10/9/19 9:57 AM, 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>
> 
> [snip]
> 
> >>> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >>> index 7722f70..ef59ed5 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -4034,6 +4048,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);
> >>>
> >>> +/**
> >>> + * 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
> >>> +strip_out_src_dst_only(uint64_t rss_hf)
> >> Inline function in public header without corresponding prefix is a bad idea.
> >> Please, move it to C file and I think that inline should be removed.
> >> Let the compiler do its job.
> >    Because I also need to check simultaneous use of SRC/DST_ONLY in
> PMD driver.
> >    In order to call strip_out_src_dst_only() function directly in driver,
> >    I put it in header file and declare it as inline function.
> 
> At bare minimum it should have rte_eth_dev_ prefix.
> Also from the name it is not clear that it is about RSS etc.
> Not sure why you need it in driver as well, hopefully I'll see.

The consideration is when handle rte_flow_action_rss, we still need to strip it out since this route will bypass the dev_configure or rss_update 
So there are two options
1, strip out at rte_flow_create , this relief all the PMDs, but code looks a little bit strange.
2. handled by PMD themselves 

Anyway both of the cases need this helper function be exposed by rte_ethdev.h, maybe we can define a macro named RTE_ETH_RSS_HF_REFINE?

Regards
Qi 
> 
> Andrew.
Simei Su Oct. 10, 2019, 2:37 p.m. UTC | #6 
Hi, Andrew

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, October 9, 2019 5:32 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Su, Simei
> <simei.su@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload types
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > Sent: Wednesday, October 9, 2019 3:55 PM
> > To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit,
> > Ferruh <ferruh.yigit@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload
> > types
> >
> > On 10/9/19 10:42 AM, Su, Simei wrote:
> > > Hi, Andrew
> > >
> > >> -----Original Message-----
> > >> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > >> Sent: Wednesday, October 9, 2019 3:18 PM
> > >> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z
> > >> <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> > >> Yigit, Ferruh <ferruh.yigit@intel.com>
> > >> Cc: dev@dpdk.org
> > >> Subject: Re: [dpdk-dev] [PATCH v11 2/3] ethdev: extend RSS offload
> > >> types
> > >>
> > >> On 10/9/19 9:57 AM, 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>
> >
> > [snip]
> >
> > >>> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > >>> index 7722f70..ef59ed5 100644
> > >>> --- a/lib/librte_ethdev/rte_ethdev.h
> > >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> > >>> @@ -4034,6 +4048,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);
> > >>>
> > >>> +/**
> > >>> + * 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
> > >>> +strip_out_src_dst_only(uint64_t rss_hf)
> > >> Inline function in public header without corresponding prefix is a bad idea.
> > >> Please, move it to C file and I think that inline should be removed.
> > >> Let the compiler do its job.
> > >    Because I also need to check simultaneous use of SRC/DST_ONLY in
> > PMD driver.
> > >    In order to call strip_out_src_dst_only() function directly in driver,
> > >    I put it in header file and declare it as inline function.
> >
> > At bare minimum it should have rte_eth_dev_ prefix.
> > Also from the name it is not clear that it is about RSS etc.
> > Not sure why you need it in driver as well, hopefully I'll see.
> 
> The consideration is when handle rte_flow_action_rss, we still need to strip it out
> since this route will bypass the dev_configure or rss_update So there are two
> options 1, strip out at rte_flow_create , this relief all the PMDs, but code looks a
> little bit strange.
> 2. handled by PMD themselves
> 
> Anyway both of the cases need this helper function be exposed by rte_ethdev.h,
> maybe we can define a macro named RTE_ETH_RSS_HF_REFINE?
> 
> Regards
> Qi

What advice do you have for above proposal or do you have a better suggestion? Thanks!

Br
Simei
> >
> > Andrew.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index af82360..69f4133 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 =
+		strip_out_src_dst_only(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 = strip_out_src_dst_only(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 7722f70..ef59ed5 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 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 | \
@@ -4034,6 +4048,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);
 
+/**
+ * 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
+strip_out_src_dst_only(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;
+}
 
 #include <rte_ethdev_core.h>