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 1363EA0524; Mon, 31 May 2021 11:58:45 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 922A340040; Mon, 31 May 2021 11:58:44 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 3589B4003F for ; Mon, 31 May 2021 11:58:42 +0200 (CEST) IronPort-SDR: S//W8Jnp5YTEqF8G+ojNVzj3e16kuRr53jmZ2FwTUwD7VBmErm30HiTXfLfqDSq7aWQ97MP0Oe nhjJsqzIs5jw== X-IronPort-AV: E=McAfee;i="6200,9189,10000"; a="203335603" X-IronPort-AV: E=Sophos;i="5.83,236,1616482800"; d="scan'208";a="203335603" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 May 2021 02:58:41 -0700 IronPort-SDR: pQKHayEbhSjxNfkk8ub+kXGV1YTzrYGGO/uRV2WCpHD+ho5rGSTHT2K4F5Y36pBSYWx+tDWZ63 abHXdr1EKYVQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,236,1616482800"; d="scan'208";a="632528039" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by fmsmga006.fm.intel.com with ESMTP; 31 May 2021 02:58:41 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Mon, 31 May 2021 02:58:41 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4 via Frontend Transport; Mon, 31 May 2021 02:58:41 -0700 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.101) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.4; Mon, 31 May 2021 02:58:40 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SQt1wNEXkLUYb6Vkcc67qev3kbTHlxTGYOfZ72gNMfiUZIPVzrSH6Xj40NsKtZDHcUBcitoTnBfZmcbo4h459zGT+Y4DtjhL9Es72qRvN3vclmTFvis18Az6jOlkCmZRxqWSgCyggjlxHD4kvvqwZdCN3tOrielLf+bVrBNnuucIjr2O6sAg0hod+A+OcGTrxIDFv7M5EcSdUBgI5B6dgCfVyDzXPPGwpnAZ0T2y6bFGcs+0Q0u/S80ADT5Lxwz+tdfRarKALgJZZcbGIime8H5VlijAmuTpZ/DovIyurhmyuBY4atFZhJJnf6hv0SGekCLz61kSCOsSVVtrhW+oZQ== 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=QXbeeRw7axEDvMz7CQp2xy7mpQD5o2KebUWVpYT8g/E=; b=Hr20mfdny7Un2nzF/SzwBXB93qBOyf+L/1VR6mTpNXJPnisAAhIvMA8LCQbzjIWHPpOyQuakR04YlEn4C4azEMZQwaH6Ezucei+ECluWIcZD37pJb+6+fEAgn0690cqfGOYR26MX9K8wdsfBjTt2YxPmrEZajnVk/rFq+DbQYMYuyR15vpxNhME87/uRUuZnxyfz4d32W4af/naouQHjsjr2iNi9tBwR9k6CzIXVAhB85ncIi0IcF+sdOuZ5efN2WzWtMY3y/CsNvc84HY+S9+Y989XLVyAHIU7lcqTBJ9LTGN9SJv5u9jpOCWA5FoZAqsE6LIzxedMhV91VbxZq8g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=QXbeeRw7axEDvMz7CQp2xy7mpQD5o2KebUWVpYT8g/E=; b=u89Ij4eB+CyrDflDKYGGI+Yn3aaECyic3qsymFAbytosi2HzPhn3ty2IJ6LqV0DWlFpzXQQEh9VGKS4jWqExyhwb0ndKySTkbuIfsk5dOtTe8fI2m25pTTcUEpfr2xeInWZhwS1+06W6XRvQ7cQ8rz0TT/h54LfAORU0fUIacTM= Received: from DM6PR11MB4491.namprd11.prod.outlook.com (2603:10b6:5:204::19) by DM5PR11MB0010.namprd11.prod.outlook.com (2603:10b6:4:6c::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4173.21; Mon, 31 May 2021 09:58:40 +0000 Received: from DM6PR11MB4491.namprd11.prod.outlook.com ([fe80::15ed:b4f4:540e:ea0c]) by DM6PR11MB4491.namprd11.prod.outlook.com ([fe80::15ed:b4f4:540e:ea0c%7]) with mapi id 15.20.4173.030; Mon, 31 May 2021 09:58:40 +0000 From: "Ananyev, Konstantin" To: Gregory Etelson , =?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: AQHXUw1e6pIatJvRL0OXGQrSmeIttar3e60AgAEsi7CAAAzA0IAAPYcAgARrFFA= Date: Mon, 31 May 2021 09:58:39 +0000 Message-ID: References: <20210527152858.13312-1-getelson@nvidia.com> <98CBD80474FA8B44BF855DF32C47DC35C617E2@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: nvidia.com; dkim=none (message not signed) header.d=none;nvidia.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [109.255.184.192] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 83a47740-6337-4d78-8e14-08d9241aaaa2 x-ms-traffictypediagnostic: DM5PR11MB0010: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6430; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: u4lC/K+9y3oS/MAmxVgCu2/CYln0GaFfXj3fCf84Eh4h5weLISCT4MsFTN1VnfWm7iDznWyTTSm5lORgNbqSTKq15nSsJrbnOSp5j1l2X6b+cMR75KwFH6lJsk5QRa3pcVGYQdJh/OCUWEaExi0Nw8d+tKjw2y4sR1IVI0RKvy6m893CRsVFCCm8+2noD7deWRRbXnUlw7G4lcp+ilKd/jiZ030FsdKi2yWNK2fdaSdMXWvIZNDQD8lxId5DrPwEyQingrJnm8mSs8kmJjb+8Dz9mBfka0LexiE8UY0hIbydsc5va/VQsBrQcCMJbK3ggXNJ4ryGVRjMuw365CIAeqhfx4JtiDQFVofjV4dx2kg6qa50U5sGZ/lfkEnEb5/GRxJJbz9crtY2giBDNAv4P2k/Z2gcjLKQvVKqnkn/58otjsba0CG2KDCLFYGWARBRRi4KsL4KF/aB4GDgoYqHERdKrRIJg7KgTE7WMBfhjkW4hESDZmnhqDEau2izsSBBQziu+Q03+2f4GJIv765v/n2mG/7MUFs8wB6eNiVqaPEzWL6JYIjNQEZt+J+8ytv/EPKdVTkdh9WCtU9GTqxYy60Wg7swITNYvx2b123iiDc= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR11MB4491.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(346002)(39860400002)(396003)(376002)(366004)(136003)(5660300002)(71200400001)(9686003)(2906002)(66556008)(38100700002)(66476007)(55236004)(316002)(64756008)(33656002)(478600001)(4326008)(186003)(66946007)(66446008)(6506007)(8676002)(55016002)(26005)(8936002)(66574015)(110136005)(83380400001)(52536014)(76116006)(54906003)(122000001)(7696005)(86362001); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: =?iso-8859-1?Q?ErTN1hr26EuEOHtIPj+Z9DA/r/hqEe7z+dmVP9pRlO72dNxDeEdkYY4UVc?= =?iso-8859-1?Q?knK1V6bqePfwUSNMJcTEcsQDahCNHbwEMfdo2xO/KHCcbhCRMNulfccqDy?= =?iso-8859-1?Q?eT39ctULhTihi7+5bTmpoT1PIhddbe2dVMbmvo2VWizLtyQVZC9DB8OQHA?= =?iso-8859-1?Q?0dZkPxTerDMb32ovN1qucIOqFEwJ0D4RW5Eev/vPmn/eDQDmkMudFKfF/s?= =?iso-8859-1?Q?RHn+ft37yKyy7bGU8lRXAilCP2Ny9BhB6bP+MlZLxWljPd2EJmRcvxUShQ?= =?iso-8859-1?Q?89QIZN4ipS0BtSGLAw2sulfxwGlO/6cVEZ60bg7FWJ+qJwJCHIIiIgw0+8?= =?iso-8859-1?Q?fhwzB9BI+0kYHOvxKFl8qIhzsyd1HgUDdhXQOD+2kguQuOu7hs5taGCPUj?= =?iso-8859-1?Q?ps92ahfS4s7vLdu6sHLoSdemQ9YhGPSaXhTRaPq9E+VCI4SSrsqWQyD+Iw?= =?iso-8859-1?Q?YM/DqOqBoQzukLIwMzm9TW6y7amwoN+FI7sOXilNz+tTo+wL94Dml7Bzen?= =?iso-8859-1?Q?CdJQskDX8CtGbXKM8HxCg9hABosHrxs6nB60zHJYkz321zZ4lWMvfA0oB9?= =?iso-8859-1?Q?T66MeMNc6EsbIL5dinJNkUbpIop0r8rHExbhGk7r0ejQprG3sffQY7yp5a?= =?iso-8859-1?Q?Lg5Lmxswei84Kab6WeQc9Bj7+2YOAHKjw2QTygXRPnnjCjUWBaIJPIahFq?= =?iso-8859-1?Q?plxW2qiiJkbtAfETmiNJQn8FnFHE/n7mxo2vwEyI//j5Vo6j+V9OvbtcXl?= =?iso-8859-1?Q?uyGHeNDt4ED647XTOKoHWNguAUChkpiWZWQCrQRjoWjniPx1Xy4rEP8aUD?= =?iso-8859-1?Q?VtIUutbZiNUxAWhl0rX0YyYiRVsCz896kqG78AqaVyN2/ks4ftub6NlRw9?= =?iso-8859-1?Q?hx+pwrTKe70PDiZW1rLthzvCI2GB3wgGGZEKRzegWyAtvw2ufvlznCRCha?= =?iso-8859-1?Q?U154F3XA0lOxHTvQYPei03vUqTrrjJsw3cSt9IJ64jWHsgRrbdu8vW4HMT?= =?iso-8859-1?Q?EvHDq3yaFabhyojkX+3iIEzdXQ0fzWGA0krbR9ALDVUUM9r2Tt9vLqZcwO?= =?iso-8859-1?Q?F6m878msW7jrvufHKMVdtQvrO7PGo6ePuQ1X/J3qhP86f2jbzT95zpyVE2?= =?iso-8859-1?Q?4fIfHUXRFgHj/AOvCNgNKarzy809gug8KSAtWZFEYpSr14oKeX3E9Y8jwC?= =?iso-8859-1?Q?zjJsRqRbUAdYmL1JGko01ESzrgJdYBTdMkSXURJJZpcp5DsQuxdC+JWIOS?= =?iso-8859-1?Q?R94WULORbSfHbmDLQ0nmJbOP8VOx/VyqRz56CK07jMJiJ0QM9tg6k1vhmJ?= =?iso-8859-1?Q?uBwRVgW5QaRdBFn9PYr+/BRXqLBbqQjfpRigpphaCkUUpHrFqDsag6LZOr?= =?iso-8859-1?Q?s0qmOi53za?= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4491.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 83a47740-6337-4d78-8e14-08d9241aaaa2 X-MS-Exchange-CrossTenant-originalarrivaltime: 31 May 2021 09:58:39.9225 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 2HU5qUaYV1o6HP2am82fWJsGqNWGqzR4qd2kLl2FQORcg8McQjQJtkvyutcO2/cn29cqptuPRS3yYkqtGKcb0FbDxssg12L5SHCn8W/IzLo= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB0010 X-OriginatorOrg: intel.com 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 rul= e. > > > > > 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..684bb028b= 2 > > > > > 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. > > > > > > 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 t= his, > > 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 ad= mit > > to falling into that trap myself, once in a while.) > > >=20 > 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=3Drelea= se > Each compiler produced identical code for both 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. =20 >=20 >=20 > > > > I think this patch is an improvement, and that such structure > > > modifications should be generally accepted, so: > > > > > > > > Acked-by: Morten Br=F8rup > > >