[PATCH v6 4/8] ethdev: use GRE protocol struct for flow matching

[Ferruh Yigit]

On 2/3/2023 3:12 PM, Thomas Monjalon wrote:
> 03/02/2023 16:02, Ferruh Yigit:
>> On 2/2/2023 5:16 PM, Thomas Monjalon wrote:
>>> 02/02/2023 13:44, Ferruh Yigit:
>>>> --- a/lib/net/rte_gre.h
>>>> +++ b/lib/net/rte_gre.h
>>>> @@ -28,6 +28,8 @@ extern "C" {
>>>>   */
>>>>  __extension__
>>>>  struct rte_gre_hdr {
>>>> +       union {
>>>> +               struct {
>>>>  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>>>>         uint16_t res2:4; /**< Reserved */
>>>>         uint16_t s:1;    /**< Sequence Number Present bit */
>>>> @@ -45,6 +47,9 @@ struct rte_gre_hdr {
>>>>         uint16_t res3:5; /**< Reserved */
>>>>         uint16_t ver:3;  /**< Version Number */
>>>>  #endif
>>>> +               };
>>>> +               rte_be16_t c_rsvd0_ver;
>>>> +       };
>>>>         uint16_t proto;  /**< Protocol Type */
>>>
>>> Why adding an unioned field c_rsvd0_ver?
>>>
>>>
>>
>> Because existing usage in the drivers require to access these fields as
>> a single 16 bytes variable.
>>
>> like mlx was using it as:
>> `X(SET_BE16,	gre_c_ver, v->c_rsvd0_ver, rte_flow_item_gre) \`
>>
>> When all usage switched to flow item specific fields to generic headers,
>> there needs a way to represent this in the generic header.
>>
>> By adding 'c_rsvd0_ver' to generic header it becomes:
>> `X(SET_BE16,	gre_c_ver, v->hdr.c_rsvd0_ver, rte_flow_item_gre) \`
>>
>>
>> Or another sample, previous version of code was updated as following:
>> `
>>  -	size = sizeof(((struct rte_flow_item_gre *)NULL)->c_rsvd0_ver);
>>  +	size = sizeof(((struct rte_flow_item_gre *)NULL)->hdr.proto);
>> `
>>
>> Because generic field to represent 'c_rsvd0_ver' is missing, 'hdr.proto'
>> was used, this was wrong.
>> Although the sizes of fields are same and functionally works, they are
>> different fields, this is worse than sizeof(uint16_t);
>>
>>
>> Another usage in testpmd:
>> `
>>   	[ITEM_GRE_C_RSVD0_VER] = {
>>   		.name = "c_rsvd0_ver",
>>  @@ -4082,7 +4082,7 @@ static const struct token token_list[] = {
>>   		.next = NEXT(item_gre, NEXT_ENTRY(COMMON_UNSIGNED),
>>    			     item_param),
>>   		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
>>  -					     c_rsvd0_ver)),
>>  +					     hdr.c_rsvd0_ver)),
>> `
>>
>>
>> But looking it again perhaps it can be named differently, because it is
>> not a reserved field in the generic header, though I am not sure what
>> can be a good variable name.
> 
> The best would be to try not using this field at all.
> I suggest to remove this patch from the series for now.
> I could try to work on it for the next release.
> 

OK