From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F23F0A0547; Fri, 28 May 2021 12:52:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8212340143; Fri, 28 May 2021 12:52:57 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 903A940040 for ; Fri, 28 May 2021 12:52:55 +0200 (CEST) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Fri, 28 May 2021 12:52:52 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Thread-Index: AQHXUw1e6pIatJvRL0OXGQrSmeIttar3e60AgAEsi7CAAAzA0A== References: <20210527152858.13312-1-getelson@nvidia.com> <98CBD80474FA8B44BF855DF32C47DC35C617E2@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Ananyev, Konstantin" , "Gregory Etelson" , Cc: , , , "Iremonger, Bernard" , "Olivier Matz" Subject: Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, > Konstantin > Sent: Friday, 28 May 2021 12.21 >=20 > > > From: dev [mailto:dev-bounces@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 > > > --- > > > 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] =3D { > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end" > > > */ > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 =3D { > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0, > > > + { { .version_ihl =3D 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 =3D { > > > @@ -131,7 +131,7 @@ static struct rte_flow_item end_item =3D { > > > 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 =3D { > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0, > > > + { { .version_ihl =3D 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 =3D { > > > 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 =3D { > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14), > > > - RTE_IPV4(15, 16, 17, 18)} > > > + { { .version_ihl =3D 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 =3D { > > > 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 =3D=3D RTE_LITTLE_ENDIAN > > > + uint8_t ihl:4; > > > + uint8_t version:4; > > > +#elif RTE_BYTE_ORDER =3D=3D 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. >=20 > 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? >=20 Let's please not introduce accessor macros for bitfields. If we don't = introduce bitfields like these, I would rather stick with the current = _MASK, _SHIFT and _FLAG defines. Yes, this change will lead to the introduction of more bitfields, both = here and in other places. We already accepted it in the eCPRI structure = (/lib/net/rte_ecpri.h), so why not just generally accept it. Are modern compilers really worse at handling a bitfield defined like = this, compared to handling a single uint8_t with hand coding? I consider = your concern very important, so I'm only asking if it is still relevant, = to avoid making decisions based on past experience that might be = outdated. (I admit to falling into that trap myself, once in a while.) > > I think this patch is an improvement, and that such structure > modifications should be generally accepted, so: > > > > Acked-by: Morten Br=F8rup >=20