+TO: rawdev maintainers, regexdev maintainers > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Wednesday, 12 October 2022 22.44 > > 12/10/2022 18:47, Jerin Jacob: > > On Wed, Oct 12, 2022 at 9:58 PM Thomas Monjalon <tho...@monjalon.net> > wrote: > > > > > > 12/10/2022 18:16, Jerin Jacob: > > > > On Wed, Oct 12, 2022 at 9:05 PM Morten Brørup > <m...@smartsharesystems.com> wrote: > > > > > > > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > > > > Sent: Wednesday, 12 October 2022 17.13 > > > > > > > > > > > > 12/10/2022 14:14, Van Haaren, Harry: > > > > > > > From: Morten Brørup <m...@smartsharesystems.com> > > > > > > > > From: Van Haaren, Harry > [mailto:harry.van.haa...@intel.com] > > > > > > > > > From: Jerin Jacob <jerinjac...@gmail.com> > > > > > > > > > > On Wed, Oct 12, 2022 at 1:40 PM Morten Brørup 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. > > > > > > > > > > > > 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, > > > > You mean to use a bitmask hence uint64_t. > > Currently it is mapped as arrays so 2^64 stats may not be needed. > > > > No strong opinion, I was just curious to understand "stats per queue > > with a predictable scheme" and how uint64_t helps with that. > > Yes I mean some bits are used for the queue number. > Something like in slide 11 of this presentation: > http://fast.dpdk.org/events/slides/DPDK-2019-09-Ethernet_Statistics.pdf
With this presentation in mind, I strongly agree with Thomas that uint64_t is the best choice of type for xstats id. A quick search shows that both eventdev and rawdev use "unsigned int", except rte_event_dev_xstats_reset() and rte_rawdev_xstats_reset(), which both use uint32_t. And regexdev uses uint16_t. Other device APIs don't have xstats.
