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 6051EA00C2; Tue, 25 Jan 2022 19:14:46 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DA91F4116E; Tue, 25 Jan 2022 19:14:45 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 1B61641161 for ; Tue, 25 Jan 2022 19:14:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643134484; x=1674670484; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=5lymtX2OAbIUw2+XPAi0etHEjPkSqzSx1eEC0USpXVs=; b=i7/SnHn6oOu/ICoPfTsgBs41Aroh/IabmdcLtN3DqUaT6IDds+vIO6Ma xmwc6dxClBfXzJx5IVg64NJo9JUtNywLzalbtEWIRIOg2FbDaphbFNOX2 NPIAiJenOiVtKv3Vdja2+5/2LCxIHc0iqs7SRmwAdQ4bCLPfasR/IDSVv OGQvtDTQOhNLF3dYX8AHIvJ0l0Lf7vWbmgIkItXc6j/EwEUwwX90/o56v 5UFV/TCFkc8rDmA87DfuYk2XDwd0T+hKK4vnqDxwJxYLAVM4m9LNbTk15 s63brEVgFCDQr0sTKakvKBuAHRdyoLLIo8Um7PiLKr2kGzMFRBnqaSCnb Q==; X-IronPort-AV: E=McAfee;i="6200,9189,10238"; a="226348232" X-IronPort-AV: E=Sophos;i="5.88,315,1635231600"; d="scan'208";a="226348232" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jan 2022 10:14:15 -0800 X-IronPort-AV: E=Sophos;i="5.88,315,1635231600"; d="scan'208";a="477201367" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.31.171]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 25 Jan 2022 10:14:11 -0800 Date: Tue, 25 Jan 2022 18:14:08 +0000 From: Bruce Richardson To: Ori Kam 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 Message-ID: References: <20211006044835.3936226-1-akozyrev@nvidia.com> <20220118153027.3947448-2-akozyrev@nvidia.com> <4371468.LvFx2qVVIh@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 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 function > > > > > > 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 duplicating > > > > > 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 it: > > > > > > * It's completely hidden from the end user, and there is no need for > > > 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 the > > > 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 expected, > > > 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. > > > > > 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 how > 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". Based > 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 function 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 compiler - 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! :-)] /Bruce