RE: xstats id type

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

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Sent: Thursday, October 13, 2022 8:13 AM
> To: Morten Brørup <mb@smartsharesystems.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob <jerinjacobk@gmail.com>; Sachin Saxena
> <sachin.saxena@oss.nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Ori
> Kam <orika@nvidia.com>; Liron Himi <lironh@marvell.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; dev@dpdk.org; Li, WeiyuanX <weiyuanx.li@intel.com>;
> Ferruh Yigit <ferruh.yigit@amd.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; david.marchand@redhat.com
> Subject: RE: xstats id type
> 
> 
> 
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Thursday, October 13, 2022 12:22 PM
> > To: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob
> > <jerinjacobk@gmail.com>; Sachin Saxena <sachin.saxena@oss.nxp.com>;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Ori Kam
> > <orika@nvidia.com>; Liron Himi <lironh@marvell.com>
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Jerin Jacob
> > Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org; Li, WeiyuanX
> > <weiyuanx.li@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> > david.marchand@redhat.com
> > Subject: [EXT] xstats id type
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > +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.

Please go ahead - I won't find time in the short term. Thanks Pavan.

> Thomas,
> Are you ok to break the ABI without deprication notice i.e. make ID as u64 for
> eventdev?
> 
> Thanks,
> Pavan.