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 8C857A0547; Fri, 28 May 2021 16:18:20 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0FFB240143; Fri, 28 May 2021 16:18:19 +0200 (CEST) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2070.outbound.protection.outlook.com [40.107.223.70]) by mails.dpdk.org (Postfix) with ESMTP id 0B0F540040 for ; Fri, 28 May 2021 16:18:18 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jhsXmgTfyKdFSAto7qcMbfm3tA0rryow/AuyiSG0GeOHrQkbF+7jDOBDnp1xmgXsDxxngTFBC0LN8LbwcfN/lvYa6RD0B5BmKdb4HpTHQwOmK+dpcSJNyVUx6UBj92Fx4tf5ERuP9aVu9SSyqlknySxGsH+u9lZyQeMGuPDC3u+hKfGbUOoiB17hvubtpGPDgVrXj8iKb3Q0IKft5Q4Do17yyAr0Ngnn62mCFJYQBi6nQTVRsWTgykE8DnKjzZKl7c36tPKEtLjaHD2Q146bplqRkdGa0MvkpJ8SsYkb9MdVMvCjcmpM2Foez3zlkD8gzMairRCcfOGEhm/bbkz9bA== 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=Nnr0HDoY6XWMNBwo4W46Xo2g9b7la0m74fw+7cu/kA4=; b=CjLZpo4YZXIpu77wY6mYUtPVfKyJ2jrjMWKhIe/X6lJMljL+P8qtQda8JARxzyEsVTUVwNo46zn/lCqkAKIFQOidjAU+oMo2oo2EV8m9Nt9PCtsaAihDPCI4T3+5AcMRZ79+Cqlg+T+xvWvQU93E3xDSCmM5jEtyfK/2/ne/CxM1sq5ekcTdFDp+szM4WB44fBn0j2QaTphSbCkBixL3K1i/a9f/J62OHs009CBUtjoYmJ+XUAjxOMRXyWx8PgHHaLupJmH5/lXREaPjOHkDuSGOYEH7MpF1s3ftP6va2CfhZSuHhiTcg4PLSj5JQkSZLuu3emW6zy3UJqr0pPBupg== 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=Nnr0HDoY6XWMNBwo4W46Xo2g9b7la0m74fw+7cu/kA4=; b=gZxcn4qeBPLilXYtnYSAYxyDVoRIkuM8V3XHreHARpSAXkP+71hPBcZNPS1A6PrPgPiRhGyjvy4DKyJy/B73KywIRX5Vz4iJ3C+cT+h9LR19Ai5ER5H58tru2KZ1xBkIponUM6m0fsoqcMLp5hTGMuiNNh0VjQ+d5AxbpYZelhGCTdChl2oQR/OKRkmI6pmDYcwWzdKS38/wDuVKGXOq8WY2gZAR57P6oRSKggbn8tuH+zHA1yW//1hXcv4MjsqnarASFYEpBS5V60yPJSvjaipuYKtad40dF48I5sAbu2T+LiKcmDFoKob6SWBOUbE9Vq7xqVu3s9D+fBsLaRDdxQ== Received: from BY5PR12MB4834.namprd12.prod.outlook.com (2603:10b6:a03:1b2::17) by BY5PR12MB4934.namprd12.prod.outlook.com (2603:10b6:a03:1db::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4173.24; Fri, 28 May 2021 14:18:16 +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.024; Fri, 28 May 2021 14:18:16 +0000 From: Gregory Etelson To: =?iso-8859-1?Q?Morten_Br=F8rup?= , "Ananyev, Konstantin" , "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: AQHXUw0PW5pL9EaAsEWlfmKxTt2zd6r3e60AgAE0hQCAAAjqAIAANegA Date: Fri, 28 May 2021 14:18:16 +0000 Message-ID: References: <20210527152858.13312-1-getelson@nvidia.com> <98CBD80474FA8B44BF855DF32C47DC35C617E2@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: smartsharesystems.com; dkim=none (message not signed) header.d=none; smartsharesystems.com; dmarc=none action=none header.from=nvidia.com; x-originating-ip: [176.230.227.218] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 83bfb502-30dc-432b-4c6c-08d921e36fda x-ms-traffictypediagnostic: BY5PR12MB4934: 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: ez5mwFey0v3NLXiNpwrEwf9dzFEBEIUSz2KfxsuMLUUPgAYvypP2lOVuHH+M1EjwztvoV2whYV3O/LK1rsmxmazf7ERrsu8/IhCP9nRrPg5vDj9I2hgD2/ffI5QDftN81kB+YEukghvZzH07kFifY3i751lAO2l7xAxWn6hZKWvPEp9FtRhqqxyJaNJaDpSDE6stalrQ40kdt3OzbSN7BcfwNdNxOqY6PhbahY4grro1jrQ6nbjYHZWDsg0VZME7bwBjuQCtctj75o0gEtfCL14Cgxos8SIHRg9HbaZszIzS1nnkvzGeojs+4xKC62/wAk/YhPJgLFOORU4z9Em0sDeoBNXnwChpRbvet69FSLoymsa/qpcexI5ohp6hcBYHg2U9mM4UUJ+xGxNo60bOlrNRhNgI3F/KNO2nX0myD76Z4AKXUvVUj20gOzlWzretDhNGAcoMbqrCd4sOc+eKTUVRpPTnSoWF/7BCHLSPu+uHECfg7WaBy/iNRKVwA8vypet6ycB0oKJsY8Wfp/gpZuzkTsN/v9XVU2B1H6MY0YJQ6cVRXuA5slp6Su8xYhcPQREwTOMRQrmO3HyMHc79ISErG/MN8JHgic16zrVjhxc= 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)(366004)(346002)(39860400002)(376002)(136003)(396003)(7696005)(26005)(6506007)(54906003)(110136005)(83380400001)(478600001)(316002)(8676002)(71200400001)(55016002)(66574015)(66946007)(76116006)(66476007)(64756008)(66556008)(4326008)(5660300002)(8936002)(52536014)(122000001)(2906002)(86362001)(66446008)(38100700002)(186003)(33656002)(9686003); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: =?iso-8859-1?Q?9srMDNecQ9iLWMSF2ny0zc/MgqVVh8uJe/q1EswJSW4p19JAlH95A8kbbk?= =?iso-8859-1?Q?q9YP2d9IzFolzTArvwtaLBEJPW5fTrFxTIVQZ7ckf3lb3H3rI6GC4BL0WY?= =?iso-8859-1?Q?F7wbp2Jp2MU2qZg98LuUOtUYFU2/7lzO9b/eBeogWO6T3Y9qTtI4tS5F8J?= =?iso-8859-1?Q?Kr41VdbZgY23dMxiEAgf9T7Dx1UcGtaIC+ceCadBXlcoSzscexxr2+1CGP?= =?iso-8859-1?Q?GACvQZBgIDwwYbOx1xKeFopGIeHUc5t58uSQj/iIp5SSrNy1bv/u8+Rvvd?= =?iso-8859-1?Q?MQd6xf6RRSSYbK4dOTEvZc6A/CnMA2kjeChmEvgkJOnsDimiI8C+Q+GJ3K?= =?iso-8859-1?Q?N++g9NmtSfu+MR2m31KLzTnC9gi8QFjX5CUiTg1ZE8TDxLk1/PqBlEW9g9?= =?iso-8859-1?Q?ReOZ573DA8Kn4ymgivxNxqNvQ+Y0Uvc4JXQzB31Zm7XO4LReG21+Yr8y5T?= =?iso-8859-1?Q?9Ai4SQwt3kuHdw0gSucOzpNmajFoZDaOui9gPR4gHLhg40VLcEUFrA72GE?= =?iso-8859-1?Q?3Li7asdUQYOndTLFDMK52x+aK7vXGbyEKIi/ZoKOWC2NmyDjFKTKv86eOb?= =?iso-8859-1?Q?YIUkO6luInbbCw49clyuaSsbJyi74YeijkEB2f33qt1lQ5ASXTDhaN4uu6?= =?iso-8859-1?Q?bJ4iLXHDue7sqGHPUL1lMEY60TLGhqqAi6YlCSpiIefeV/eRL4Mwd8SGlu?= =?iso-8859-1?Q?1C4T4sR4WPdOO11ZhkVHwk3WWhiD0OtIXgRkhplUMMYZ1eAFuIeVTCjfNT?= =?iso-8859-1?Q?e+m167aFbdMZYfAApnsDZ//KPw5HZn9CjeRtjVba4EEVznc2FTGGlCQyYr?= =?iso-8859-1?Q?9kRXwASesprdmaHcz4uyX1p8S0Hsm3FX6gjQdUZYDXLRn+SuVW/nQEArLE?= =?iso-8859-1?Q?Gpsp2TIYjsOzZfe03Cek17Jvv9l7XIO1M4GYCTq4/6XNcSbSNznVsaxDi3?= =?iso-8859-1?Q?NNTfzUYOv1+B87HmxaTiGh50D6oYCLgziiirmgE9QWJvgfnEHj6JZyWIqr?= =?iso-8859-1?Q?o3o/dUNS+9Rj0SRvmHdtgVBL89OG39THQL6c2pGaq0vN2s+y/nRotqJsfF?= =?iso-8859-1?Q?d0evZt3O2Wywjkhu0VAJrZxxmSOLk9mFFZf022N82VL5NPKdaGukS0O5Ce?= =?iso-8859-1?Q?+bp/eGNYMcspQnCq6fj5sII5QE9oRVnSXSb04nhD88ubKn+0J3/dprmhnH?= =?iso-8859-1?Q?SJKHBLiNyVMl5TgHNWm3q3bfaEFzwQc2p0SzASycjRMZi9pIf5SUTwxr8d?= =?iso-8859-1?Q?NSJWpv8cqFiLQfODT8qPaPX6aojbErQX+gg5d1sotLBgUlSjl7Tao2xQO8?= =?iso-8859-1?Q?1vjKD35XNfUYf2vy0LJEADw5s6rYAJ92CtJOw6u4sVvYBOGSPCJX9O5Llw?= =?iso-8859-1?Q?rwYCqCOCDs?= 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: 83bfb502-30dc-432b-4c6c-08d921e36fda X-MS-Exchange-CrossTenant-originalarrivaltime: 28 May 2021 14:18:16.5557 (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: pZVZHbD8UCvBhgRoy4EWPesq4kkasw+v1XifGVbEuPujZtpj3bI69Y97vSvfqxpfNoURMuTi6HAfcAfQ0Ms8Sw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4934 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 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? > > >=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 _MA= SK, > _SHIFT and _FLAG defines. >=20 > Yes, this change will lead to the introduction of more bitfields, both he= re > and in other places. We already accepted it in the eCPRI structure > (/lib/net/rte_ecpri.h), so why not just generally accept it. >=20 > Are modern compilers really worse at handling a bitfield defined like thi= s, > 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 av= oid > making decisions based on past experience that might be outdated. (I admi= t > 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 f= unctions: void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ih= l) { h->version_ihl =3D ((version & 0x0f) << 4) | (ihl & 0x0f); } void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ih= l) { 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 =20 > > > I think this patch is an improvement, and that such structure > > modifications should be generally accepted, so: > > > > > > Acked-by: Morten Br=F8rup > >