Hi,
> -----Original Message-----
> From: Ori Kam <or...@nvidia.com>
> Sent: Wednesday, January 19, 2022 6:57 PM
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>;
> Sean Zhang (Networking SW) <xiazh...@nvidia.com>; Matan Azrad
> <ma...@nvidia.com>; Ferruh Yigit <ferruh.yi...@intel.com>
> Cc: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>; dev@dpdk.org
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>
> 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
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.
Correct me if my understanding is not right.
Thanks,
Sean
