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 8C82EA04A8; Wed, 26 Jan 2022 10:45:23 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 11BD64270A; Wed, 26 Jan 2022 10:45:23 +0100 (CET) Received: from NAM12-BN8-obe.outbound.protection.outlook.com (mail-bn8nam12on2048.outbound.protection.outlook.com [40.107.237.48]) by mails.dpdk.org (Postfix) with ESMTP id 7A6CA4069D for ; Wed, 26 Jan 2022 10:45:21 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=T/49XST5o2EfHnzsKsL9Ga6y3AzNXMhk30+keRfSkZDsl1F+us7r1lUIhKJTVTAyiNn1lUynWSOOvEyrCpcMPmu5uI1DvUONhI5/PcSVi+76GfZMH/YMfDzkdGu1yre+xwFzWWaUHqWyambaPcRWejzBxb+fcym3DF5K3b/rWidko64gVev3Hr6ksvTEdiTaeov2vJRLz005CTBPqr6gSv5sBX23aBAXsPS1j76MR3dEWLCP/wZEGl6TxcbytmrQMN/1yC/KoNPGN24AJ4ShgxdefsmfI4g1wKIoxKWltqDDalIBKKn1AX1/rP4ltAUV4nEzDti+fdlAqE/5IbXLnw== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=lW9THoQ9BZs1EA92DYLc+8Ja33u2uoLlyo2BgS6FQq4=; b=DaOwsnwQhSr9orb4ol7I4GpCHjuebgtbAzZc/uPnr1iL00KjpdVDX2Rfv7zLd8y0DDcA4TrFsnP8WKFCVWc167cW5dA+gYPeXx4jo2ph1cPe22jdOAXDnf2mgM1dsgvML+RjwnY2Xrxu+m6ongawyAPs/J4lN4Rir/IrPEu8Da09N6VLcLY6mWRRP97YPQrDSYn5P27P30j64AwbeUfFHJ2OJ5biC987cs0RpzfU7UNCn5JsR/BS82D8ClXNUypkWoSJMSaEjbU++llIDr/lZAQVAtqZvnPPX6mn/8kGIkqI/MeA/1IiN856OzcU5UZm/ZQ1NDJI4/m8Kl2tn4L9qg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; 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=lW9THoQ9BZs1EA92DYLc+8Ja33u2uoLlyo2BgS6FQq4=; b=uRFKbwhWonRB4VpsEmFqS2wqwO/MNeGVGupDIAaEwESs55q8pwmC+sCbHU1I0JYS5pTeHoEwz3qxpWSmKgoAjUk30JIhnnbgtCSRnk1MfaUq2RUi1YwcEdoo32+3gsLxkHGwdUhuwmkwsJBfT8ppm3HtCt2Fe7isg8OnENYKNJv3RkCV5ELRVfGsZ4/QbA4baENf29src8QoMDE80JYkg5AYUuqxWzcNjRrNWViVUEqZ1mT2FCP/hw6Eb3CW6iUWPw9rAkqO3oEodiLsMZC9eCj4G+hMnLuwLgdaWo+Xek/cGjqCVtFrvK018e1qPYU67cnL6M2d6iJ9snqkWhveGQ== Received: from MWHPR12MB1295.namprd12.prod.outlook.com (2603:10b6:300:11::11) by DM5PR12MB1706.namprd12.prod.outlook.com (2603:10b6:3:10f::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.13; Wed, 26 Jan 2022 09:45:19 +0000 Received: from MW2PR12MB4666.namprd12.prod.outlook.com (2603:10b6:302:13::22) by MWHPR12MB1295.namprd12.prod.outlook.com (2603:10b6:300:11::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.8; Wed, 26 Jan 2022 09:45:18 +0000 Received: from MW2PR12MB4666.namprd12.prod.outlook.com ([fe80::78:438a:c6b7:1cc1]) by MW2PR12MB4666.namprd12.prod.outlook.com ([fe80::78:438a:c6b7:1cc1%3]) with mapi id 15.20.4909.019; Wed, 26 Jan 2022 09:45:18 +0000 From: Ori Kam To: Bruce Richardson CC: Jerin Jacob , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Alexander Kozyrev , dpdk-dev , Ivan Malov , Andrew Rybchenko , Ferruh Yigit , "mohammad.abdul.awal@intel.com" , Qi Zhang , Jerin Jacob , Ajit Khaparde , David Marchand , Olivier Matz , Stephen Hemminger Subject: RE: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints Thread-Topic: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints Thread-Index: AQHYDIBlr97eIE8EiUKphVVj8UOlEqxyRq0AgAAx7gCAAAL1gIAABlkAgAFK2cCAAEewAIAAAT0AgAAGo/A= Date: Wed, 26 Jan 2022 09:45:18 +0000 Message-ID: References: <20211006044835.3936226-1-akozyrev@nvidia.com> <20220118153027.3947448-2-akozyrev@nvidia.com> <4371468.LvFx2qVVIh@thomas> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 0f054891-003a-427a-bfbc-08d9e0b0900b x-ms-traffictypediagnostic: MWHPR12MB1295:EE_|DM5PR12MB1706:EE_ x-ld-processed: 43083d15-7273-40c1-b7db-39efd9ccc17a,ExtAddr,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: HeIeqgHl0a6Rl4xvYqVhzV5Oma4QcWB7E3NYcusI25B3PBsDXYvWOcGxdmR74rXmYVZ8TDoPpRsASnxYEL9Tj9WA2YtQF+jX8+5w9QiGqwS7McQ8kyKvdaD5ehEAKIurctzu1fNG5N7KOUXkdqWW+cE/V+Ltfszk3SERUAWi136qj4C4lIuvuWw5QcPSCCci8E3IOJW6KsR92D1vux+QpeKRbMaHwxdgUcVuvHHJMe2LeEYC7Pjf1BfU5teVtdUlumQsU9vgr6/8GlXguVtjA7HIn//M0avfm06xekJjfgP+DkZEQV9ShUhC3wBWITT7yf/t++gR1seKXJ6Q3OIPlT6gzt4tX9t7TjDXcVNNv9Zyg8vr/beW2V7My0DmfcxrVgFfhdjnS16fYQYZgNfeXDb7EBw2SEFn+30Y+h0kkQs8Y4tffpv5PCglZeTq1/4+i3jKnqrNly2yDhsjQaWevAGNlYkHMN2p9MoZRaJ/XaXU+7s5WImPTn4W/fza02WfLB0ElHi77y3aKbdL7XX68SaJRXVZM9G14GJ+uw7kHIkwoxmAdTCPF/KDaT8HatwjSovBcJjSy1DBG1d56I55SnAeu3aDqhu5r6hjUi3mV9l/kXXg3ENXFmsvP9ZM2SaXvmHmh4cXzWVoehxsAl+Cv7NpuQF6Q5nZRs0AYF9L2i05Oe1ya/HtpcdrKLnbwHHXcPlTfWmqChBhRC0SHJ9cP0OaYbX/crnMwlyQLiPN2/cdm6Vh+j1i4qFvucGGZgxWBSXaK48Hu+lDq8HCeI2ATsERC3vlPEhK6Hbf8rsvvVY= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MWHPR12MB1295.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(8676002)(186003)(4326008)(9686003)(6916009)(71200400001)(26005)(66946007)(2906002)(55016003)(8936002)(122000001)(966005)(52536014)(76116006)(33656002)(38070700005)(7696005)(66446008)(53546011)(6506007)(86362001)(508600001)(54906003)(5660300002)(66556008)(66476007)(64756008)(316002)(7416002)(38100700002)(83380400001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?YuU2aYO+pO2j2gKOCUbu+l4X2hFvmm1UV1qFVo2z5JExk4LlgeZWzlY4iawQ?= =?us-ascii?Q?cMvGm25O+zBfBvPuJgxoxUKtIE4sgjQFlLnRn8p3XHFD0wwn0AOBrYJTjezJ?= =?us-ascii?Q?S2VFJCI3v1U4uxqk2pizU/rorMCseLTmIkESpfPosjCQAu+ExvSbk/5eAMeq?= =?us-ascii?Q?gYaQQ/x5pf8Qf4HCGEyN+pLSsXmd5acABaK9+sZ2n3Vsyb9m3kyHUAVvYhFP?= =?us-ascii?Q?s40H6OPmpUxsjLl4jFMOVTA0aV+KFL9DLaGy+A4BIKUDsn0HbUYinKJe5kSI?= =?us-ascii?Q?Cb4Or6prEXGD/S9XnkHjFQt6oOrPZYCWrIluWKvtpxNPiup7oOa40VtAg2dg?= =?us-ascii?Q?YCUkrM/EUfR+b2lj7a1Wi4XLhJKDQFSdSYAhDNBd9rd/JpZbS0w7kB7l6Zuf?= =?us-ascii?Q?pC1Gn/PvegMhc567l2e/1SJ3QAdeI+kH4O4kPU3FxsY9//vmHrzoPE/3SlEF?= =?us-ascii?Q?dK/B6B0g+9Hjc2LFHraHqgDfFJujRQlr+9ObSE6+tLjLYhnHMwvKNEfrmWsL?= =?us-ascii?Q?4lFhn6SPqB05uKVU4kRsUILYZrpiy/wB1iTOEYR9uM+jYQDyj/GV4OwzHXyL?= =?us-ascii?Q?kj2A7knHMjnVGsZa9wWsj5A5CDFg/V7ABITrZm1+sRCBKC3RzUFF3iTy9AuU?= =?us-ascii?Q?+BXZm+CpuZDhAWzFJVJyJmJmdsJ8xu0sv30q7Iv9awLiAvQntubjKUdjJ2/B?= =?us-ascii?Q?XA+0QipRNC3yV3gYay6Bz5Ne7LHMJcktaAzuRam35LJSduggNX/LWy3rMm/P?= =?us-ascii?Q?2KosjwW8mdMdVrzFBHDDpDvyeDunNjrnRZS1AiAjF0VeJj0dYcGFHYzsgrPv?= =?us-ascii?Q?qQ5bCVO8mM2lEG2pAE5u2whqDvhPZV6UKVnM5+Xj3kKqFxUBd+PRX215uBVM?= =?us-ascii?Q?c5nLu44cYRznZKXPJwXBrrKGEH2FCfVSLEP3cHuiGD5NecpK8qqS11umcFus?= =?us-ascii?Q?WtFllB7p5REENwawx676uzTC+lWwggbcpU8szpXX9TyV2l5TFtRvs6+cygwt?= =?us-ascii?Q?bU01NRqn1ZwIlPQ3dUNpD4EMrn6T9PMW9DfwQEsoZPv8+js8xn5GDMERbfyN?= =?us-ascii?Q?ZeL2Aou07Ekh+jZ+s0Bau7i7sH/6wko3Q5XOHebSm6v0z4vjZp7i71ogcNnO?= =?us-ascii?Q?ltk1MUBhDZNCxLtHAyX5A9vuS4Jj5FlYm2IpqneGlLEvQRcJl7epn6d8uQA3?= =?us-ascii?Q?ALX0MHQsNlJqvguT0W1wunDMjLAPWEBJj8OL4xEcyy6yGuTyWITNWnCl1cxA?= =?us-ascii?Q?S7JUnRgHNQ5JvY+MK1fErSrtLUgIE4+3KilNOPsWl7AZSxRWzb8KtsHA3uIm?= =?us-ascii?Q?qAaGZ2FiLc58DvhVl8M7lu3eB63zhkDBsY3JepzrU3ZHU+0TOejeewtzDxzQ?= =?us-ascii?Q?2ELhnxLFVRFnj+my2V5m4MhywGt2mHFz/eVOEqN/59hAJvSBcSfqCTeMZSF0?= =?us-ascii?Q?BBKvNUCNyarF0MiN5xmrxApZG/l8drxmGmtQbNt9tiCrDt05QVEUNdAeZv0q?= =?us-ascii?Q?0DVgEDsC7EDYsh8mMFiB9gxu0wtmU2fhNlA10CiGMykj1wZrXsXHPlmxGW0n?= =?us-ascii?Q?7/OTUKoQTXdc7ON+lTkzhyFbM23nLz4xbUrAi7HeQ+rUY/f4zU6vu5m9y1Ou?= =?us-ascii?Q?/94Z9J4+zAI0d6OA2flMsfk=3D?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW2PR12MB4666.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 0f054891-003a-427a-bfbc-08d9e0b0900b X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jan 2022 09:45:18.4565 (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: ukne6T2ARsoJXEHuFFxIn3frc7kRqE/5F9I4jd3ez+28fzBwor/1u7Dp2du9CEFjZn6mupg/Ap9nERdtwN869g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1706 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 > -----Original Message----- > From: Bruce Richardson > Sent: Tuesday, January 25, 2022 8:14 PM > To: Ori Kam > Subject: Re: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hi= nts >=20 > On Tue, Jan 25, 2022 at 06:09:42PM +0000, Bruce Richardson wrote: > > On Tue, Jan 25, 2022 at 03:58:45PM +0000, Ori Kam wrote: > > > Hi Bruce, > > > > > > > -----Original Message----- From: Bruce Richardson > > > > Sent: Monday, January 24, 2022 8:09 PM > > > > Subject: Re: [PATCH v2 01/10] ethdev: introduce flow > > > > pre-configuration hints > > > > > > > > On Mon, Jan 24, 2022 at 11:16:15PM +0530, Jerin Jacob wrote: > > > > > On Mon, Jan 24, 2022 at 11:05 PM Thomas Monjalon > > > > > wrote: > > > > > > > > > > > > 24/01/2022 15:36, Jerin Jacob: > > > > > > > On Tue, Jan 18, 2022 at 9:01 PM Alexander Kozyrev > > > > > > > wrote: > > > > > > > > +struct rte_flow_port_attr { + /** + * Version > > > > > > > > of the struct layout, should be 0. + */ + > > > > > > > > uint32_t version; > > > > > > > > > > > > > > Why version number? Across DPDK, we are using dynamic functio= n > > > > > > > versioning, I think, that would be sufficient for ABI > > > > > > > versioning > > > > > > > > > > > > Function versioning is not ideal when the structure is accessed > > > > > > in many places like many drivers and library functions. > > > > > > > > > > > > The idea of this version field (which can be a bitfield) is to > > > > > > update it when some new features are added, so the users of the > > > > > > struct can check if a feature is there before trying to use it. > > > > > > It means a bit more code in the functions, but avoid duplicatin= g > > > > > > functions as in function versioning. > > > > > > > > > > > > Another approach was suggested by Bruce, and applied to dmadev. > > > > > > It is assuming we only add new fields at the end (no removal), > > > > > > and focus on the size of the struct. By passing sizeof as an > > > > > > extra parameter, the function knows which fields are OK to use. > > > > > > Example: > > > > > > http://code.dpdk.org/dpdk/v21.11/source/lib/dmadev/rte_dmadev.c= #L476 > > > > > > > > > > + @Richardson, Bruce Either approach is fine, No strong opinion. > > > > > We can have one approach and use it across DPDK for consistency. > > > > > > > > > > > > > In general I prefer the size-based approach, mainly because of its > > > > simplicity. However, some other reasons why we may want to choose i= t: > > > > > > > > * It's completely hidden from the end user, and there is no need fo= r > > > > an extra struct field that needs to be filled in > > > > > > > > * Related to that, for the version-field approach, if the field is > > > > present in a user-allocated struct, then you probably need to start > > > > preventing user error via: - having the external struct not have th= e > > > > field and use a separate internal struct to add in the version info > > > > after the fact in the versioned function. Alternatively, - provide = a > > > > separate init function for each structure to fill in the version > > > > field appropriately > > > > > > > > * In general, using the size-based approach like in the linked > > > > example is more resilient since it's compiler-inserted, so there is > > > > reduced chance of error. > > > > > > > > * A sizeof field allows simple-enough handling in the drivers - > > > > especially since it does not allow removed fields. Each driver only > > > > needs to check that the size passed in is greater than that expecte= d, > > > > thereby allowing us to have both updated and non-updated drivers > > > > co-existing simultaneously. [For a version field, the same scheme > > > > could also work if we keep the no-delete rule, but for a bitmask > > > > field, I believe things may get more complex in terms of checking] > > > > > > > > In terms of the limitations of using sizeof - requiring new fields = to > > > > always go on the end, and preventing shrinking the struct - I think > > > > that the simplicity gains far outweigh the impact of these > > > > strictions. > > > > > > > > * Adding fields to struct is far more common than wanting to remove > > > > one > > > > > > > > * So long as the added field is at the end, even if the struct size > > > > doesn't change the scheme can still work as the versioned function > > > > for the old struct can ensure that the extra field is appropriately > > > > zeroed (rather than random) on entry into any driver function > > > > > > > > > > Zero can be a valid value so this is may result in an issue. > > > > > > > In this instance, I was using zero as a neutral, default-option value. = If > > having zero as the default causes problems, we can always make the > > structure size change to force a new size value. > > > > > > * If we do want to remove a field, the space simply needs to be > > > > marked as reserved in the struct, until the next ABI break release, > > > > when it can be compacted. Again, function versioning can take care = of > > > > appropriately zeroing this field on return, if necessary. > > > > > > > > > > This means that PMD will have to change just for removal of a field I > > > would say removal is not allowed. > > > > > > > My 2c from considering this for the implementation in dmadev. :-) > > > > > > Some concerns I have about your suggestion: 1. The size of the struct > > > is dependent on the system, for example Assume this struct { Uint16_t > > > a; Uint32_t b; Uint8_t c; Uint32_t d; } Incase of 32 bit machine the > > > size will be 128 bytes, while in 64 machine it will be 96 > > > > Actually, I believe that in just about every system we support it will = be > > 4x4B i.e. 16 bytes in size. How do you compute 96 or 128 byte sizes? In > > any case, the actual size value doesn't matter in practice, since all > > sizes should be computed by the compiler using sizeof, rather than > > hard-coded. > > You are correct my mistake with the numbers. I still think there might be some issue but I can't think of anything. So dropping it. > > > > > > 2. ABI breakage, as far as I know changing size of a struct is ABI > > > breakage, since if the application got the size from previous version > > > and for example created array or allocated memory then using the new > > > structure will result in memory override. > > > > > > I know that flags/version is not easy since it means creating new > > > Structure for each change. I prefer to declare that size can change > > > between DPDK releases is allowd but as long as we say ABI breakage is > > > forbidden then I don't think your solution is valid. And we must go > > > with the version/flags and create new structure for each change. > > > > > > > whatever approach is taken for this, I believe we will always need to > > create a new structure for the changes. This is because only functions > > can be versioned, not structures. The only question therefore becomes h= ow > > to pass ABI version information, and therefore by extension structure > > version information across a library to driver boundary. This has to be > > an extra field somewhere, either in a structure or as a function > > parameter. I'd prefer not in the structure as it exposes it to the user= . > > In terms of the field value, it can either be explicit version info as > > version number or version flags, or implicit versioning via "size". Bas= ed > > off the "YAGNI" principle, I really would prefer just using sizes, as > > it's far easier to manage and work with for all concerned, and requires > > no additional documentation for the programmer or driver developer to > > understand. > > > As a third alternative that I would find acceptable, we could also just > take the approach of passing the ABI version explicitly across the functi= on > call i.e. 22 for DPDK_21.11. I'd find this ok too on the basis that it's > largely self explanatory, and can be inserted automatically by the compil= er > - again reducing chances of errors. [However, I also believe that using > sizes is still simpler again, which is why it's still my first choice! :-= )] >=20 Just to make sure I fully understand your suggestion. We will create new struct for each change. The function will stay the same For example I had the following: Struct base { Uint32_t x; } Function (struct base *input) { Inner_func (input, sizeof(struct base)) } Now I'm adding new member so it will look like this: Struct new { Uint32_t x; Uint32_t y; } When I want to call the function I need to cast Function((struct base*) new)=20 Right? This means that in both cases the sizeof wil return the same value, What am I missing? =20 > /Bruce