> -----Original Message----- > From: Morten Brørup <m...@smartsharesystems.com> > Sent: Wednesday, October 12, 2022 11:41 AM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; Jerin Jacob > <jerinjac...@gmail.com> > Cc: Jerin Jacob <jer...@marvell.com>; dev@dpdk.org; Li, WeiyuanX > <weiyuanx...@intel.com>; Thomas Monjalon <tho...@monjalon.net>; Ferruh Yigit > <ferruh.yi...@amd.com>; Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> > Subject: RE: rte_event_dev_xstats_reset id type > > > From: Van Haaren, Harry [mailto:harry.van.haa...@intel.com] > > Sent: Wednesday, 12 October 2022 12.30 > > > > > -----Original Message----- > > > From: Jerin Jacob <jerinjac...@gmail.com> > > > Sent: Wednesday, October 12, 2022 10:45 AM > > > > > > On Wed, Oct 12, 2022 at 1:40 PM Morten Brørup > > <m...@smartsharesystems.com> > > > wrote: > > > > > > > > Hi Jerin (eventdev maintainer), > > > > > > + harry.van.haa...@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
