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 C739AA00BE; Tue, 25 Jan 2022 19:09:52 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 55D384116A; Tue, 25 Jan 2022 19:09:52 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id E791341161 for ; Tue, 25 Jan 2022 19:09:50 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643134191; x=1674670191; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=65ivAKuLT/HLO1AJ8mao4FD1ioSx0uCpvJCKmRbmzG4=; b=UOY9+KUUpiDduuDNf9HlzaaE7VhqMKgiNodXkDkoQ2In9GaiQsaNTNZ/ NRhcZcrUKfF782fCQGFvnXghPVxyImvL+z5T4xCO0vNhAvLOK/5EuncuW go5a0aGW5uVc0deWqb+wRjs9ZoF+yLvLKOgRaY+vgcb6UKdW7GCUbJ+33 Q9HitogePVDmg5huPTGYQpPpKA7HNV3HRT4A9H515s/8N6dqUVPAocphw iKbiK0SRQ+ZjXwfkoo42u+/06mCjcordjXDE85oHtb98uWDGi1czkpBx1 T7DkqT4Wl5TWuAoj0JswdTxRiwCnAP1JlaJ3SMnI+kWlSgkuTlmJMUtJU Q==; X-IronPort-AV: E=McAfee;i="6200,9189,10238"; a="307084807" X-IronPort-AV: E=Sophos;i="5.88,315,1635231600"; d="scan'208";a="307084807" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jan 2022 10:09:49 -0800 X-IronPort-AV: E=Sophos;i="5.88,315,1635231600"; d="scan'208";a="477199545" 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:09:45 -0800 Date: Tue, 25 Jan 2022 18:09:42 +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 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. Regards, /Bruce