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

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

On Tue, Jan 25, 2022 at 06:09:42PM +0000, Bruce Richardson wrote:
> On Tue, Jan 25, 2022 at 03:58:45PM +0000, Ori Kam wrote:
> > 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.
> > 
> 
> In this instance, I was using zero as a neutral, default-option value. If
> having zero as the default causes problems, we can always make the
> structure size change to force a new size value.
> 
> > > * 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
> 
> Actually, I believe that in just about every system we support it will be
> 4x4B i.e. 16 bytes in size. How do you compute 96 or 128 byte sizes? In
> any case, the actual size value doesn't matter in practice, since all
> sizes should be computed by the compiler using sizeof, rather than
> hard-coded.
> 
> > 
> > 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.
> > 
> 
> whatever approach is taken for this, I believe we will always need to
> create a new structure for the changes. This is because only functions
> can be versioned, not structures. The only question therefore becomes how
> to pass ABI version information, and therefore by extension structure
> version information across a library to driver boundary. This has to be
> an extra field somewhere, either in a structure or as a function
> parameter. I'd prefer not in the structure as it exposes it to the user.
> In terms of the field value, it can either be explicit version info as
> version number or version flags, or implicit versioning via "size". Based
> off the "YAGNI" principle, I really would prefer just using sizes, as
> it's far easier to manage and work with for all concerned, and requires
> no additional documentation for the programmer or driver developer to
> understand.
> 
As a third alternative that I would find acceptable, we could also just
take the approach of passing the ABI version explicitly across the function
call i.e. 22 for DPDK_21.11. I'd find this ok too on the basis that it's
largely self explanatory, and can be inserted automatically by the compiler
- again reducing chances of errors. [However, I also believe that using
sizes is still simpler again, which is why it's still my first choice! :-)]

/Bruce