[RFC] ethdev: support input set change by RSS action

  From: Simei Su <simei.su@intel.com>

This RFC introduces inputset structure to rte_flow_action_rss to
support input set specific configuration by rte_flow RSS action.

We can give an testpmd command line example to make it more clear.

For example, below flow selects the l4 port as inputset for any
eth/ipv4/tcp packet: #flow create 0 ingress pattern eth / ipv4 / tcp /
end actions rss inputset tcp src mask 0xffff dst mask 0xffff /end

Signed-off-by: Simei Su <simei.su@intel.com>
---
 lib/librte_ethdev/rte_flow.h | 3 +++
 1 file changed, 3 insertions(+)
  

On Thu, Jul 04, 2019 at 12:47:09PM +0800, simei wrote:
> From: Simei Su <simei.su@intel.com>
> 
> This RFC introduces inputset structure to rte_flow_action_rss to
> support input set specific configuration by rte_flow RSS action.
> 
> We can give an testpmd command line example to make it more clear.
> 
> For example, below flow selects the l4 port as inputset for any
> eth/ipv4/tcp packet: #flow create 0 ingress pattern eth / ipv4 / tcp /
> end actions rss inputset tcp src mask 0xffff dst mask 0xffff /end
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
>  lib/librte_ethdev/rte_flow.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index f3a8fb1..2a455b6 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1796,6 +1796,9 @@ struct rte_flow_action_rss {
>  	uint32_t queue_num; /**< Number of entries in @p queue. */
>  	const uint8_t *key; /**< Hash key. */
>  	const uint16_t *queue; /**< Queue indices to use. */
> +	struct rte_flow_item *inputset; /** Provide more specific inputset configuration.
> +					 * ignore spec, only mask.
> +					 */
>  };
>  
>  /**

To make sure I understand, is this kind of a more flexible version of
rte_flow_action_rss.types?

For instance while specifying .types = ETH_RSS_IPV4 normally covers both
source and destination addresses, does this approach enable users to perform
RSS on source IP only? In which case, what value does the Toeplitz algorithm
assume for the destination, 0x0? (note: must be documented)

My opinion is that, unless you know of a hardware which can perform RSS on
random bytes of a packet, this approach is a bit overkill at this point.

How about simply adding the needed ETH_RSS_* definitions
(e.g. ETH_RSS_IPV4_(SRC|DST))? How many are needed?

There are currently 20 used bits and struct rte_flow_action_rss.types is
64-bit wide. I'm sure we can manage something without harming the ABI. Even
better, you wouldn't need a deprecation notice.

If you use the suggested approach, please update testpmd and its
documentation as part of the same patch, thanks.
  

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, July 4, 2019 5:07 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] ethdev: support input set change by RSS action
> 
> On Thu, Jul 04, 2019 at 12:47:09PM +0800, simei wrote:
> > From: Simei Su <simei.su@intel.com>
> >
> > This RFC introduces inputset structure to rte_flow_action_rss to
> > support input set specific configuration by rte_flow RSS action.
> >
> > We can give an testpmd command line example to make it more clear.
> >
> > For example, below flow selects the l4 port as inputset for any
> > eth/ipv4/tcp packet: #flow create 0 ingress pattern eth / ipv4 / tcp /
> > end actions rss inputset tcp src mask 0xffff dst mask 0xffff /end
> >
> > Signed-off-by: Simei Su <simei.su@intel.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index f3a8fb1..2a455b6 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1796,6 +1796,9 @@ struct rte_flow_action_rss {
> >  	uint32_t queue_num; /**< Number of entries in @p queue. */
> >  	const uint8_t *key; /**< Hash key. */
> >  	const uint16_t *queue; /**< Queue indices to use. */
> > +	struct rte_flow_item *inputset; /** Provide more specific inputset
> configuration.
> > +					 * ignore spec, only mask.
> > +					 */
> >  };
> >
> >  /**
> 
> To make sure I understand, is this kind of a more flexible version of
> rte_flow_action_rss.types?

Yes
> 
> For instance while specifying .types = ETH_RSS_IPV4 normally covers both
> source and destination addresses, does this approach enable users to perform
> RSS on source IP only? 

Yes, .it is the case to select any subset of 5 tuples or even tunnel header's id for hash

> In which case, what value does the Toeplitz algorithm
> assume for the destination, 0x0? (note: must be documented)

My understanding is src/dst pair is only required for a symmetric case
But for Toeplitz, it is just a hash function, it process a serial of data with specific algorithm, have no idea about which part is src and dst , 
So for ip src only with Toeplitz, dst is not required to be a placeholder..
anything I missed, would you share more insight?

Thanks
Qi

> 
> My opinion is that, unless you know of a hardware which can perform RSS on
> random bytes of a packet, this approach is a bit overkill at this point.
> 
> How about simply adding the needed ETH_RSS_* definitions (e.g.
> ETH_RSS_IPV4_(SRC|DST))? How many are needed?
> 
> There are currently 20 used bits and struct rte_flow_action_rss.types is 64-bit
> wide. I'm sure we can manage something without harming the ABI. Even
> better, you wouldn't need a deprecation notice.
> 
> If you use the suggested approach, please update testpmd and its
> documentation as part of the same patch, thanks.
> 
> --
> Adrien Mazarguil
> 6WIND
  

On Thu, Jul 04, 2019 at 01:55:14PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Thursday, July 4, 2019 5:07 PM
> > To: Su, Simei <simei.su@intel.com>
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [RFC] ethdev: support input set change by RSS action
> > 
> > On Thu, Jul 04, 2019 at 12:47:09PM +0800, simei wrote:
> > > From: Simei Su <simei.su@intel.com>
> > >
> > > This RFC introduces inputset structure to rte_flow_action_rss to
> > > support input set specific configuration by rte_flow RSS action.
> > >
> > > We can give an testpmd command line example to make it more clear.
> > >
> > > For example, below flow selects the l4 port as inputset for any
> > > eth/ipv4/tcp packet: #flow create 0 ingress pattern eth / ipv4 / tcp /
> > > end actions rss inputset tcp src mask 0xffff dst mask 0xffff /end
> > >
> > > Signed-off-by: Simei Su <simei.su@intel.com>
> > > ---
> > >  lib/librte_ethdev/rte_flow.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index f3a8fb1..2a455b6 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -1796,6 +1796,9 @@ struct rte_flow_action_rss {
> > >  	uint32_t queue_num; /**< Number of entries in @p queue. */
> > >  	const uint8_t *key; /**< Hash key. */
> > >  	const uint16_t *queue; /**< Queue indices to use. */
> > > +	struct rte_flow_item *inputset; /** Provide more specific inputset
> > configuration.
> > > +					 * ignore spec, only mask.
> > > +					 */
> > >  };
> > >
> > >  /**
> > 
> > To make sure I understand, is this kind of a more flexible version of
> > rte_flow_action_rss.types?
> 
> Yes
> > 
> > For instance while specifying .types = ETH_RSS_IPV4 normally covers both
> > source and destination addresses, does this approach enable users to perform
> > RSS on source IP only? 
> 
> Yes, .it is the case to select any subset of 5 tuples or even tunnel header's id for hash
> 
> > In which case, what value does the Toeplitz algorithm
> > assume for the destination, 0x0? (note: must be documented)
> 
> My understanding is src/dst pair is only required for a symmetric case
> But for Toeplitz, it is just a hash function, it process a serial of data with specific algorithm, have no idea about which part is src and dst , 
> So for ip src only with Toeplitz, dst is not required to be a placeholder..

Right, I had symmetric Toeplitz in mind and wondered what would happen when
users would not select the required fields. I guess the PMD would have to
reject unsupported combinations.

> anything I missed, would you share more insight?

No, that answers the question, thanks.

Now what about my suggestion below? In short: extending ETH_RSS_* assuming
there's enough bits left in there, instead of adding a whole new framework
and breaking ABI in the process.

> > My opinion is that, unless you know of a hardware which can perform RSS on
> > random bytes of a packet, this approach is a bit overkill at this point.
> > 
> > How about simply adding the needed ETH_RSS_* definitions (e.g.
> > ETH_RSS_IPV4_(SRC|DST))? How many are needed?
> > 
> > There are currently 20 used bits and struct rte_flow_action_rss.types is 64-bit
> > wide. I'm sure we can manage something without harming the ABI. Even
> > better, you wouldn't need a deprecation notice.
> > 
> > If you use the suggested approach, please update testpmd and its
> > documentation as part of the same patch, thanks.
  

Hi Adrien:

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, July 4, 2019 10:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Su, Simei <simei.su@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] ethdev: support input set change by RSS action
> 
> On Thu, Jul 04, 2019 at 01:55:14PM +0000, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Thursday, July 4, 2019 5:07 PM
> > > To: Su, Simei <simei.su@intel.com>
> > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Yang,
> > > Qiming <qiming.yang@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [RFC] ethdev: support input set change by
> > > RSS action
> > >
> > > On Thu, Jul 04, 2019 at 12:47:09PM +0800, simei wrote:
> > > > From: Simei Su <simei.su@intel.com>
> > > >
> > > > This RFC introduces inputset structure to rte_flow_action_rss to
> > > > support input set specific configuration by rte_flow RSS action.
> > > >
> > > > We can give an testpmd command line example to make it more clear.
> > > >
> > > > For example, below flow selects the l4 port as inputset for any
> > > > eth/ipv4/tcp packet: #flow create 0 ingress pattern eth / ipv4 /
> > > > tcp / end actions rss inputset tcp src mask 0xffff dst mask 0xffff
> > > > /end
> > > >
> > > > Signed-off-by: Simei Su <simei.su@intel.com>
> > > > ---
> > > >  lib/librte_ethdev/rte_flow.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > b/lib/librte_ethdev/rte_flow.h index f3a8fb1..2a455b6 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -1796,6 +1796,9 @@ struct rte_flow_action_rss {
> > > >  	uint32_t queue_num; /**< Number of entries in @p queue. */
> > > >  	const uint8_t *key; /**< Hash key. */
> > > >  	const uint16_t *queue; /**< Queue indices to use. */
> > > > +	struct rte_flow_item *inputset; /** Provide more specific
> > > > +inputset
> > > configuration.
> > > > +					 * ignore spec, only mask.
> > > > +					 */
> > > >  };
> > > >
> > > >  /**
> > >
> > > To make sure I understand, is this kind of a more flexible version
> > > of rte_flow_action_rss.types?
> >
> > Yes
> > >
> > > For instance while specifying .types = ETH_RSS_IPV4 normally covers
> > > both source and destination addresses, does this approach enable
> > > users to perform RSS on source IP only?
> >
> > Yes, .it is the case to select any subset of 5 tuples or even tunnel
> > header's id for hash
> >
> > > In which case, what value does the Toeplitz algorithm assume for the
> > > destination, 0x0? (note: must be documented)
> >
> > My understanding is src/dst pair is only required for a symmetric case
> > But for Toeplitz, it is just a hash function, it process a serial of
> > data with specific algorithm, have no idea about which part is src and dst , So
> for ip src only with Toeplitz, dst is not required to be a placeholder..
> 
> Right, I had symmetric Toeplitz in mind and wondered what would happen
> when users would not select the required fields. I guess the PMD would have
> to reject unsupported combinations.
> 
> > anything I missed, would you share more insight?
> 
> No, that answers the question, thanks.
> 
> Now what about my suggestion below? In short: extending ETH_RSS_*
> assuming there's enough bits left in there, instead of adding a whole new
> framework and breaking ABI in the process.

Since the hardware can support any combination of input set (not just 5 tuples), we'd like to make it more generic.
Those will be some pain due to ABI break, but we think its worth for long term.

Thanks
Qi

> 
> > > My opinion is that, unless you know of a hardware which can perform
> > > RSS on random bytes of a packet, this approach is a bit overkill at this
> point.
> > >
> > > How about simply adding the needed ETH_RSS_* definitions (e.g.
> > > ETH_RSS_IPV4_(SRC|DST))? How many are needed?
> > >
> > > There are currently 20 used bits and struct
> > > rte_flow_action_rss.types is 64-bit wide. I'm sure we can manage
> > > something without harming the ABI. Even better, you wouldn't need a
> deprecation notice.
> > >
> > > If you use the suggested approach, please update testpmd and its
> > > documentation as part of the same patch, thanks.
> 
> --
> Adrien Mazarguil
> 6WIND
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Zhang, Qi Z
> Sent: Tuesday, July 9, 2019 11:11 AM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Su, Simei <simei.su@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] ethdev: support input set change by RSS action
> > Right, I had symmetric Toeplitz in mind and wondered what would happen
> > when users would not select the required fields. I guess the PMD would
> > have to reject unsupported combinations.
> >
> > > anything I missed, would you share more insight?
> >
> > No, that answers the question, thanks.
> >
> > Now what about my suggestion below? In short: extending ETH_RSS_*
> > assuming there's enough bits left in there, instead of adding a whole
> > new framework and breaking ABI in the process.
> 
> Since the hardware can support any combination of input set (not just 5
> tuples), we'd like to make it more generic.
> Those will be some pain due to ABI break, but we think its worth for long
> term.

# IMO, The decided method should be compatible with normal RSS(without rte_flow) case 
as well(Currently is struct rte_eth_conf.rx_adv_conf.rss_conf.rss_hf used 
in the dev_configure()) as from HW perspective it will be same  the HW block which does 
both.
# From Marvell HW point of view, We can support any combination of input set.
But it is more of a micro code(i.e need to first provide the list protocol supported to HW)
and then use it latter. So, I think, extending the ETH_RSS_* would be more portable
approach __without loosing__ the functionality. Since there is around 40 bits are left, I think
more standard protocol can fit in the 40 bits.