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 6C8D6A04AD; Mon, 24 Jan 2022 19:09:08 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EED114117E; Mon, 24 Jan 2022 19:09:07 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id AC53A4117A for ; Mon, 24 Jan 2022 19:09:06 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643047746; x=1674583746; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=vf8SdxLlIulv6cvV3e1b76SXaEHZRfvHnlWl9NqtFWc=; b=NmMmfMZ/cs4eJWOiYX/ewi9yPBOfrVDhWwsy97IYC2n54sfjlEjlSmdm dmPdDKHKQsvz8boG7iEc6SpGtCbzQAYYO3IrNA/IZqZe74Js/vZ6oNRVw ut0m5kPzlRXy2yPyIVSw2hCPdSFhbB0AueNOIMXPDfc7GLagO4HLXOrjG ftKH0452KkZ48udEC30WVS2JL6JflKRuRtlu6E9LUVn06hu/RS6hLx4WP Q5XvEGFsxYbo+zW1Qiw0UIExsOodda8E+7INdBqG4aKV4VnYBIUrfmL+a qHRGsg1aKrsl11ntapCoDq/pU2zZkrZAugU/Jsy8Gnz8aXu4552vWKtUK g==; X-IronPort-AV: E=McAfee;i="6200,9189,10237"; a="246056438" X-IronPort-AV: E=Sophos;i="5.88,311,1635231600"; d="scan'208";a="246056438" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Jan 2022 10:09:05 -0800 X-IronPort-AV: E=Sophos;i="5.88,311,1635231600"; d="scan'208";a="596863244" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.31.187]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 24 Jan 2022 10:09:02 -0800 Date: Mon, 24 Jan 2022 18:08:58 +0000 From: Bruce Richardson To: Jerin Jacob Cc: Thomas Monjalon , Alexander Kozyrev , dpdk-dev , Ori Kam , 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 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 * 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. My 2c from considering this for the implementation in dmadev. :-) /Bruce