diff mbox series

[RFC] ethdev: add GRE optional fields to flow API

Message ID 20190514071829.5251-1-jackmin@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [RFC] ethdev: add GRE optional fields to flow API |

Checks

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

Commit Message

Xiaoyu Min May 14, 2019, 7:18 a.m. UTC 
  Add GRE's checksum, key, and sequence field to the
struct rte_flow_item_gre in order to match.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 lib/librte_ethdev/rte_flow.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andrew Rybchenko May 14, 2019, 7:34 a.m. UTC | #1 
On 5/14/19 10:18 AM, Xiaoyu Min wrote:
> Add GRE's checksum, key, and sequence field to the
> struct rte_flow_item_gre in order to match.
>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> ---
>   lib/librte_ethdev/rte_flow.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 63f84fca65..fb04af3268 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
>   	 */
>   	rte_be16_t c_rsvd0_ver;
>   	rte_be16_t protocol; /**< Protocol type. */
> +	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> +	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> +	rte_be32_t key; /**< application specific key value, optional. */
> +	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
>   };
>   
>   /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */

What is the purpose to match checksum, reserved and sequence number?
Adrien Mazarguil May 14, 2019, 9 a.m. UTC | #2 
On Tue, May 14, 2019 at 10:34:22AM +0300, Andrew Rybchenko wrote:
> On 5/14/19 10:18 AM, Xiaoyu Min wrote:
> > Add GRE's checksum, key, and sequence field to the
> > struct rte_flow_item_gre in order to match.
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > ---
> >   lib/librte_ethdev/rte_flow.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 63f84fca65..fb04af3268 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
> >   	 */
> >   	rte_be16_t c_rsvd0_ver;
> >   	rte_be16_t protocol; /**< Protocol type. */
> > +	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> > +	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> > +	rte_be32_t key; /**< application specific key value, optional. */
> > +	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
> >   };
> >   /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */
> 
> What is the purpose to match checksum, reserved and sequence number?

I think it's not really an issue, this structure only describes a packet
header as found on the wire like other pattern items; rte_flow users only
have to provide a mask to select the fields to be matched.

However you can't just modify an existing public structure without going
through the lengthy API/ABI deprecation/versioning process.

The reason these fields were not initially part of rte_flow_item_gre is that
each of them is optional, meaning the GRE header has variable length.
They should be handled through separate objects like IPv6 options (struct
rte_flow_item_ipv6_ext), ARP (struct rte_flow_item_arp_eth_ipv4) or ICMPv6
neighbor discovery (struct rte_flow_item_icmp6_nd_opt), either all together
e.g.:

 RTE_FLOW_ITEM_TYPE_GRE_OPTS

 struct rte_flow_item_gre_opts {
     rte_be16_t checksum; /**< Checksum for GRE header and payload (C bit). */
     rte_be16_t rsvd1; /**< Reserved bits (C bit). */
     rte_be32_t key; /**< Application specific key value (K bit). */
     rte_be32_t sequence; /**< Sequence number for GRE packet (S bit). */
 };

Or separately, since I guess only key matters no need to define the others:

 RTE_FLOW_ITEM_TYPE_GRE_KEY

 struct rte_flow_item_gre_key {
     rte_be32_t key; /**< Application specific key value (K bit). */
 };

In both cases, the default mask for this object should cover "key". Make
sure to update documentation and testpmd in the same patch.
Stephen Hemminger May 14, 2019, 3:25 p.m. UTC | #3 
On Tue, 14 May 2019 10:18:29 +0300
Xiaoyu Min <jackmin@mellanox.com> wrote:

> Add GRE's checksum, key, and sequence field to the
> struct rte_flow_item_gre in order to match.
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> ---
>  lib/librte_ethdev/rte_flow.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 63f84fca65..fb04af3268 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
>  	 */
>  	rte_be16_t c_rsvd0_ver;
>  	rte_be16_t protocol; /**< Protocol type. */
> +	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> +	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> +	rte_be32_t key; /**< application specific key value, optional. */
> +	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
>  };
>  
>  /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */

This breaks ABI.

To extend you need to add a new flow item type and keep old
one as legacy.
Xiaoyu Min May 15, 2019, 8:55 a.m. UTC | #4 
On Tue, 19-05-14, 11:00, Adrien Mazarguil wrote:
> On Tue, May 14, 2019 at 10:34:22AM +0300, Andrew Rybchenko wrote:
> > On 5/14/19 10:18 AM, Xiaoyu Min wrote:
> > > Add GRE's checksum, key, and sequence field to the
> > > struct rte_flow_item_gre in order to match.
> > > 
> > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > > ---
> > >   lib/librte_ethdev/rte_flow.h | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index 63f84fca65..fb04af3268 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
> > >   	 */
> > >   	rte_be16_t c_rsvd0_ver;
> > >   	rte_be16_t protocol; /**< Protocol type. */
> > > +	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> > > +	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> > > +	rte_be32_t key; /**< application specific key value, optional. */
> > > +	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
> > >   };
> > >   /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */
> > 
> > What is the purpose to match checksum, reserved and sequence number?
> 
> I think it's not really an issue, this structure only describes a packet
> header as found on the wire like other pattern items; rte_flow users only
> have to provide a mask to select the fields to be matched.
> 
> However you can't just modify an existing public structure without going
> through the lengthy API/ABI deprecation/versioning process.
> 
> The reason these fields were not initially part of rte_flow_item_gre is that
> each of them is optional, meaning the GRE header has variable length.
> They should be handled through separate objects like IPv6 options (struct
> rte_flow_item_ipv6_ext), ARP (struct rte_flow_item_arp_eth_ipv4) or ICMPv6
> neighbor discovery (struct rte_flow_item_icmp6_nd_opt), either all together
> e.g.:
> 
>  RTE_FLOW_ITEM_TYPE_GRE_OPTS
> 
>  struct rte_flow_item_gre_opts {
>      rte_be16_t checksum; /**< Checksum for GRE header and payload (C bit). */
>      rte_be16_t rsvd1; /**< Reserved bits (C bit). */
>      rte_be32_t key; /**< Application specific key value (K bit). */
>      rte_be32_t sequence; /**< Sequence number for GRE packet (S bit). */
>  };
> 
> Or separately, since I guess only key matters no need to define the others:
> 
>  RTE_FLOW_ITEM_TYPE_GRE_KEY
> 
>  struct rte_flow_item_gre_key {
>      rte_be32_t key; /**< Application specific key value (K bit). */
>  };

I will go to this approach.

> 
> In both cases, the default mask for this object should cover "key". Make
> sure to update documentation and testpmd in the same patch.
> 

Sure, I will do.
Thank you, adrien, for your detail explaination.

I'll send v2 RFC soon.
> -- 
> Adrien Mazarguil
> 6WIND
Xiaoyu Min May 15, 2019, 8:56 a.m. UTC | #5 
On Tue, 19-05-14, 08:25, Stephen Hemminger wrote:
> On Tue, 14 May 2019 10:18:29 +0300
> Xiaoyu Min <jackmin@mellanox.com> wrote:
> 
> > Add GRE's checksum, key, and sequence field to the
> > struct rte_flow_item_gre in order to match.
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 63f84fca65..fb04af3268 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
> >  	 */
> >  	rte_be16_t c_rsvd0_ver;
> >  	rte_be16_t protocol; /**< Protocol type. */
> > +	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> > +	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> > +	rte_be32_t key; /**< application specific key value, optional. */
> > +	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
> >  };
> >  
> >  /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */
> 
> This breaks ABI.
> 
> To extend you need to add a new flow item type and keep old
> one as legacy.
OK, I'll add a new flow item.

Thank you.

-Jack
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 63f84fca65..fb04af3268 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -847,6 +847,10 @@  struct rte_flow_item_gre {
 	 */
 	rte_be16_t c_rsvd0_ver;
 	rte_be16_t protocol; /**< Protocol type. */
+	rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
+	rte_be16_t rsvd1; /**< present when C bit is set, optional. */
+	rte_be32_t key; /**< application specific key value, optional. */
+	rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */