RE: [RFC 1/3] ethdev: support GRE optional fields

["Sean Zhang (Networking SW)" <xiazhang@nvidia.com>]

Hi,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Wednesday, January 19, 2022 6:57 PM
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> 
> Hi,
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> >
> > 19/01/2022 10:53, Ferruh Yigit:
> > > On 12/30/2021 3:08 AM, Sean Zhang wrote:
> > > > --- a/lib/ethdev/rte_flow.h
> > > > +++ b/lib/ethdev/rte_flow.h
> > > >   /**
> > > > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > > > + *
> > > > + * Matches GRE optional fields in header.
> > > > + */
> > > > +struct rte_gre_hdr_option {
> > > > +	rte_be16_t checksum;
> > > > +	rte_be32_t key;
> > > > +	rte_be32_t sequence;
> > > > +};
> > > > +
> > >
> > > Hi Ori, Andrew,
> > >
> > > The decision was to have protocol structs in the net library and
> > > flow structs use from there, wasn't it?
> > > (Btw, a deprecation notice is still pending to clear some existing
> > > ones)
> > >
> > > So for the GRE optional fields, what about having a struct in the
> 'rte_gre.h'?
> > > (Also perhaps an GRE extended protocol header can be defined
> > > combining 'rte_gre_hdr' and optional fields struct.) Later flow API
> > > struct can embed that struct.
> >
> > +1 for using librte_net.
> > This addition in rte_flow looks to be a mistake.
> > Please fix the next version.
> >
> Nice idea,
> but my main concern is that the header should have the header is defined.
> Since some of the fields are optional this will look something like this:
> gre_hdr_option_checksum {
> rte_be_16_t checksum;
> }
> 
> gre_hdr_option_key {
> rte_be_32_t key;
> }
> 
> gre_hdr_option_ sequence {
> rte_be_32_t sequence;
> }
> 
> I don't want to have so many rte_flow_items, Has more and more protocols
> have optional data it doesn't make sense to create the item for each.
> 
> If I'm looking at it from an ideal place, I would like that the optional fields will
> be part of the original item.
> For example in test pmd I would like to write:
> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
> is yyy key is xxx / end This means that the structure will look like this:
> struct rte_flow_item_gre {
> 	union {
> 		struct {
> 			/**
> 		 	* Checksum (1b), reserved 0 (12b), version (3b).
> 			 * Refer to RFC 2784.
> 			 */
> 			rte_be16_t c_rsvd0_ver;
> 			rte_be16_t protocol; /**< Protocol type. */
> 		}
> 		struct rte_gre_hdr hdr
> 	}
> 	rte_be_16_t checksum;
> 	rte_be_32_t key;
> 	rte_be_32_t sequence;
> };
> The main issue with this is that it breaks ABI, Maybe to solve this we can
> create a new structure gre_ext?
> 
> In any way I think we should think how we allow adding members to
> structures without ABI breakage.
> 
> Best,
> Ori

Thanks for the comments and suggestion.
So the acceptable solution is to have new structs define in rte_gre.h?
struct gre_hdr_opt_checksum {
	rte_be_16_t checksum;
}
 
struct gre_hdr_opt_key {
	rte_be_32_t key;
}
 
struct gre_hdr_opt_ sequence {
	rte_be_32_t sequence;
}

And to add new struct gre_ext defined in rte_flow.h:
struct gre_ext {
	struct rte_gre_hdr hdr;
	struct gre_hdr_opt_checkum checksum;
	struct rte_hdr_opt_key key;
	struct rte_hdr_opt_seq seq;
};

And we use struct gre_ext for this new added flow item gre_option.

Correct me if my understanding is not right.

Thanks,
Sean