DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
@ 2019-05-14  7:18 Xiaoyu Min
  2019-05-14  7:18 ` Xiaoyu Min
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Xiaoyu Min @ 2019-05-14  7:18 UTC (permalink / raw)
  To: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, jackmin

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. */
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  7:18 [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API Xiaoyu Min
@ 2019-05-14  7:18 ` Xiaoyu Min
  2019-05-14  7:34 ` Andrew Rybchenko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Xiaoyu Min @ 2019-05-14  7:18 UTC (permalink / raw)
  To: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, jackmin

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. */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  7:18 [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API Xiaoyu Min
  2019-05-14  7:18 ` Xiaoyu Min
@ 2019-05-14  7:34 ` Andrew Rybchenko
  2019-05-14  7:34   ` Andrew Rybchenko
  2019-05-14  9:00   ` Adrien Mazarguil
  2019-05-14 15:25 ` Stephen Hemminger
  2019-05-15 13:24 ` [dpdk-dev] [RFC v2] ethdev: add GRE key field " Xiaoyu Min
  3 siblings, 2 replies; 14+ messages in thread
From: Andrew Rybchenko @ 2019-05-14  7:34 UTC (permalink / raw)
  To: Xiaoyu Min, Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit; +Cc: dev

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  7:34 ` Andrew Rybchenko
@ 2019-05-14  7:34   ` Andrew Rybchenko
  2019-05-14  9:00   ` Adrien Mazarguil
  1 sibling, 0 replies; 14+ messages in thread
From: Andrew Rybchenko @ 2019-05-14  7:34 UTC (permalink / raw)
  To: Xiaoyu Min, Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit; +Cc: dev

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?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  7:34 ` Andrew Rybchenko
  2019-05-14  7:34   ` Andrew Rybchenko
@ 2019-05-14  9:00   ` Adrien Mazarguil
  2019-05-14  9:00     ` Adrien Mazarguil
  2019-05-15  8:55     ` Jack Min
  1 sibling, 2 replies; 14+ messages in thread
From: Adrien Mazarguil @ 2019-05-14  9:00 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: Xiaoyu Min, Thomas Monjalon, Ferruh Yigit, dev

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.

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  9:00   ` Adrien Mazarguil
@ 2019-05-14  9:00     ` Adrien Mazarguil
  2019-05-15  8:55     ` Jack Min
  1 sibling, 0 replies; 14+ messages in thread
From: Adrien Mazarguil @ 2019-05-14  9:00 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: Xiaoyu Min, Thomas Monjalon, Ferruh Yigit, dev

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.

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  7:18 [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API Xiaoyu Min
  2019-05-14  7:18 ` Xiaoyu Min
  2019-05-14  7:34 ` Andrew Rybchenko
@ 2019-05-14 15:25 ` Stephen Hemminger
  2019-05-14 15:25   ` Stephen Hemminger
  2019-05-15  8:56   ` Jack Min
  2019-05-15 13:24 ` [dpdk-dev] [RFC v2] ethdev: add GRE key field " Xiaoyu Min
  3 siblings, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2019-05-14 15:25 UTC (permalink / raw)
  To: Xiaoyu Min
  Cc: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14 15:25 ` Stephen Hemminger
@ 2019-05-14 15:25   ` Stephen Hemminger
  2019-05-15  8:56   ` Jack Min
  1 sibling, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2019-05-14 15:25 UTC (permalink / raw)
  To: Xiaoyu Min
  Cc: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14  9:00   ` Adrien Mazarguil
  2019-05-14  9:00     ` Adrien Mazarguil
@ 2019-05-15  8:55     ` Jack Min
  2019-05-15  8:55       ` Jack Min
  1 sibling, 1 reply; 14+ messages in thread
From: Jack Min @ 2019-05-15  8:55 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: Andrew Rybchenko, Thomas Monjalon, Ferruh Yigit, dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-15  8:55     ` Jack Min
@ 2019-05-15  8:55       ` Jack Min
  0 siblings, 0 replies; 14+ messages in thread
From: Jack Min @ 2019-05-15  8:55 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: Andrew Rybchenko, Thomas Monjalon, Ferruh Yigit, dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-14 15:25 ` Stephen Hemminger
  2019-05-14 15:25   ` Stephen Hemminger
@ 2019-05-15  8:56   ` Jack Min
  2019-05-15  8:56     ` Jack Min
  1 sibling, 1 reply; 14+ messages in thread
From: Jack Min @ 2019-05-15  8:56 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API
  2019-05-15  8:56   ` Jack Min
@ 2019-05-15  8:56     ` Jack Min
  0 siblings, 0 replies; 14+ messages in thread
From: Jack Min @ 2019-05-15  8:56 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Adrien Mazarguil, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [RFC v2] ethdev: add GRE key field to flow API
  2019-05-14  7:18 [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API Xiaoyu Min
                   ` (2 preceding siblings ...)
  2019-05-14 15:25 ` Stephen Hemminger
@ 2019-05-15 13:24 ` Xiaoyu Min
  2019-05-15 13:24   ` Xiaoyu Min
  3 siblings, 1 reply; 14+ messages in thread
From: Xiaoyu Min @ 2019-05-15 13:24 UTC (permalink / raw)
  To: Adrien Mazarguil, John McNamara, Marko Kovacevic,
	Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, jackmin

Add new rte_flow_item_gre_key in order to match the optional key field.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 v2:
 * new rte_flow_item_gre_key added instead of expanding existing
   rte_flow_item_gre.
 * updated document.

 doc/guides/prog_guide/rte_flow.rst |  9 +++++++++
 lib/librte_ethdev/rte_flow.c       |  1 +
 lib/librte_ethdev/rte_flow.h       | 27 +++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 937f52bce1..8b79818f44 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -980,6 +980,15 @@ Matches a GRE header.
 - ``protocol``: protocol type.
 - Default ``mask`` matches protocol only.
 
+Item: ``GRE_KEY``
+^^^^^^^^^^^^^^^^^
+
+Matches a GRE key field.
+This should be preceded by item ``GRE``
+
+- ``key``: key value.
+- Default ``mask`` matches key only.
+
 Item: ``FUZZY``
 ^^^^^^^^^^^^^^^
 
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 3277be1edb..1cd5f4d263 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -55,6 +55,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
 	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
 	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
+	MK_FLOW_ITEM(GRE_KEY, sizeof(struct rte_flow_item_gre_key)),
 	MK_FLOW_ITEM(FUZZY, sizeof(struct rte_flow_item_fuzzy)),
 	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
 	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 63f84fca65..b36c3b980a 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -289,6 +289,13 @@ enum rte_flow_item_type {
 	 */
 	RTE_FLOW_ITEM_TYPE_GRE,
 
+	/**
+	 * Matches a GRE optional key field.
+	 *
+	 * See struct rte_flow_item_gre_key.
+	 */
+	RTE_FLOW_ITEM_TYPE_GRE_KEY,
+
 	/**
 	 * [META]
 	 *
@@ -856,6 +863,26 @@ static const struct rte_flow_item_gre rte_flow_item_gre_mask = {
 };
 #endif
 
+/**
+ * RTE_FLOW_ITEM_GRE_KEY.
+ *
+ * Matches the presence of a GRE key.
+ *
+ * Normally preceding by:
+ *
+ * - RTE_FLOW_ITEM_TYPE_GRE
+ */
+struct rte_flow_item_gre_key {
+	rte_be32_t key; /**< Application specific key value (K bit). */
+};
+
+/** Default mask for RTE_FLOW_ITEM_TYPE_GRE_KEY. */
+#ifndef __cplusplus
+static const struct rte_flow_item_gre_key rte_flow_gre_key_mask = {
+	.key = RTE_BE32(UINT32_MAX),
+};
+#endif
+
 /**
  * RTE_FLOW_ITEM_TYPE_FUZZY
  *
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [RFC v2] ethdev: add GRE key field to flow API
  2019-05-15 13:24 ` [dpdk-dev] [RFC v2] ethdev: add GRE key field " Xiaoyu Min
@ 2019-05-15 13:24   ` Xiaoyu Min
  0 siblings, 0 replies; 14+ messages in thread
From: Xiaoyu Min @ 2019-05-15 13:24 UTC (permalink / raw)
  To: Adrien Mazarguil, John McNamara, Marko Kovacevic,
	Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, jackmin

Add new rte_flow_item_gre_key in order to match the optional key field.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 v2:
 * new rte_flow_item_gre_key added instead of expanding existing
   rte_flow_item_gre.
 * updated document.

 doc/guides/prog_guide/rte_flow.rst |  9 +++++++++
 lib/librte_ethdev/rte_flow.c       |  1 +
 lib/librte_ethdev/rte_flow.h       | 27 +++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 937f52bce1..8b79818f44 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -980,6 +980,15 @@ Matches a GRE header.
 - ``protocol``: protocol type.
 - Default ``mask`` matches protocol only.
 
+Item: ``GRE_KEY``
+^^^^^^^^^^^^^^^^^
+
+Matches a GRE key field.
+This should be preceded by item ``GRE``
+
+- ``key``: key value.
+- Default ``mask`` matches key only.
+
 Item: ``FUZZY``
 ^^^^^^^^^^^^^^^
 
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 3277be1edb..1cd5f4d263 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -55,6 +55,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
 	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
 	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
+	MK_FLOW_ITEM(GRE_KEY, sizeof(struct rte_flow_item_gre_key)),
 	MK_FLOW_ITEM(FUZZY, sizeof(struct rte_flow_item_fuzzy)),
 	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
 	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 63f84fca65..b36c3b980a 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -289,6 +289,13 @@ enum rte_flow_item_type {
 	 */
 	RTE_FLOW_ITEM_TYPE_GRE,
 
+	/**
+	 * Matches a GRE optional key field.
+	 *
+	 * See struct rte_flow_item_gre_key.
+	 */
+	RTE_FLOW_ITEM_TYPE_GRE_KEY,
+
 	/**
 	 * [META]
 	 *
@@ -856,6 +863,26 @@ static const struct rte_flow_item_gre rte_flow_item_gre_mask = {
 };
 #endif
 
+/**
+ * RTE_FLOW_ITEM_GRE_KEY.
+ *
+ * Matches the presence of a GRE key.
+ *
+ * Normally preceding by:
+ *
+ * - RTE_FLOW_ITEM_TYPE_GRE
+ */
+struct rte_flow_item_gre_key {
+	rte_be32_t key; /**< Application specific key value (K bit). */
+};
+
+/** Default mask for RTE_FLOW_ITEM_TYPE_GRE_KEY. */
+#ifndef __cplusplus
+static const struct rte_flow_item_gre_key rte_flow_gre_key_mask = {
+	.key = RTE_BE32(UINT32_MAX),
+};
+#endif
+
 /**
  * RTE_FLOW_ITEM_TYPE_FUZZY
  *
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-05-15 13:25 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14  7:18 [dpdk-dev] [RFC] ethdev: add GRE optional fields to flow API Xiaoyu Min
2019-05-14  7:18 ` Xiaoyu Min
2019-05-14  7:34 ` Andrew Rybchenko
2019-05-14  7:34   ` Andrew Rybchenko
2019-05-14  9:00   ` Adrien Mazarguil
2019-05-14  9:00     ` Adrien Mazarguil
2019-05-15  8:55     ` Jack Min
2019-05-15  8:55       ` Jack Min
2019-05-14 15:25 ` Stephen Hemminger
2019-05-14 15:25   ` Stephen Hemminger
2019-05-15  8:56   ` Jack Min
2019-05-15  8:56     ` Jack Min
2019-05-15 13:24 ` [dpdk-dev] [RFC v2] ethdev: add GRE key field " Xiaoyu Min
2019-05-15 13:24   ` Xiaoyu Min

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).