Hi,
> -----Original Message-----
> From: Thomas Monjalon <tho...@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
