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.
