> -----Original Message-----
> From: Thomas Monjalon <tho...@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.
