On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
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.
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;
}
Correct me if my understanding is not right.
Thanks,
Sean
