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>
> Sent: Tuesday, January 25, 2022 4:29 PM
> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> 
> On 1/25/2022 1:06 PM, Ori Kam wrote:
> > 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 it is not valid header representation, please forget about it.
> 
> But this means initially suggested 'struct gre_ext' is wrong, right?
> 
> So should 'rte_flow_item_gre_opt' use separate structs, like:
> 
> struct rte_flow_item_gre_opt {
>    struct gre_hdr_opt_checkum checksum;
>    struct rte_hdr_opt_key key;
>    struct rte_hdr_opt_seq seq;
> }
> 
Yes this is the last suggestion from Sean, the only difference is that
he created a new item gre_ext that holds the struct that you listed above
and added the gre header. This means that from rte_flow thre is only one GRE item
(the old one can be deprecated) 

Ori
> 
> > 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
> >>>
> >>>
> >