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

[Alexander Kozyrev]

On Monday, January 24, 2022 13:09 Bruce Richardson <bruce.richardson at 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
> <thomas at monjalon.net> wrote:
> > >
> > > 24/01/2022 15:36, Jerin Jacob:
> > > > On Tue, Jan 18, 2022 at 9:01 PM Alexander Kozyrev
> <akozyrev at 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.