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 EFA88A0524; Mon, 31 May 2021 13:10:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7167640040; Mon, 31 May 2021 13:10:17 +0200 (CEST) Received: from NAM10-MW2-obe.outbound.protection.outlook.com (mail-mw2nam10on2069.outbound.protection.outlook.com [40.107.94.69]) by mails.dpdk.org (Postfix) with ESMTP id D10CD4003F for ; Mon, 31 May 2021 13:10:15 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IGKFIkKy8b5DJFESpvHZrsWvoA9fLmkR+TX0jxE7kOegxcU/kcXiFZjyPBhVFfZ/R70Xk8UIyAo+m2p2xcHP4/mTgdOD/NYMOfSpgHIOQ88UftTe4/b+w6yjvLo1/PexSw/a2V3sW/vr1Iqpi6XEqczGhGT8f4SALEtXOcl4ZZLlQDKSgI7RZdL419FOgpRMnkNvxOG78OuMfT2D0l+76ioVSBupcFVoPs6DIl3u/YE+OI6yrEmtTGk4auWX0eiR/bYw3poJmFXKFxVQjEaX6rWdHFAhOj0K3SC3v3zQ5cktzXiglJcGvrmgZCpxQqagPC4UJmWU6bDvC9fausSfhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=TCfu38ZuJE49gx10wQf2PXMCxjEjkyE2eIqznWpFyWI=; b=jQDgzJL6NZB/UXokbKFbukXkzZ8vDua9es4CHQQBu/r80c0LN2Z0Dg9f5qPzy6ILUkHmA6/fUOQqc9YekTwjPCe93Xxd2Iqnfo32jKEjHWYDltKx8Lvc5LTkpCaoy/8RoyyUKz/TNZUgGADS1MoXP35/DBSgAjRgLJDzKavBqq1+jQObcoefUUFipXGF/3NPREC2AVPYdGTm9yJrLZFdnhjmTuCBQK7iL8u2EqWDzwDazLgzCxDkyq+12Vy1iPQ5u9/3RDoQGJTDG2jwqZCvn77ds+BXoN3PGMIwN7t16w+9Bof5YibQbXkEw4O+iQzwmpbYDtcxfPJG9iM/0eZtQQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=TCfu38ZuJE49gx10wQf2PXMCxjEjkyE2eIqznWpFyWI=; b=sHKHsnKESWmNLfY/xKq2sop/6er+tV+tsEtUpy+IQly0wDgXLxKKamxsRw+fMXWlGsLtAmy7Gj2BVOlWc8HjgBuO9LhYavN4b7g4y1aQ6pezQAMRTIPJJdIE9AjgkfGHrcYiHF+/9sP/EZypURONTiWVqbA4ZdYZLKpGQfImY3PjpZYLiA4wn1OF+yVvOeJDYZ2CLIBbnwAi93scTYYxiSAyDTUruJYLkP2uYMBZKk5M8b+Xy2xtov+P4sg8HABPmKy3P/PxT7ej8TjBjIVwAZghdSCaCivmEmvT3YZCdP2m+1fCpqKzDBKXCPLdsbyMdhMZBouR4vv/PLbFjK2rMQ== Received: from BY5PR12MB4834.namprd12.prod.outlook.com (2603:10b6:a03:1b2::17) by BY5PR12MB4195.namprd12.prod.outlook.com (2603:10b6:a03:200::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4173.22; Mon, 31 May 2021 11:10:13 +0000 Received: from BY5PR12MB4834.namprd12.prod.outlook.com ([fe80::45ab:9d36:e3f8:40e2]) by BY5PR12MB4834.namprd12.prod.outlook.com ([fe80::45ab:9d36:e3f8:40e2%3]) with mapi id 15.20.4173.030; Mon, 31 May 2021 11:10:13 +0000 From: Gregory Etelson To: "Ananyev, Konstantin" , =?iso-8859-1?Q?Morten_Br=F8rup?= , "dev@dpdk.org" CC: Matan Azrad , Ori Kam , Raslan Darawsheh , "Iremonger, Bernard" , Olivier Matz Thread-Topic: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Thread-Index: AQHXUw0PW5pL9EaAsEWlfmKxTt2zd6r3e60AgAE0hQCAAAjqAIAANegAgARx8YCAABLYkA== Date: Mon, 31 May 2021 11:10:13 +0000 Message-ID: References: <20210527152858.13312-1-getelson@nvidia.com> <98CBD80474FA8B44BF855DF32C47DC35C617E2@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=nvidia.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 94ece3fc-1754-4b99-e1d2-08d92424a9b8 x-ms-traffictypediagnostic: BY5PR12MB4195: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6790; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 6O6gbAMqtF8tn0w6z5lcQxc4wLUZdNIIfxt/UUPYAXXnjBmHpREd4LukjGq1ibpEpnVs3CbFBUPixvPNeyKtKRv6cGlyHhY/r7dMqp4f9hBYBEmtfGCYzukG8tAmBKIq3RkNlbaEPNpp2FppOOdAxfc2hldbyNm+Q3pX9stquth5+EOHv8aHrRVDDogNt35+Ydz60w0fXPI9DpmnAkvhj4aW41CGNHD9FQCAK61Ng+OaoViBjI0OM9dzMYrqSHSCfrOTnVcvCBy/B0mVv4C3z+VZeOgvHQr/R3mM5NiB4oZp7bzQuoRciodo6GDGlY+13IuUtTeUSTZCv+S/K5PRK5bb2zgf/NB7iVaswxe+6DPUxl8PhCBnZiQiSJIbig4mgeSvFhTCRv3eoA5t24OEYPpuMs95us9e0PIIsoO3DYtWznI/JdTM2ToOicR9Zz8DS7DjPXICk+w9BNfhKdWb5oKo/b/gy2A99IYZ3dIbDvWa6ovTIYGEXzBHMg2mgVZa/JSf3CHsP71xb/nsP8te3/zdaRDvCSs9yQMls9y7nw29OCQ5NRtZ6wlvHtvz4rpMuXipIAXWg9XyJOdJ5CJH6/O7V5Y7SZE4U3BbJ6FwXk8= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BY5PR12MB4834.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(346002)(366004)(396003)(136003)(376002)(39860400002)(2906002)(5660300002)(26005)(55016002)(8676002)(9686003)(66556008)(64756008)(33656002)(186003)(8936002)(122000001)(86362001)(66946007)(316002)(38100700002)(6506007)(66574015)(76116006)(66476007)(54906003)(110136005)(66446008)(478600001)(7696005)(83380400001)(52536014)(71200400001)(4326008); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: =?iso-8859-1?Q?Cf80V+nybPwawRfWuZ5S+cYOeRVCWIbnnNdTNk9mOluVHTpEdRdrd8eTqB?= =?iso-8859-1?Q?o3uEjdxbG2z6LSGFfMdwwkS1B9hlD0ppvn3w6tQHhL0Ctl+PiIdSkoCIl9?= =?iso-8859-1?Q?enDd7F7cmM1MwfTgKhfEEAa4pl9lM/CtONHTsac4oxqkXDZ+QkP8qeyH6g?= =?iso-8859-1?Q?2ycuy8krU2lfAjY9LJ+O4yTDh7QmDoeVA1e0BkxdNgd3KmQcTN73TVjhq4?= =?iso-8859-1?Q?MUKyfgBiJqn+5oiaN0Z/bNViSrs6V3J10lxF8ubxTyxmp25JwelAvhO2Fo?= =?iso-8859-1?Q?JySzN+Z4MauMCmhCbgRsbdJv/JuuYeKuR29IATC7wv4ImupZPUyrfBUTZE?= =?iso-8859-1?Q?ewKN8tfBKtdlc94YGiFoo6Xh+00rU/IjsdJrSYgwOZN6PFhahUpR0Uo3jG?= =?iso-8859-1?Q?SE23s7heMdqy6gNWdJLi9PSyKhB/WlPqmIe4ngPc99fRPHu8+PPqcsjbuQ?= =?iso-8859-1?Q?1P8BPMNFUZUcoPlVEdxpTR2rAJDj4SoN942fpS1TvEh6AMEgzqFPrAMc47?= =?iso-8859-1?Q?AAODRl7rAmuacGzI/+eGhQkSzxuTnSEUA2mc8WruGlgCcWj0dz6hk12T06?= =?iso-8859-1?Q?qbeMLsNDe9y0n0RDU98Q8JW4ci3TXBh3Z3B6IuwGcHrW/k7eSm1Zb4rxn8?= =?iso-8859-1?Q?YhzUb9se5PpLFgJ6EQY9CKUnKBOI1TXEnks1T8mJJRO/8fBZZMhweXdV95?= =?iso-8859-1?Q?vX1wKyruzp1YkrUrsCeJAo5I4nsxghXMbx8x9X7N6lk/9CSJj5TcW/Ki+T?= =?iso-8859-1?Q?N9fITPI0XOeb6DxVkNZQrCbZYGNMBF6Q45NqAchbYvs1b9JVg0G8FRzXHt?= =?iso-8859-1?Q?n2I6GoG7CEc6zbzWQUCkNMLWaMo5jt74Yt2uK/XiVZO0kBqK3IplRJLZAg?= =?iso-8859-1?Q?ezHaX8ZHW5l6oR/1s9TFPm9tcJl66LsWXLYQFnak9H7WBwquZviv+086LA?= =?iso-8859-1?Q?cGSBhZiPa8KulJ6IJdtvmQvqdfZDPDVPJ5FncjMv4sLO3lYLVPlFPY8jKs?= =?iso-8859-1?Q?+nKDGjEeNGxz8SHkHhHS4QNdfucDgPu95AhddAO5NpJAsRmqFlA6j/Fw/l?= =?iso-8859-1?Q?Uk30C6hpaQrlZI/wsocu64Q8EclxxMCmOUEplkq0bw7VzLIBotXDt9G3dX?= =?iso-8859-1?Q?6fknfqNa1wFuZDkVN2LQSA+Nky0RqM8R+yIa5mdZ3Yb9Y5njJxReiIdSTE?= =?iso-8859-1?Q?wB0GgZpURkxb+2VnGkpW1/d+jEZwNCxUCkFWjWtt1m990rGZjOnDJJp1s3?= =?iso-8859-1?Q?iPlHRB11j7AsSShZfwfbQD3WhgtimRJsKQpmM6pjWpp8XUBSMOrcPCJDXc?= =?iso-8859-1?Q?wf4Ko++NZwlmbuQMYZ2ZwABQAwvkjLHBWfq9OMgTRR1MFKzeR5DQRO5oHa?= =?iso-8859-1?Q?jJ6Sb4mnkq?= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4834.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 94ece3fc-1754-4b99-e1d2-08d92424a9b8 X-MS-Exchange-CrossTenant-originalarrivaltime: 31 May 2021 11:10:13.2435 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 9esDijnp3cCy8VS1q9HWMQCGdxSkJYKqFlL8aAXhOK9n2lHas6M1g4+vDVKnWj8n4qIMTlpLdONWoeaH8wVGiw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4195 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" > > > > > > 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 lengt= h */ > > > > > > + __extension__ > > > > > > + union { > > > > > > + uint8_t version_ihl; /**< version and header lengt= h */ > > > > > > + 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. > > > > > > > > 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? > > > > > > > > > > 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 compared x86 code generated with gcc-9, gcc-10 and clang-10 for these > 2 functions: > > void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, > > uint8_t ihl) { > > h->version_ihl =3D ((version & 0x0f) << 4) | (ihl & 0x0f); } void > > test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t > > ihl) { > > h->version =3D version & 0x0f; > > h->ihl =3D ihl & 0x0f; > > } > > meson configuration flags: --default-library=3Dstatic > > --buildtype=3Drelease Each compiler produced identical code for both > functions. >=20 > For that particular case (2 bit-fields packed tightly into one byte) comp= ilers > usually perform quite well. At least I never saw issues for such case. > Bit-fields that do cross byte boundaries - that might be a trouble. >=20 Can we keep both implementations, the combined byte and the bit-field,=20 grouped into a union ? In that case application or PMD can select access method that fits. =20 > > > > > > > > > I think this patch is an improvement, and that such structure > > > > modifications should be generally accepted, so: > > > > > > > > > > Acked-by: Morten Br=F8rup > > > >