event/sw: fix selftest xstats reset API usage

Message ID 20221012123419.182631-1-harry.van.haaren@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Jerin Jacob
Headers
Series event/sw: fix selftest xstats reset API usage |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Van Haaren, Harry Oct. 12, 2022, 12:34 p.m. UTC
  The eventdev xstats reset API takes an ID of "uint32_t", while
the rest of the xstats APIs require an "unsigned int". On some
platforms these might not be the same bitwidth, however this was
assumed in the code.

Fix by providing a uint32_t to the xstats_reset() function.
Fixes: e21df4b062b5 ("test/eventdev: add SW xstats tests")

Reported-by: Morten Brørup <mb@smartsharesystems.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

Note this is a fix for a potential build issue in 64-bit BE systems.

---
 drivers/event/sw/sw_evdev_selftest.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
  

Comments

Thomas Monjalon Oct. 12, 2022, 1:05 p.m. UTC | #1
12/10/2022 14:34, Harry van Haaren:
> The eventdev xstats reset API takes an ID of "uint32_t", while
> the rest of the xstats APIs require an "unsigned int". On some
> platforms these might not be the same bitwidth, however this was
> assumed in the code.
> 
> Fix by providing a uint32_t to the xstats_reset() function.

Why adding a comment about the type in the code?
We are not adding /* using the right type */ in each line of code.

[...]
> -		/* reset to zero */
> +		/* reset to zero: note API requires uint32_t not unsigned int */
> +		uint32_t reset_id = id;
>  		int reset_ret = rte_event_dev_xstats_reset(evdev,
  
Van Haaren, Harry Oct. 12, 2022, 1:08 p.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 12, 2022 2:06 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; Li, WeiyuanX <weiyuanx.li@intel.com>;
> ferruh.yigit@amd.com; andrew.rybchenko@oktetlabs.ru; Morten Brørup
> <mb@smartsharesystems.com>
> Subject: Re: [PATCH] event/sw: fix selftest xstats reset API usage
> 
> 12/10/2022 14:34, Harry van Haaren:
> > The eventdev xstats reset API takes an ID of "uint32_t", while
> > the rest of the xstats APIs require an "unsigned int". On some
> > platforms these might not be the same bitwidth, however this was
> > assumed in the code.
> >
> > Fix by providing a uint32_t to the xstats_reset() function.
> 
> Why adding a comment about the type in the code?
> We are not adding /* using the right type */ in each line of code.

Correct; but as the SW PMD was the only selftest code that actually
uses xstats_reset() I err-ed on the side of warning devs that there's
some "funky" or inconsistent API requirements here.

I can respin without if that seems better, but my intention was to
help future readers..?

<snip>
  
Thomas Monjalon Oct. 12, 2022, 3:09 p.m. UTC | #3
12/10/2022 15:08, Van Haaren, Harry:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2022 14:34, Harry van Haaren:
> > > The eventdev xstats reset API takes an ID of "uint32_t", while
> > > the rest of the xstats APIs require an "unsigned int". On some
> > > platforms these might not be the same bitwidth, however this was
> > > assumed in the code.
> > >
> > > Fix by providing a uint32_t to the xstats_reset() function.
> > 
> > Why adding a comment about the type in the code?
> > We are not adding /* using the right type */ in each line of code.
> 
> Correct; but as the SW PMD was the only selftest code that actually
> uses xstats_reset() I err-ed on the side of warning devs that there's
> some "funky" or inconsistent API requirements here.
> 
> I can respin without if that seems better, but my intention was to
> help future readers..?

It does not really help.
If you want to help, and you think the API is wrong,
then please send a patch to propose a change in the API.
  

Patch

diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index ed7ae6a685..f8496bc44e 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -1525,10 +1525,11 @@  xstats_id_reset_tests(struct test *t)
 				dev_names[i], dev_expected[i], val);
 			goto fail;
 		}
-		/* reset to zero */
+		/* reset to zero: note API requires uint32_t not unsigned int */
+		uint32_t reset_id = id;
 		int reset_ret = rte_event_dev_xstats_reset(evdev,
 						RTE_EVENT_DEV_XSTATS_DEVICE, 0,
-						&id,
+						&reset_id,
 						1);
 		if (reset_ret) {
 			printf("%d: failed to reset successfully\n", __LINE__);
@@ -1647,10 +1648,11 @@  xstats_id_reset_tests(struct test *t)
 				port_expected[i], id);
 			failed = 1;
 		}
-		/* reset to zero */
+		/* reset to zero: note API requires uint32_t not unsigned int */
+		uint32_t reset_id = id;
 		int reset_ret = rte_event_dev_xstats_reset(evdev,
 						RTE_EVENT_DEV_XSTATS_PORT, PORT,
-						&id,
+						&reset_id,
 						1);
 		if (reset_ret) {
 			printf("%d: failed to reset successfully\n", __LINE__);
@@ -1762,10 +1764,11 @@  xstats_id_reset_tests(struct test *t)
 				queue_names[i], queue_expected[i], val);
 			failed = 1;
 		}
-		/* reset to zero */
+		/* reset to zero: note API requires uint32_t not unsigned int */
+		uint32_t reset_id = id;
 		int reset_ret = rte_event_dev_xstats_reset(evdev,
 						RTE_EVENT_DEV_XSTATS_QUEUE,
-						queue, &id, 1);
+						queue, &reset_id, 1);
 		if (reset_ret) {
 			printf("%d: failed to reset successfully\n", __LINE__);
 			failed = 1;