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 781A6A04A8; Wed, 26 Jan 2022 14:41:22 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1C6FC4272D; Wed, 26 Jan 2022 14:41:22 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 6E26A42716 for ; Wed, 26 Jan 2022 14:41:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643204479; x=1674740479; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=Q3VfojIUBkmtwRfAY6+l4ICkbkinEdCr0SO8J3DaaRU=; b=A1/x+z0o34D50KjZfu5wB2zG2O4QUZzo74nha9Z1sLNFSnpUqchxjp0/ JOKqZJk55u96isRtZHTOAluriIZUMwMcin43f3SHrPF9e07MSwr3ac+Os T2O+W+DcpIxu9V1hDTv34JdWefOxlEodGvoTUO6qbp902Oo0ZQNASlMPF BWv/FJr/9fzXcZUYY/E7MhZpIpHPcK6U2DW2rg1S1FaSNHDdtJIoDH4nH xcehpmO1qBCgPFyIPFP+uVX5OQvX513nLoFyPRI+ri5DW6PmTuwbSrtQ5 HL3VJMFNbC6kWCY66X9nQ15nfySFVbNnC0UuucCOMAWEcw9YW6IPobS2p g==; X-IronPort-AV: E=McAfee;i="6200,9189,10238"; a="227221766" X-IronPort-AV: E=Sophos;i="5.88,318,1635231600"; d="scan'208";a="227221766" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2022 05:41:17 -0800 X-IronPort-AV: E=Sophos;i="5.88,318,1635231600"; d="scan'208";a="495358264" Received: from lmullane-mobl.ger.corp.intel.com (HELO bricha3-MOBL.ger.corp.intel.com) ([10.252.16.194]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Jan 2022 05:41:14 -0800 Date: Wed, 26 Jan 2022 13:41:11 +0000 From: Bruce Richardson To: Ori Kam Cc: "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Jerin Jacob , 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> <8883807.CDJkKcVGEf@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 12:19:43PM +0000, Ori Kam wrote: > > > > -----Original Message----- > > From: Thomas Monjalon > > Sent: Wednesday, January 26, 2022 1:22 PM > > Subject: Re: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints > > > > 26/01/2022 11:52, Bruce Richardson: > > > 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. > > > > Yes, this is the kind of explanation we need in our guideline doc. > > Alternatives can be documented as well. > > If we can list pros/cons in the doc, it will be easier to choose > > the best approach and to explain the choice during code review. > > > > > Thanks you very much for the clear explanation. > > The draw back is that we need also to duplicate the functions. > Using the flags/version we only need to create new structures > and from application point of view it knows what exta fields it gets. > (I agree that application knowledge has downsides but also advantages) > > In the case of flags/version your example will look like this (this is for the record and may other > developers are intrested): > > struct x_dev_cfg { //original struct > int ver; > int x; > }; > > struct x_dev_cfg_v2 { // new struct > int ver; > int x; > bool flag; // new field; > }; > > > The function is always the same function: > 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); > } > > When calling this function with old struct: > X_dev_cfg(id, (struct x_dev_cfg *)cfg) > > When calling this function with new struct: > X_dev_cfg(id, (struct x_dev_cfg *)cfg_v2) > > In PMD: > If (cfg->ver >= 2) > // version 2 logic > Else If (cfg->v >=0) > // base version logic > > > When using flags it gives even more control since pmd can tell exactly what > features are required. > > All options have prons/cons > I vote for the version one. > > We can have a poll 😊 > Or like Thomas said list pros and cons and each subsystem can > have it own selection. The biggest issue I have with this version approach is how is the user meant to know what version number to put into the structure? When the user upgrades from one version of DPDK to the next, are they manually to update their version numbers in all their structures? If they don't, they then may be mistified if they use the newer fields and find that they "don't work" because they forgot that they need to update the version field to the newer version at the same time. The reason I prefer the size field is that it is impossible for the end user to mess things up, and the entirity of the mechanism is internal, and hidden from the user. Regards, /Bruce