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

[Ori Kam <orika@nvidia.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> 
> On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
> > 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.
> >
> 
> What about having a struct for 'options' and use in in flow item for options,
> like:
> 
> struct gre_hdr_opt {
>    struct gre_hdr_opt_checkum checksum;
>    struct rte_hdr_opt_key key;
>    struct rte_hdr_opt_seq seq;
> }
> 
> struct gre_hdr_ext {
>    struct rte_gre_hdr hdr;
>    struct gre_hdr_opt;
> }
> 
> struct rte_flow_item_gre_opt {
>    struct gre_hdr_opt hdr;
> }

Fom my understanding the header should reflect structures
as they appear in the spec.

If we look at the spec, from my understanding each of those items is stand-alone.
It is possible to have just key or key and seq or any other combination.
So the struct you suggested is not valid struct in gre header.

If you are O.K with adding such a struct to the gre file I will also be O.K with it.

Best,
Ori
> 
> > Correct me if my understanding is not right.
> >
> > Thanks,
> > Sean
> >
> >