RE: rte_event_dev_xstats_reset id type

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Wednesday, October 12, 2022 11:41 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Jerin Jacob
> <jerinjacobk@gmail.com>
> Cc: Jerin Jacob <jerinj@marvell.com>; dev@dpdk.org; Li, WeiyuanX
> <weiyuanx.li@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@amd.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: RE: rte_event_dev_xstats_reset id type
> 
> > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> > Sent: Wednesday, 12 October 2022 12.30
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Wednesday, October 12, 2022 10:45 AM
> > >
> > > On Wed, Oct 12, 2022 at 1:40 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > > 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.

> > Lets change the unit test code from "unsigned int" to uint32_t, and
> > that will fix the issue?
> >
> > From a quick review in the test code, there are 3x occurrences of
> > "unsigned int id" being used.
> > I will send a patch to change them later today.
> 
> A simple change to uint32_t would be incorrect.
> 
> rte_event_dev_xstats_by_name_get() uses unsigned int, not uint32_t.
> 
> Only rte_event_dev_xstats_reset() uses uint32_t.

Agreed, the fix needs to be aware of which func to call, will handle that in the patch.
Patch on the way to fix event/sw/selftest code.

> > > > Or even better, use the same type for an "xstats id" across all
> > device types. For
> > > ethdev devices, they are uint64_t, but I don't know why. (They are
> > passed around as
> > > arrays, so they could be 32 bit. I guess that they were originally
> > not used in arrays, so
> > > unsigned int seemed the logical choice.)
> > > >
> > > >
> > > > [1]: https://bugs.dpdk.org/show_bug.cgi?id=1101
> > > > [2]:
> > https://git.dpdk.org/dpdk/tree/drivers/event/sw/sw_evdev_selftest.c#n17
> > 66
> > > >
> > > >
> > > > Med venlig hilsen / Kind regards,
> > > > -Morten Brørup