Re: xstats id type

[Thomas Monjalon <thomas@monjalon.net>]

13/10/2022 09:12, Pavan Nikhilesh Bhagavatula:
> From: Morten Brørup <mb@smartsharesystems.com>
> > +TO: rawdev maintainers, regexdev maintainers
> > 
> > > From: Thomas Monjalon [mailto:thomas@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
> > <thomas@monjalon.net>
> > > wrote:
> > > > >
> > > > > 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,
> > > >
> > > > 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:
> > > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__fast.dpdk.org_events_slides_DPDK-2D2019-2D09-2DEthernet-
> > 5FStatistics.pdf&d=DwIFAw&c=nKjWec2b6R0mOyPaz7xtfQ&r=1cjuAHrGh74
> > 5jHNmj2fD85sUMIJ2IPIDsIJzo6FN6Z0&m=ApdcbroZzSNlcY1t4c8iv9HZk6YSJOA
> > Hpg93zuyIEEWa6xkViBTdoCA3iir_FCtW&s=wEMA0lnyrTmxmmDINhzOagGvV
> > Z3TcIrzfK5NbJHafdM&e=
> > 
> > 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.
> 
> Harry, 
> Are you working on a patch for this change? If not I will do it.
> 
> Thomas,
> Are you ok to break the ABI without deprication notice i.e. make ID as u64 for eventdev?

Yes, it is only increasing size of function parameters, right?
The only problematic part is that the application must pass
a pointer of the right size, meaning some application code change.

It would be an exception in the process,
so I am Cc'ing the techboard for more opinions.