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 C8567A0524; Wed, 2 Jun 2021 11:51:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9B7384069F; Wed, 2 Jun 2021 11:51:48 +0200 (CEST) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2085.outbound.protection.outlook.com [40.107.223.85]) by mails.dpdk.org (Postfix) with ESMTP id 2341240689 for ; Wed, 2 Jun 2021 11:51:47 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YdQbkyov9Tp4JKOw1lbov9HSFEc0FkbZ/EhdCVUAQPAaVm5p90kxdjYXHfV4xuOOg47cjDzlMKlioXxzAjwZfyYdxy3JoSQzbcqQxXbyy8wndsZEop6ZPDmv2EWcGtfC+LBaqT06Mh3CU6bweyHmEg6nQYroQ+rkODzfTDGjvtvb205C2KoXpK+IxdFGZ2X5FJexRrqvrqoMR3hcXe913gP0ybze+YDTFAQLvy6taXm3zZSSs49y96PRkuOpsso5vpK8t+Y8zq3NZF1eseV+EeK/huLgw8guh20EMGlczMvgLFklBuuZh0VRBySOc4bvwLZYRpU5xo5KpFe+EFsZKQ== 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=895VAPGjNykL6zn11r72FFU0uDeyjgkqQjgGkFF1h6k=; b=Yfa0Xpp19c0pM2f46EecNmZIBmS6kksmM/pc9bZrUbQ+7tbKt0kGWTYNTKN1KwNzUKmnWkzi+HmGAYQ3NinUqpxntFTQSuWoxtiU10abOuOQ8L3ChCVJtdZI86Q938oCSbHXq2NyIQ6kx3R4l9ct4yD9BND7rWsHPglJgU+xmSIsmpR6xichOEEzcuhMSmsOX1AhLe3YGhyZb0Go1DoGoan8rpBHXHK20U0OOJulNxTFF0DKZ5K9/SFhtfNZYaOrKxCxvXoPHIuUoIb3aGSAKILraxVlDp9e0go/jj7aBnlMWjkdULuU/8eMisiYgIKKJlrPcq9Ikqjj2NY9hrXRPg== 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=895VAPGjNykL6zn11r72FFU0uDeyjgkqQjgGkFF1h6k=; b=jQU4BVzb3Vw4SYRvwfZkinRKIChPurayLpeSSmXdd+Jk2zI3iA8NUfdOKuPTM+V3gAB/Ro8VK1Y6sf4jAaeXatojl5eqdMBLVO8OGt5xhY2g2VA5gayTc6Jhx5n2znvnDujO4aPhP/uLycEmieWLlaXAFUzGMKluBfsXP6SHIrb7Gl+NOQNT1zTVYw2v7mcO+gzMyds8IGnFXFtejtpSn7hrpFg725oJif1N2rrP42LudlpQSD5yPdYsssnDrRU2zdcQl4Cl9rikKNkulCVELo4nO+LmbWcG03VPzBlA5cR7CX7XVABoqeLjC8DEmrQX5L/l0+AptH58A59ICUTv7Q== Received: from BY5PR12MB4834.namprd12.prod.outlook.com (2603:10b6:a03:1b2::17) by BYAPR12MB2887.namprd12.prod.outlook.com (2603:10b6:a03:12e::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4173.24; Wed, 2 Jun 2021 09:51:45 +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.4195.020; Wed, 2 Jun 2021 09:51:45 +0000 From: Gregory Etelson To: =?iso-8859-1?Q?Morten_Br=F8rup?= , "Iremonger, Bernard" , "dev@dpdk.org" CC: Matan Azrad , Ori Kam , Raslan Darawsheh , Olivier Matz , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Thread-Index: AQHXUw0PW5pL9EaAsEWlfmKxTt2zd6r3e60AgAE0hQCAAAjqAIAANegAgARx8YCAABLYkIAC38YA Date: Wed, 2 Jun 2021 09:51:45 +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: 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: 3b378052-d6c5-4d92-b2f9-08d925ac087b x-ms-traffictypediagnostic: BYAPR12MB2887: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8273; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: CdbUC/SmTPQoiTtaOSEUcI6szFE4sD3NsaCL3WQrtwNUzWyuLLtgMok6X2+CZhm4tg6L9HaHSh6yT+0/6hHZZXvmk+e3tNdYokcmoZKB83Oapfi/h/LGSrRaFBYwNzgu+TOAZEpejwDnLE9yJgM7hJw8ZHlbflT4vRlkXhaUJXrlgFTYrPJ0W2fM6+U2zDK4PehkAsqhCgN4+BzcDtlnfkICmIm22SuQGGbFBVjYLfkNdQvXAcNaDwd0NsVcGiRFuMNXQ8MC/bjoresuTrzfaAgBbhMB/RFjdKOTegLrYE/1zstZCkPdB+7uM2687s4TTdtfxGCOdo1JOY78krQwgyo0qzOHgThhtgrq/wKE3jx3pcA8WA+NMoMDvx4Huy2reWK51t+whEw2hdSZ0H3C/wQqYDgUMA/7owrUziFRh0ihfpKp/sgXtxTKhiw14+yfP5fx1F+u16qt0RB9kSP8Ljy8jBGOsqBoZkIUl4Dfm8z1CZofmpRvlDCKf6t5tVgZ5QvmPZfunk3AjSSvTxZaGAcqHvLDrOH5EZv8O0rFYlIYcQQxJ8WAHGDCpgy7pRWfVX3NcCjWTJvZLqK2aF3lwtd0IYoCvROtJzpgufPsxvs= 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)(376002)(346002)(366004)(136003)(39860400002)(396003)(5660300002)(186003)(33656002)(8936002)(316002)(54906003)(38100700002)(8676002)(7696005)(122000001)(478600001)(26005)(52536014)(86362001)(9686003)(71200400001)(66574015)(83380400001)(6506007)(53546011)(107886003)(66476007)(76116006)(66946007)(64756008)(66446008)(66556008)(55016002)(2906002)(4326008)(110136005); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: =?iso-8859-1?Q?sWh2eAIfIEIK0/spaxjgmcGGx/OsEeWiHAEof0iKqHiaJiB8ZRoQesb7o4?= =?iso-8859-1?Q?DIQ5auITYd4PDi0SXRVDjQpNJXlQS5jAAnRr/EhQpHHixpgmR7DWPUEJyR?= =?iso-8859-1?Q?c6xnhypwfMN8kjHhxFRzS8vfZ9OHWarDEBheJCyloLRMJpA8pLMqcegATB?= =?iso-8859-1?Q?MEI5ccm/+kTkF60k2ozpq8NR08wcc7GhEeVkjshvmlWE1YmDkZrJzMiEru?= =?iso-8859-1?Q?KnEs4EMBBADxTKfrMK9azHqK4FkvSZqJwPiqLfIxTFtfdpKXBgWFNr6pJ5?= =?iso-8859-1?Q?8qRKLmIFUSyFwK7GybAcB23UykzgYDLAGnmflQoTbut3iAV9RF+//DhpgG?= =?iso-8859-1?Q?XY24iWSdufkv8cvLNo2M9LmLqskGPbCtyvWyDm/dAQal9vXUcB/7cxZErr?= =?iso-8859-1?Q?8fkCLnR8Q7jJ1ICNLZM+uahK2RE7Gi3fRjXAa+cS5DC9uZZocKaP9h46nl?= =?iso-8859-1?Q?oBZjtEAxfcVJEdEe0t0TCBZISS1kf3t1kTPjz1uKMcnXfhl3AnLaSYW3tn?= =?iso-8859-1?Q?5roAert8MBm6icJh8dnCopbHgzT3G0CHQV7oUWhbjOvouM1yAzRbVZycR/?= =?iso-8859-1?Q?wZ8dA7kFeSTUmNjGvCi5tCCFckG8+esw4o8Ims6KVN4or3QODzeewRqJwY?= =?iso-8859-1?Q?haGyExxtocb0JvMzICcuoT+3T7Ho+YzJwv1spjEDvOjy+UyQ8eSaLtaBLi?= =?iso-8859-1?Q?MV2Kjrt1RQ8k1t4JeXWtqhEgcfFwIdPKEZedterLcY2bKbqlx8m/HKD+t6?= =?iso-8859-1?Q?/7YwMWweQDw5ZwBLie/naZvn3HuTapZO8xQL0AMy9rmhCiblISaJlXxDST?= =?iso-8859-1?Q?+dBOkPaHTCdU+g9kgOacd8wEWD51x7ITbiUSThxSurMTmTQy7YK5Ynamfl?= =?iso-8859-1?Q?EDeMfKezDMMtf5sEpWW15uiXSgkZkltyOs86yQVr0njske2jqRZAnW2k8f?= =?iso-8859-1?Q?d/8+XVvdQj+RGQNXWnEni9fTut2d9ZfysqeI1oWl99ZqmQEwGJFScYvmc8?= =?iso-8859-1?Q?9HU0nsK5vjVHfta0HqCZ5bxt+KAe5Qzl4JA3PoYREPu4yCSecG9VCJepZK?= =?iso-8859-1?Q?rtpnh7fUvtxZ/HGhmSgZVqhnd/jjtTYd4g/uQ5LTtE7oK5j0KL0SygfW8c?= =?iso-8859-1?Q?P8unAgheEu/WanuZx5+8ckPuRp0oeeaIXoLS2wppaqS9J8gLuGlysZ7Db0?= =?iso-8859-1?Q?PmSZZoAvha0sjE9tWCClS9+FwWtqjMb6dnjxZj540ZYRDBgnR2VHGPXIJH?= =?iso-8859-1?Q?ePcAq3fqUUi2aIEoP2igA5LelD6gDXOa/iOhzYBPxMqj8rZVPszzyAogOS?= =?iso-8859-1?Q?p1ceFaPGPd0CRCZuqZkysav+XuoqLkIOLJYvMB1jbnSrW6KB90dcXP0Cm1?= =?iso-8859-1?Q?gDERuRNbWk?= 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: 3b378052-d6c5-4d92-b2f9-08d925ac087b X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jun 2021 09:51:45.4604 (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: XsiE2bMtituiK5Xcs4cWXRyirq/U50y6Yftjy21tPWGPymEThTOVPap9VG6T/TR65HJgWGc4DWWJzWN+IHIKcQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2887 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, Is there another concern about that patch ? Please comment. Regards, Gregory > -----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 field= s >=20 > > > > > > > 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 --gi= t > > > > > > > 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 len= gth */ > > > > > > > + __extension__ > > > > > > > + union { > > > > > > > + uint8_t version_ihl; /**< version and header len= gth */ > > > > > > > + 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. > > > > For that particular case (2 bit-fields packed tightly into one byte) > > compilers usually perform quite well. At least I never saw issues for s= uch > 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, > 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 > > > > >