[RFC] ethdev: add IPv4/IPv6 DSCP rewrite action

  For some overlay network, such as VXLAN, the DSCP field in the new outer
IP header after VXLAN decapsulation may need to be updated accordingly.

This commit introduce the DSCP modify action for IPv4 and IPv6.

Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
---
 doc/guides/prog_guide/rte_flow.rst | 40 ++++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_flow.h       | 31 +++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)
  

On 12/10/19 8:23 AM, Suanming Mou wrote:
> For some overlay network, such as VXLAN, the DSCP field in the new outer
> IP header after VXLAN decapsulation may need to be updated accordingly.
> 
> This commit introduce the DSCP modify action for IPv4 and IPv6.
> 
> Signed-off-by: Suanming Mou <suanmingm@mellanox.com>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

as usual it requires testpmd support and a driver which
supports it (I understand that it may be omitted in RFC).

Few minor notes below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 40 ++++++++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_flow.h       | 31 +++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index a254c81..54b9250 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2558,6 +2558,46 @@ the other path depending on HW capability.
>     | ``mask`` | bit-mask applies to "data" |
>     +----------+----------------------------+
>  
> +Action: ``SET_IPV4_DSCP``
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Set IPv4DSCP.

I think it is better to have space between IPv4 and DSCP above.

> +
> +Modify DSCP in IPv4 header.
> +
> +It must be used with RTE_FLOW_ITEM_TYPE_IPV4 in pattern.
> +Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned.
> +
> +.. _table_rte_flow_action_set_ipv4_dscp:
> +
> +.. table:: SET_IPV4_DSCP
> +
> +   +-----------+---------------------------------+
> +   | Field     | Value                           |
> +   +===========+=================================+
> +   | ``dscp``  | DSCP in low 6 bits, rest ingore |

ingore -> ignore

> +   +-----------+---------------------------------+
> +
> +Action: ``SET_IPV6_DSCP``
> +^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Set IPv6DSCP.

Same.

> +
> +Modify DSCP in IPv6 header.
> +
> +It must be used with RTE_FLOW_ITEM_TYPE_IPV6 in pattern.
> +Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned.
> +
> +.. _table_rte_flow_action_set_ipv6_dscp:
> +
> +.. table:: SET_IPV6_DSCP
> +
> +   +-----------+---------------------------------+
> +   | Field     | Value                           |
> +   +===========+=================================+
> +   | ``dscp``  | DSCP in low 6 bits, rest ingore |

ingore -> ignore

> +   +-----------+---------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 452d359..76bf080 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2004,6 +2004,26 @@ enum rte_flow_action_type {
>  	 * See struct rte_flow_action_set_meta.
>  	 */
>  	RTE_FLOW_ACTION_TYPE_SET_META,
> +
> +	/**
> +	 * Modify IPv4 DSCP in the outermost IP header.
> +	 *
> +	 * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_IPV4,
> +	 * then the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> +	 *
> +	 * See struct rte_flow_action_set_dscp.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP,
> +
> +	/**
> +	 * Modify IPv6 DSCP in the outermost IP header.
> +	 *
> +	 * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_IPV6,
> +	 * then the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> +	 *
> +	 * See struct rte_flow_action_set_dscp.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP,
>  };
>  
>  /**
> @@ -2530,6 +2550,17 @@ struct rte_flow_action_set_meta {
>  	uint32_t mask;
>  };
>  
> +/**
> + * RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
> + * RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP
> + *
> + * Set the DSCP value for IPv4/IPv6 header.
> + * DSCP in low 6 bits, rest ignored.
> + */
> +struct rte_flow_action_set_dscp {
> +	uint8_t dscp;
> +};
> +
>  /* Mbuf dynamic field offset for metadata. */
>  extern int rte_flow_dynf_metadata_offs;
>  
>
  

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, December 10, 2019 3:33 PM
> To: Suanming Mou <suanmingm@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; John McNamara <john.mcnamara@intel.com>;
> Marko Kovacevic <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Ori Kam
> <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC] ethdev: add IPv4/IPv6 DSCP rewrite action
> 
> On 12/10/19 8:23 AM, Suanming Mou wrote:
> > For some overlay network, such as VXLAN, the DSCP field in the new
> > outer IP header after VXLAN decapsulation may need to be updated
> accordingly.
> >
> > This commit introduce the DSCP modify action for IPv4 and IPv6.
> >
> > Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> as usual it requires testpmd support and a driver which supports it (I understand
> that it may be omitted in RFC).

Thanks. Yes, it will be added later.

> 
> Few minor notes below.

Will update the second version fix these.

> 
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 40
> ++++++++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_flow.h       | 31 +++++++++++++++++++++++++++++
> >  2 files changed, 71 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index a254c81..54b9250 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2558,6 +2558,46 @@ the other path depending on HW capability.
> >     | ``mask`` | bit-mask applies to "data" |
> >     +----------+----------------------------+
> >
> > +Action: ``SET_IPV4_DSCP``
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Set IPv4DSCP.
> 
> I think it is better to have space between IPv4 and DSCP above.
> 
> > +
> > +Modify DSCP in IPv4 header.
> > +
> > +It must be used with RTE_FLOW_ITEM_TYPE_IPV4 in pattern.
> > +Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned.
> > +
> > +.. _table_rte_flow_action_set_ipv4_dscp:
> > +
> > +.. table:: SET_IPV4_DSCP
> > +
> > +   +-----------+---------------------------------+
> > +   | Field     | Value                           |
> > +   +===========+=================================+
> > +   | ``dscp``  | DSCP in low 6 bits, rest ingore |
> 
> ingore -> ignore
> 
> > +   +-----------+---------------------------------+
> > +
> > +Action: ``SET_IPV6_DSCP``
> > +^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Set IPv6DSCP.
> 
> Same.
> 
> > +
> > +Modify DSCP in IPv6 header.
> > +
> > +It must be used with RTE_FLOW_ITEM_TYPE_IPV6 in pattern.
> > +Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned.
> > +
> > +.. _table_rte_flow_action_set_ipv6_dscp:
> > +
> > +.. table:: SET_IPV6_DSCP
> > +
> > +   +-----------+---------------------------------+
> > +   | Field     | Value                           |
> > +   +===========+=================================+
> > +   | ``dscp``  | DSCP in low 6 bits, rest ingore |
> 
> ingore -> ignore
> 
> > +   +-----------+---------------------------------+
> > +
> >  Negative types
> >  ~~~~~~~~~~~~~~
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index 452d359..76bf080 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2004,6 +2004,26 @@ enum rte_flow_action_type {
> >  	 * See struct rte_flow_action_set_meta.
> >  	 */
> >  	RTE_FLOW_ACTION_TYPE_SET_META,
> > +
> > +	/**
> > +	 * Modify IPv4 DSCP in the outermost IP header.
> > +	 *
> > +	 * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_IPV4,
> > +	 * then the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION
> error.
> > +	 *
> > +	 * See struct rte_flow_action_set_dscp.
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP,
> > +
> > +	/**
> > +	 * Modify IPv6 DSCP in the outermost IP header.
> > +	 *
> > +	 * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_IPV6,
> > +	 * then the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION
> error.
> > +	 *
> > +	 * See struct rte_flow_action_set_dscp.
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP,
> >  };
> >
> >  /**
> > @@ -2530,6 +2550,17 @@ struct rte_flow_action_set_meta {
> >  	uint32_t mask;
> >  };
> >
> > +/**
> > + * RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
> > + * RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP
> > + *
> > + * Set the DSCP value for IPv4/IPv6 header.
> > + * DSCP in low 6 bits, rest ignored.
> > + */
> > +struct rte_flow_action_set_dscp {
> > +	uint8_t dscp;
> > +};
> > +
> >  /* Mbuf dynamic field offset for metadata. */  extern int
> > rte_flow_dynf_metadata_offs;
> >
> >
  

On Tue, 10 Dec 2019 10:33:28 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> > For some overlay network, such as VXLAN, the DSCP field in the new outer
> > IP header after VXLAN decapsulation may need to be updated accordingly.
> > 
> > This commit introduce the DSCP modify action for IPv4 and IPv6.
> > 
> > Signed-off-by: Suanming Mou <suanmingm@mellanox.com>  
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> as usual it requires testpmd support and a driver which
> supports it (I understand that it may be omitted in RFC).

And it requires documentation and a software implementation in the flow
classifier.


Plus you conveniently exclude defining what happens to reserved bits.
"What ever our hardware does is correct" is not a useful answer.
You need to be precise and limited in what is allowed to make this usable.

Sorry, to be so negative. This feature is fine in itself and a useful
incremental improvement. But nobody has stepped up to address the usability
of rte_flow.
  

On Tue, 10 Dec 2019 08:55:40 +0000
Suanming Mou <suanmingm@mellanox.com> wrote:

> > -----Original Message-----
> > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > Sent: Tuesday, December 10, 2019 3:33 PM
> > To: Suanming Mou <suanmingm@mellanox.com>; Adrien Mazarguil
> > <adrien.mazarguil@6wind.com>; John McNamara <john.mcnamara@intel.com>;
> > Marko Kovacevic <marko.kovacevic@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> > Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Ori Kam
> > <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>; Slava Ovsiienko
> > <viacheslavo@mellanox.com>
> > Subject: Re: [dpdk-dev] [RFC] ethdev: add IPv4/IPv6 DSCP rewrite action
> > 
> > On 12/10/19 8:23 AM, Suanming Mou wrote:  
> > > For some overlay network, such as VXLAN, the DSCP field in the new
> > > outer IP header after VXLAN decapsulation may need to be updated  
> > accordingly.  
> > >
> > > This commit introduce the DSCP modify action for IPv4 and IPv6.
> > >
> > > Signed-off-by: Suanming Mou <suanmingm@mellanox.com>  
> > 
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > 
> > as usual it requires testpmd support and a driver which supports it (I understand
> > that it may be omitted in RFC).  
> 
> Thanks. Yes, it will be added later.

Then this patch must not be applied until one or more drivers is ready to use it.
Very easy to get API and semantics wrong, until it is there.
  

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, December 10, 2019 8:32 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Suanming Mou <suanmingm@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Shahaf Shuler
> <shahafs@mellanox.com>; Ori Kam <orika@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; Slava Ovsiienko <viacheslavo@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC] ethdev: add IPv4/IPv6 DSCP rewrite action
> 
> On Tue, 10 Dec 2019 10:33:28 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> > > For some overlay network, such as VXLAN, the DSCP field in the new
> outer
> > > IP header after VXLAN decapsulation may need to be updated
> accordingly.
> > >
> > > This commit introduce the DSCP modify action for IPv4 and IPv6.
> > >
> > > Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> >
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >
> > as usual it requires testpmd support and a driver which
> > supports it (I understand that it may be omitted in RFC).
> 
> And it requires documentation and a software implementation in the flow
> classifier.
> 

Why in the flow classifier?
I don't remember any new code that was added to rte_flow was also added to flow classifier.

> 
> Plus you conveniently exclude defining what happens to reserved bits.
> "What ever our hardware does is correct" is not a useful answer.
> You need to be precise and limited in what is allowed to make this usable.
> 

The action does just what it says nothing more and nothing less. 
It just modify the DSCP value with a value given by the application.
Reserved bits are not touched.

> Sorry, to be so negative. This feature is fine in itself and a useful
> incremental improvement. But nobody has stepped up to address the
> usability
> of rte_flow.

I'm sorry but I don't understand your comment, 
According to my knowledge more and more applications are using rte_flow, and each new feature that is added is based on some 
customer need. 

Thanks,
Ori
  

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, December 11, 2019 2:32 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Suanming Mou <suanmingm@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; John McNamara <john.mcnamara@intel.com>;
> Marko Kovacevic <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org;
> Shahaf Shuler <shahafs@mellanox.com>; Ori Kam <orika@mellanox.com>;
> Matan Azrad <matan@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC] ethdev: add IPv4/IPv6 DSCP rewrite action
> 
> On Tue, 10 Dec 2019 10:33:28 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> > > For some overlay network, such as VXLAN, the DSCP field in the new
> > > outer IP header after VXLAN decapsulation may need to be updated
> accordingly.
> > >
> > > This commit introduce the DSCP modify action for IPv4 and IPv6.
> > >
> > > Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> >
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >
> > as usual it requires testpmd support and a driver which supports it (I
> > understand that it may be omitted in RFC).
> 
> And it requires documentation and a software implementation in the flow
> classifier.

Could you please give the previous example code which add to both of them?

> 
> 
> Plus you conveniently exclude defining what happens to reserved bits.
> "What ever our hardware does is correct" is not a useful answer.
> You need to be precise and limited in what is allowed to make this usable.

Sorry, I feel a little confused.
The DSCP only use 6 bits both in IPv4 and IPv6 header. The data struct is defined by the IP protocol, it is not defined by the hardware.
And the rest bits will be ignored as only 6 bits in the header. It is also added in the code comments.
The action is to apply the 6 bits DSCP value to the 6 bits DSCP field in IPv4 or IPv6 header.
Could you please point out the not precise part much more detail?

Thank you for the comments. Will try to make it better.

> 
> Sorry, to be so negative. This feature is fine in itself and a useful incremental
> improvement. But nobody has stepped up to address the usability of rte_flow.

It's hard to say,  it's just like some people like meat and some like vegetables.