RE: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints

[Ori Kam <orika@nvidia.com>]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, January 26, 2022 3:41 PM
> To: Ori Kam <orika@nvidia.com>
> Subject: Re: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints
> 
> On Wed, Jan 26, 2022 at 12:19:43PM +0000, Ori Kam wrote:
> >
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > 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.
> 

The solution is simple when you define new struct in the struct you write what
should be the version number.
You can also define that 0 is the latest one, so application that are are writing code which
is size agnostic will just set 0 all the time.

 
> Regards,
> /Bruce