Re: rte_event_dev_xstats_reset id type

[Thomas Monjalon <thomas@monjalon.net>]

12/10/2022 18:16, Jerin Jacob:
> On Wed, Oct 12, 2022 at 9:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Wednesday, 12 October 2022 17.13
> > >
> > > 12/10/2022 14:14, Van Haaren, Harry:
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > > > On Wed, Oct 12, 2022 at 1:40 PM Morten Brørup wrote:
> > > > > > > >
> > > > > > > > Hi Jerin (eventdev maintainer),
> > > > > > >
> > > > > > > + harry.van.haaren@intel.com as the changes in
> > > drivers/event/sw.
> > > > > >
> > > > > > Thanks Jerin.
> > > > > >
> > > > > >
> > > > > > > > While looking into bug #1101 [1], I noticed a mix of unsigned
> > > int
> > > > > > and uint32_t in
> > > > > > > the test code, which will fail on 64-bit big endian CPUs.
> > > > > >
> > > > > > Aha; that we can fix. I am curious why this isn't found in
> > > CI/reported
> > > > > > before.
> > > > >
> > > > > We probably don't test any 64-bit *big endian* architectures. Just
> > > a guess.
> > > >
> > > > Seems so yes.
> > > >
> > > > > > > > Specifically, rte_event_dev_xstats_reset() is called with the
> > > "ids"
> > > > > > parameter
> > > > > > > pointing to an unsigned int [2], but that parameter is a
> > > pointer to
> > > > > > an uint32_t.
> > > > > > > >
> > > > > > > > I think the type of the ids array parameter to
> > > > > > rte_event_dev_xstats_reset() should
> > > > > > > be changed to unsigned int array, like in the other
> > > > > > rte_event_dev_xxx() functions.
> > > > > >
> > > > > > In this case, we have the option to change the type of a variable
> > > in a
> > > > > > test-case, or change API and cause API/ABI breakage.
> > > > >
> > > > > Well.. yes, but I would phrase that last option: Change the
> > > API/ABI, so related
> > > > > functions consistently use the same type for the same variable,
> > > instead of randomly
> > > > > mixing uint64_t, uint32_t and unsigned int, depending on function.
> > > >
> > > > Aah ok; I see your point now; there is inconsistent usage of
> > > uint32_t/unsigned int
> > > > between the Eventdev APIs itself. Agree this is sub-optimal, and
> > > would have been
> > > > nice to have spotted before the Eventdev API was stabilized.
> > > >
> > > >
> > > > > Unfortunately, these functions are not marked experimental, so
> > > breaking API/ABI is
> > > > > hard to do. :-(
> > > >
> > > > Agreed again.
> > >
> > > 22.11 is a breaking release,
> > > and changing type in the API is not much impactful,
> > > so that's something you can change now,
> > > or be quiet forever :)
> >
> > Question:
> > 1. Only change the "xstats id" type in the one eventdev function, which deviates from other eventdev functions, or
> > 2. Change the "xstats id" type for all xstats functions across all device types, for consistency across device types?
> >
> > If 2, then what would be a good type?
> 
> +1 for second option and the type as uint32_t
> 
> >
> > Ethdev uses uint64_t for xstats id, and (speaking without knowledge about its internals) that seems like overkill to me. Arrays of these are being used, so size does matter.

uint64_t is not overkill if you consider having stats per queue with a predictable scheme.
That's an improvement I would like to work on,
so I would like to keep uint64_t please.