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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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;
}

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