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

[Ori Kam]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson at intel.com>
> Sent: Tuesday, January 25, 2022 8:14 PM
> To: Ori Kam <orika at nvidia.com>
> Subject: Re: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints
> 
> 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 at 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 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
> > > >
> > >
> > > 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.
> >
You are correct my mistake with the numbers.
I still think there might be some issue but I can't think of anything.
So dropping it.

> > >
> > > 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! :-)]
> 

Just to make sure I fully understand your suggestion.
We will create new struct for each change.
The function will  stay the same
For example I had the following:

Struct base {
 Uint32_t x;
}

Function (struct base *input)
{
	Inner_func (input, sizeof(struct base))
}

Now I'm adding new member so it will look like this:
Struct new {
 Uint32_t x;
Uint32_t y;
}

When I want to call the function I need to cast
Function((struct base*) new) 

Right?

This means that in both cases the sizeof wil return the same value,
What am I missing?
 
> /Bruce