> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Gregory Etelson > > Sent: Thursday, 27 May 2021 17.29 > and version fields > > > > RTE IPv4 header definition combines the `version' and `ihl' fields > > into a single structure member. > > This patch introduces dedicated structure members for both `version' > > and `ihl' IPv4 fields. Separated header fields definitions allow to > > create simplified code to match on the IHL value in a flow rule. > > The original `version_ihl' structure member is kept for backward > > compatibility. > > > > Signed-off-by: Gregory Etelson <getel...@nvidia.com> > > --- > > app/test/test_flow_classify.c | 8 ++++---- > > lib/net/rte_ip.h | 16 +++++++++++++++- > > 2 files changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/app/test/test_flow_classify.c > > b/app/test/test_flow_classify.c > > index 951606f248..4f64be5357 100644 > > --- a/app/test/test_flow_classify.c > > +++ b/app/test/test_flow_classify.c > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def > > ipv4_defs[NUM_FIELDS_IPV4] = { > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end" > > */ > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = { > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0, > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0, > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} > > }; > > static const struct rte_flow_item_ipv4 ipv4_mask_24 = { > > @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = { > > RTE_FLOW_ITEM_TYPE_END, > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" > > */ > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = { > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0, > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0, > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} > > }; > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = { > > RTE_FLOW_ITEM_TYPE_TCP, > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end" > > */ > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = { > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14), > > - RTE_IPV4(15, 16, 17, 18)} > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)} > > }; > > > > static struct rte_flow_item_sctp sctp_spec_1 = { > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h > > index 4b728969c1..684bb028b2 100644 > > --- a/lib/net/rte_ip.h > > +++ b/lib/net/rte_ip.h > > @@ -38,7 +38,21 @@ extern "C" { > > * IPv4 Header > > */ > > struct rte_ipv4_hdr { > > - uint8_t version_ihl; /**< version and header length */ > > + __extension__ > > + union { > > + uint8_t version_ihl; /**< version and header length */ > > + struct { > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > + uint8_t ihl:4; > > + uint8_t version:4; > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > + uint8_t version:4; > > + uint8_t ihl:4; > > +#else > > +#error "setup endian definition" > > +#endif > > + }; > > + }; > > uint8_t type_of_service; /**< type of service */ > > rte_be16_t total_length; /**< length of packet */ > > rte_be16_t packet_id; /**< packet ID */ > > -- > > 2.31.1 > > > > This does not break the ABI, but it could be discussed if it breaks the API > due to the required structure initialization changes shown in > test_flow_classify.c.
Yep, I guess it might be classified as API change. Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field: type_of_service, fragment_offset. If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too. But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields. Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead? > I think this patch is an improvement, and that such structure modifications > should be generally accepted, so: > > Acked-by: Morten Brørup <m...@smartsharesystems.com>
