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 8D829A04A8; Wed, 26 Jan 2022 11:52:14 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 14DFA42719; Wed, 26 Jan 2022 11:52:14 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 9094742716 for ; Wed, 26 Jan 2022 11:52:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643194332; x=1674730332; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=n4FGdDPHk3768Hzew3lM6ro/PjMnkjNiXr9yv+zdw9k=; b=aa2mG5/y2EeNQ3bD6Xxg+FCN4JundL/C6gp1SC7B7merA06VefslwJ/+ vU0/elm/RXZR2kbsNWApXat6Mtf5ZIE2jCFOxuTI+oZLHQXlm9a0C4lmS 8ic8gMJROKCHTzIG+S0sgr3CmVJpGTg//sqSkabkhxqV57qCfN7tfVecx dbhQODrn97xZeh30Ox5kZlGrnFFnYwpTyMWjvXtE+3jicEtY8fk77zbxi hnZBiS89ffH5LEpRwY9DCP7I8PByRSFJIROdRTYaB3kdlFdTmzHtNuMCD IEO1oKzJyCkogbHzKbdG30rakwzBKIJ7TRCDJGwPOawUtqxkmqML19FSu g==; X-IronPort-AV: E=McAfee;i="6200,9189,10238"; a="270975959" X-IronPort-AV: E=Sophos;i="5.88,317,1635231600"; d="scan'208";a="270975959" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2022 02:52:11 -0800 X-IronPort-AV: E=Sophos;i="5.88,317,1635231600"; d="scan'208";a="495322333" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.16.35]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Jan 2022 02:52:07 -0800 Date: Wed, 26 Jan 2022 10:52:04 +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 Wed, Jan 26, 2022 at 09:45:18AM +0000, Ori Kam wrote: > > > > -----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 hints > > > > 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. > > > > 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 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! :-)] > > > > 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) > > Right? > > This means that in both cases the sizeof wil return the same value, > What am I missing? > The scenario is as follows. Suppose we have the initial state as below: struct x_dev_cfg { int x; }; int x_dev_cfg(int dev_id, struct x_dev_cfg *cfg) { struct x_dev *dev = x_devs[id]; // some setup/config may go here return dev->configure(cfg, sizeof(cfg)); // sizeof(cfg) == 4 } Now, supposing we need to add in a new field into the config structure, a very common occurance. This will indeed break the ABI, so we need to use ABI versioning, to ensure that apps passing in the old structure, only call a function which expects the old structure. Therefore, we need a copy of the old structure, and a function to work on it. This gives this result: struct x_dev_cfg { int x; bool flag; // new field; }; struct x_dev_cfg_v22 { // needed for ABI-versioned function int x; }; /* this function will only be called by *newly-linked* code, which uses * the new structure */ int x_dev_cfg(int dev_id, struct x_dev_cfg *cfg) { struct x_dev *dev = x_devs[id]; // some setup/config may go here return dev->configure(cfg, sizeof(cfg)); // sizeof(cfg) is now 8 } /* this function is called by apps linked against old version */ int x_dev_cfg_v22(int dev_id, struct x_dev_cfg_v22 *cfg) { struct x_dev *dev = x_devs[id]; // some setup/config may go here return dev->configure((void *)cfg, sizeof(cfg)); // sizeof(cfg) is still 4 } With the above library code, we have different functions using the different structures, so ABI compatibility is preserved - apps passing in a 4-byte struct call a function using the 4-byte struct, while newer apps can use the 8-byte version. The final part of the puzzle is then how drivers react to this change. Originally, all drivers only use "x" in the config structure because that is all that there is. That will still continue to work fine in the above case, as both 4-byte and 8-byte structs have the same x value at the same offset. i.e. no driver updates for x_dev is needed. On the other hand, if there are drivers that do want/need the new field, they can also get to use it, but they do need to check for its presence before they do so, i.e they would work as below: if (size_param > struct(x_dev_cfg_v22)) { // or "== struct(x_dev_cfg)" // use flags field } Hope this is clear now. /Bruce