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

[Bruce Richardson <bruce.richardson@intel.com>]

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.

Regards,
/Bruce