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 EAE3DA0C51; Thu, 10 Jun 2021 06:10:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 352A04067C; Thu, 10 Jun 2021 06:10:30 +0200 (CEST) Received: from NAM12-BN8-obe.outbound.protection.outlook.com (mail-bn8nam12on2062.outbound.protection.outlook.com [40.107.237.62]) by mails.dpdk.org (Postfix) with ESMTP id D66344003C for ; Thu, 10 Jun 2021 06:10:27 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dHN7hGGhVyxz3m5vPkulnAmIh39pV1SKZCnfvlBiODu7x5pcXY1Jz5y7jt45U0NdN25g9S09PHIoxiyszXg09yBUwTXJkwXSVF7qSUri0F43OziF4+eByJokMudZOjPeFzACb8v5vLfCgwznq727akphC+V4TCt6wRg9KsM3Ub0HhKz+xz8a66XtGGpr1jNZbBhFf2IbvLlOpca+f4Yye4XjhBCEb4P02/sZj1mlfxtuXgpYh40peh7nu9Fs0ixKGdlkAyk+x1EvMwvysXFM6EnlTWDP6LL4A7egrMIDHuJtxzJGLPLrUhVk5F8VwnPEjugtGRTn0eMxh3MNM2PRTA== 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=xCZilCLiIPIyHv7wuFE2ltKqdzl87CC5foZ4YCu4joA=; b=hCkHqOdN0lfD9teHor57kGM74liXAIjI4ZBKt2okdjumPUNxWhInamv7eYyrIRSkHBk1rjX6aT+I6pA5HEQi3htAX1cVgIeVsmZIKyZ06e69Cqjei9H+9G/9MHsB5i9FKNxsDlW66Flbh8KlgThqcHtCkpURu1VDDGcgViY7Z/Arp0wU3XLHexlPGOeQd+Qy/u+CeYWR6vb5QHRg02Yf7qWZvsRPqt7Bahjn9nEjkBDcjD4mkA+M1qOe1jnrHDDvrSxvG/9B01SyBFTNTTVPkLFiDZOxEJ1z9QNp4HmgQqpuMZj0EcbE1XZpLRfLMiI7mA+AvAVM/I4+TpLElyCwIg== 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=xCZilCLiIPIyHv7wuFE2ltKqdzl87CC5foZ4YCu4joA=; b=NQ184ehjKzBINigYUJepPZCzFyYKOQ7AMWUwIajsdO+h1UofrqznzTsWj161pSeZMtCkVhficnJIkc7WjTQJakc/afKZUaL7rRH+Oh5Zhx2IJy8xkVNarkD4eEyqIxAlY13mriqv+mnKZZLDCaLQH1By3raq0isy08RMMy5UtY9Bj4/0uTmldScV75L1f2+GLozimadFHyAsGprzMg158XrsXRzqTo3mI6zn4LG3Ylegk35TJc/pv8cSaYvYyM/5c31/m8u8cm/Gh4DTz0/yDv91ul+ZsWu6MDyoim/OZYwMdb/Qs7tOp+eHkxZ76Fc2rCQ7dU2cEey9mB8Ml4exzA== Received: from BY5PR12MB4834.namprd12.prod.outlook.com (2603:10b6:a03:1b2::17) by BY5PR12MB3953.namprd12.prod.outlook.com (2603:10b6:a03:194::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4195.27; Thu, 10 Jun 2021 04:10:25 +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.4219.022; Thu, 10 Jun 2021 04:10:25 +0000 From: Gregory Etelson To: "Iremonger, Bernard" , Olivier Matz , =?iso-8859-1?Q?Morten_Br=F8rup?= , "dev@dpdk.org" CC: Matan Azrad , Ori Kam , Raslan Darawsheh , Asaf Penso Thread-Topic: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Thread-Index: AQHXUw0PW5pL9EaAsEWlfmKxTt2zd6r3e60AgAE0hQCAAAjqAIAANegAgARx8YCAABLYkIAC38YAgAxiMVA= Date: Thu, 10 Jun 2021 04:10:25 +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: [176.230.224.216] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 85522673-2451-4339-09fb-08d92bc5acb5 x-ms-traffictypediagnostic: BY5PR12MB3953: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: AM3a2JpukbD1afRo2BmgVYlBj3+C/e8+daq1WrFItzjru93tYxG5Ypl24Jn1CSdrjtscmdUsvNv+YMx8AAW1u+vOS3LvjrGywufP8Y84UHxgmKDtTPcy39M1TjK2SUxVl9zp4Hme5kJHER3qA0iOSIzIbwAWzT+fTVHPO8WyJOlRNTwcGMAwpM8M65F1/UpBEo4vfsQUjNndYONFYJQEdONMzPn5d5ov+l1xmY2NO0Sd4If4H6ol5oucq85sVuZIO9S4cjl/10uE3LeylYaR/HcIS243qMovoCIidS6gIpMetHpj7DZx59qssbH4s47Ak0E52v1pLgOrhyg6WPjwJ3RicTROm9seaDcr1NhJdYv01WshqilKggGfGq7WfQLnf1Lokm1Dg+Iu4GPu4NZ+rJmyjM9j07m+T1rXKSZBwHbMtVQmRhk0erXcXOEVRfYX8F9RvqMcLVleh4HFLqdYYawmKJrlbJHYB/fOE8tV4NA4Y5jYBCer7EhFjRfbaEwJRODyd7L036dGUAyw1jUWHQY0AVTQ7MlcYfUnrlT+Ug6wjawKPY4FHPpy+kHEcOvz8rvDAAyXiAbEKN9bDPE2LzTenn1PHlcX6mOq3HSMoGrhH2ncEXmnJ34E975Xs/f7NxqN5afmAGgNW/cmaOcR4X2M0/fFUanfi0p5Rc+eBvABd3SRXxBdkJ7J0zK1u0OmOf2IIw5I/CMsYBo2XvF95FWy2Y1XCdEC0iy3i7cKxo4= 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)(39860400002)(136003)(346002)(376002)(396003)(366004)(52536014)(966005)(9686003)(5660300002)(6506007)(53546011)(478600001)(76116006)(55016002)(66556008)(86362001)(33656002)(107886003)(66574015)(83380400001)(66946007)(66476007)(64756008)(66446008)(26005)(186003)(71200400001)(8676002)(54906003)(8936002)(316002)(110136005)(122000001)(7696005)(38100700002)(2906002)(4326008); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?RFIrYJKqNFcGNZjM2x4bmu/+XpdUKAau82thWkYSON8D65nfbssbz4K8q2?= =?iso-8859-1?Q?ID0dXb5KXra5uYzc0pFKjetUXMIQ0DgC7DGMZKj90dgp197fd02d5BIsVc?= =?iso-8859-1?Q?BMMYVmCGyTth4Zz0xDNHuD6RDdeQtF7R6PGPdH4rMi92lAHWzb5lFSiHAF?= =?iso-8859-1?Q?R4XRV6fVTLl+YrDwwWxWsvoTxM+aP+8PABO6fxQTi9PIqY4/vUVmyIyh72?= =?iso-8859-1?Q?fYKlamPGTidOTakUho1p0LGgAHMG7HbcQZc5hXwD6bs9hqeeuydtBpdR1I?= =?iso-8859-1?Q?oCmUnmBL/eGKhRZ06DrchhG5HR94xfNM4LkjbJL9Afi+/NL+L/HpBaLVl8?= =?iso-8859-1?Q?eDjzKCYvGMRa9ihMsRe7Wcui/Y/fIlf7YV+ymPlhcxs6Y+ZxhtnXXcRVjY?= =?iso-8859-1?Q?flsShQz+Vf/KeVlDZKLvlrDazjefi3ndvm6NirbptKPBbYmJ/NBmagilzA?= =?iso-8859-1?Q?BEX///PPj/zU8dVIlUVqcxWYOAZ+AklpHM9QlInPKPwwuRInC26YyH9yxA?= =?iso-8859-1?Q?USs++MLGEXq/CAni+NexarfN/S8czLfxkZ9exaDT2ErUmaorMWCcYO3WrY?= =?iso-8859-1?Q?hBAq48VORnbt5zygjAEQQcxXJNzzEFQuGt0XeSSIzHKuKPv1Hy2o9X8Sca?= =?iso-8859-1?Q?cYzDgLnhnmrVdPIOLGB7aMx/dEIh1qSqV7DSuz4J3zclZFLkf6MDEuHbii?= =?iso-8859-1?Q?+v3zLjzYkx4nrrMzcd4HUP3iZOnDkyz5raHGZqm1ph0puboMGo5LuhRDBy?= =?iso-8859-1?Q?PkimPFc6vo/igjYm33TJqVJZbvrIjJyW2GLrB8NtL+k/d8rtALAY/k04LM?= =?iso-8859-1?Q?QaZXq72PGc2+0QBMvPdetNnQ56e59mwUNha6vbW+334sQXNVS14qxhptDf?= =?iso-8859-1?Q?qQ1Xc/W5tNpNxXPSO38TaX+M30dJKe7wXEoMrIWdovPGmmlN4AxhrqE5QR?= =?iso-8859-1?Q?UN4xsevokXaS/SoAOAStKCoQF2QEamA9sG4EMLce+ufn0PF7BhpLingnCJ?= =?iso-8859-1?Q?2z9Xih2RKZsXr0SewcsQHaDh78cJzL4zDOphZOi5wKVXx5/GkwMrJtc2vE?= =?iso-8859-1?Q?RMnXGJpGa2iYYB1GsWfChRAQSe64esgMxT82RIkD8HhSWPUaFghlTcgJnR?= =?iso-8859-1?Q?DARgM7LwUWfgxMekYvKBhXN3JT3jdbBL3InaRZ5wiMASIDsZsYUIDk07JK?= =?iso-8859-1?Q?Y5ZuOnCDw6HTpgK39bAqzpy+wFipH6D/yfmNwoee7HQgSG+hI05xQZDCb7?= =?iso-8859-1?Q?dTXLYlkhZ7LeavwAR0d1mAdiV4+HIsD5YbpsPPP3VC9GK0kHofqRtOsAx0?= =?iso-8859-1?Q?KNFgvG05vRi1K75uwifCQXtgeU2m2BHoMAZM1X2gXE3ikpUzO0YPnJiDGj?= =?iso-8859-1?Q?oRczmdbX+e?= 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: 85522673-2451-4339-09fb-08d92bc5acb5 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Jun 2021 04:10:25.3344 (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: q5p1BtA5xUmoFS4XM9FRwXgaDgpYOeWDJS4R2G2xqvMeSi3q4zat+fybI/Z2QZORjLqVkK18/ejrbIc6PO7Pzw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB3953 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" Hello, There was no activity that patch for a long time. The patch is marked as failed, but we verified failed tests and concluded t= hat the failures can be ignored. https://patchwork.dpdk.org/project/dpdk/patch/20210527152858.13312-1-getels= on@nvidia.com/ How should I proceed with this case ? Please advise. Thank you. Regards, Gregory > -----Original Message----- > From: Gregory Etelson > Sent: Wednesday, June 2, 2021 12:52 > To: Morten Br=F8rup ; Iremonger, Bernard > ; dev@dpdk.org > Cc: Matan Azrad ; Ori Kam ; > Raslan Darawsheh ; Olivier Matz > ; Thomas Monjalon > Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version field= s >=20 > Hello, >=20 > Is there another concern about that patch ? > Please comment. >=20 > Regards, > Gregory >=20 > > -----Original Message----- > > From: Gregory Etelson > > Sent: Monday, May 31, 2021 14:10 > > To: Ananyev, Konstantin ; Morten > Br=F8rup > > ; dev@dpdk.org > > Cc: Matan Azrad ; Ori Kam ; > Raslan > > Darawsheh ; Iremonger, Bernard > > ; Olivier Matz > > > Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl 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 / en= d" > > > > > > > > */ > > > > > > > > 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 / en= d" > > > > > > > > */ > > > > > > > > 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/ en= d" > > > > > > > > */ > > > > > > > > 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 l= ength */ > > > > > > > > + __extension__ > > > > > > > > + union { > > > > > > > > + uint8_t version_ihl; /**< version and header l= ength */ > > > > > > > > + 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 bot= h > > > functions. > > > > > > For that particular case (2 bit-fields packed tightly into one byte) > > > compilers 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. > > > > > > > Can we keep both implementations, the combined byte and the bit-field, > > grouped into a union ? In that case application or PMD can select > > access method that fits. > > > > > > > > > > > > > > > > > I think this patch is an improvement, and that such > > > > > > > structure > > > > > > modifications should be generally accepted, so: > > > > > > > > > > > > > > Acked-by: Morten Br=F8rup > > > > > >