rte_event_dev_xstats_reset id type

rte_event_dev_xstats_reset id type

[Morten Brørup]

Hi Jerin (eventdev maintainer),

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.

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.

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#n1766


Med venlig hilsen / Kind regards,
-Morten Brørup

Re: rte_event_dev_xstats_reset id type

[Jerin Jacob]

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.

>
> 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.
>
> 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.
>
> 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#n1766
>
>
> Med venlig hilsen / Kind regards,
> -Morten Brørup
>

RE: rte_event_dev_xstats_reset id type

[Van Haaren, Harry]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, October 12, 2022 10:45 AM
> To: Morten Brørup <mb@smartsharesystems.com>; Van Haaren, Harry
> <harry.van.haaren@intel.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
> 
> 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.


> > 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.
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.


> > 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#n1766
> >
> >
> > Med venlig hilsen / Kind regards,
> > -Morten Brørup

RE: rte_event_dev_xstats_reset id type

[Morten Brørup]

> 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.

> 
> 
> > > 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.

Unfortunately, these functions are not marked experimental, so breaking API/ABI is hard to do. :-(

> 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.

> 
> 
> > > 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

RE: rte_event_dev_xstats_reset id type

[Van Haaren, Harry]

> -----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

Re: rte_event_dev_xstats_reset id type

[Thomas Monjalon]

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 :)

RE: rte_event_dev_xstats_reset id type

[Morten Brørup]

> 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?

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.

Re: rte_event_dev_xstats_reset id type

[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.
>

Re: rte_event_dev_xstats_reset id type

[Thomas Monjalon]

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,
so I would like to keep uint64_t please.

Re: rte_event_dev_xstats_reset id type

[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.


> so I would like to keep uint64_t please.
>
>

Re: rte_event_dev_xstats_reset id type

[Thomas Monjalon]

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:
http://fast.dpdk.org/events/slides/DPDK-2019-09-Ethernet_Statistics.pdf

xstats id type

[Morten Brørup]

+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:
> 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.

RE: xstats id type

[Pavan Nikhilesh Bhagavatula]

> -----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.

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

Thanks,
Pavan.

Re: xstats id type

[Thomas Monjalon]

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.

RE: [EXT] Re: xstats id type

[Pavan Nikhilesh Bhagavatula]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 13, 2022 1:57 PM
> To: Morten Brørup <mb@smartsharesystems.com>; 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>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@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; techboard@dpdk.org
> Subject: [EXT] Re: xstats id type
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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?

Yes

> The only problematic part is that the application must pass
> a pointer of the right size, meaning some application code change.

Most likely they'll get a -Wincompatible-pointer-types not the end of the world.

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

RE: xstats id type

[Van Haaren, Harry]

> -----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.