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

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

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


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