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

[Ori Kam <orika@nvidia.com>]

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