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

[Ori Kam <orika@nvidia.com>]

Hi Bruce,

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, January 24, 2022 8:09 PM
> Subject: Re: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints
> 
> 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@monjalon.net> wrote:
> > >
> > > 24/01/2022 15:36, Jerin Jacob:
> > > > On Tue, Jan 18, 2022 at 9:01 PM Alexander Kozyrev <akozyrev@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
> 

Zero can be a valid value so this is may result in an issue.

> * 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.
> 

This means that PMD will have to change just for removal of a field
I would say removal is not allowed.

> My 2c from considering this for the implementation in dmadev. :-)

Some concerns I have about your suggestion:
1. The size of the struct is dependent on the system, for example
Assume this struct 
{
Uint16_t a;
Uint32_t b;
Uint8_t c;
Uint32_t d;
}
Incase of 32 bit machine the size will be 128 bytes, while in 64 machine it will be 96

2. ABI breakage, as far as I know changing size of a struct is ABI breakage, since if 
the application got the size from previous version and for example created array
or allocated memory then using the new structure will result in memory override.

I know that flags/version is not easy since it means creating new 
Structure for each change. I prefer to declare that size can change between
DPDK releases is allowd but as long as we say ABI breakage is forbidden then I don't think your
solution is valid.
And we must go with the version/flags and create new structure for each change.

Best,
Ori
> 
> /Bruce