On Monday, January 24, 2022 13:09 Bruce Richardson <bruce.richard...@intel.com>
wrote:
> On Mon, Jan 24, 2022 at 11:16:15PM +0530, Jerin Jacob wrote:
> > On Mon, Jan 24, 2022 at 11:05 PM Thomas Monjalon
> <tho...@monjalon.net> wrote:
> > >
> > > 24/01/2022 15:36, Jerin Jacob:
> > > > On Tue, Jan 18, 2022 at 9:01 PM Alexander Kozyrev
> <akozy...@nvidia.com> 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
>
> * 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.
>
> My 2c from considering this for the implementation in dmadev. :-)
>
> /Bruce
Thank you for the suggestions. I have no objections in adopting a size-based
approach.
I can keep versions or switch to sizeof as long as we can agree on some uniform
way.
