* [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe
@ 2021-07-13 20:12 Stephen Hemminger
2021-07-19 17:15 ` Tyler Retzlaff
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Stephen Hemminger @ 2021-07-13 20:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, anatoly.burakov
The first argument to rte_bsf32_safe was incorrectly declared as
a 64 bit value. This function only correctly handles on 32 bit values
and the underlying function rte_bsf32 only accepts 32 bit values.
This was introduced when the safe version was added and probably cause
by copy/paste from the 64 bit version.
The bug passed silently under the radar until some other code was
built with -Wall and -Wextra in C++ and C++ complains about the
missing cast.
Yes, this is a API signature change, but the original code was wrong.
It is an inline so not an ABI change.
Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
Cc: anatoly.burakov@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/eal/include/rte_common.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5fe..99eb5f1820ae 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -623,7 +623,7 @@ rte_bsf32(uint32_t v)
* Returns 0 if ``v`` was 0, otherwise returns 1.
*/
static inline int
-rte_bsf32_safe(uint64_t v, uint32_t *pos)
+rte_bsf32_safe(uint32_t v, uint32_t *pos)
{
if (v == 0)
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe
2021-07-13 20:12 [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe Stephen Hemminger
@ 2021-07-19 17:15 ` Tyler Retzlaff
2021-07-19 22:00 ` Stephen Hemminger
2021-07-23 0:52 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
2021-07-23 15:45 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2 siblings, 1 reply; 8+ messages in thread
From: Tyler Retzlaff @ 2021-07-19 17:15 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, anatoly.burakov
On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> The first argument to rte_bsf32_safe was incorrectly declared as
> a 64 bit value. This function only correctly handles on 32 bit values
> and the underlying function rte_bsf32 only accepts 32 bit values.
> This was introduced when the safe version was added and probably cause
> by copy/paste from the 64 bit version.
there are multiple errors in this family of functions [1] both in usage
and signatures. we previously discussed rolling all fixes up into a single
patch and announcing an api break.
a doc patch was submitted as per the process documented for breaking api
but received no replies [2]
i have a full patch that corrects the whole family if you would like to
take it instead. contact me offline if you are interested.
1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
2. http://mails.dpdk.org/archives/dev/2021-March/201868.html
the change stand-alone is correct so
Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
>
> The bug passed silently under the radar until some other code was
> built with -Wall and -Wextra in C++ and C++ complains about the
> missing cast.
>
> Yes, this is a API signature change, but the original code was wrong.
> It is an inline so not an ABI change.
>
> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
> Cc: anatoly.burakov@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/eal/include/rte_common.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index d5a32c66a5fe..99eb5f1820ae 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -623,7 +623,7 @@ rte_bsf32(uint32_t v)
> * Returns 0 if ``v`` was 0, otherwise returns 1.
> */
> static inline int
> -rte_bsf32_safe(uint64_t v, uint32_t *pos)
> +rte_bsf32_safe(uint32_t v, uint32_t *pos)
> {
> if (v == 0)
> return 0;
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe
2021-07-19 17:15 ` Tyler Retzlaff
@ 2021-07-19 22:00 ` Stephen Hemminger
2021-07-20 13:26 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2021-07-19 22:00 UTC (permalink / raw)
To: Tyler Retzlaff; +Cc: dev, anatoly.burakov
On Mon, 19 Jul 2021 10:15:34 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> > The first argument to rte_bsf32_safe was incorrectly declared as
> > a 64 bit value. This function only correctly handles on 32 bit values
> > and the underlying function rte_bsf32 only accepts 32 bit values.
> > This was introduced when the safe version was added and probably cause
> > by copy/paste from the 64 bit version.
>
> there are multiple errors in this family of functions [1] both in usage
> and signatures. we previously discussed rolling all fixes up into a single
> patch and announcing an api break.
>
> a doc patch was submitted as per the process documented for breaking api
> but received no replies [2]
>
> i have a full patch that corrects the whole family if you would like to
> take it instead. contact me offline if you are interested.
>
> 1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 2. http://mails.dpdk.org/archives/dev/2021-March/201868.html
>
> the change stand-alone is correct so
>
> Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
Thanks, I think the larger set should go into 21.11 where API/ABI break
would be ok. My bit was all about fixing the bug where current code
breaks C++ users.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe
2021-07-19 22:00 ` Stephen Hemminger
@ 2021-07-20 13:26 ` Thomas Monjalon
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2021-07-20 13:26 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Tyler Retzlaff, dev, anatoly.burakov, david.marchand
20/07/2021 00:00, Stephen Hemminger:
> On Mon, 19 Jul 2021 10:15:34 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
>
> > On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> > > The first argument to rte_bsf32_safe was incorrectly declared as
> > > a 64 bit value. This function only correctly handles on 32 bit values
> > > and the underlying function rte_bsf32 only accepts 32 bit values.
> > > This was introduced when the safe version was added and probably cause
> > > by copy/paste from the 64 bit version.
> >
> > there are multiple errors in this family of functions [1] both in usage
> > and signatures. we previously discussed rolling all fixes up into a single
> > patch and announcing an api break.
> >
> > a doc patch was submitted as per the process documented for breaking api
> > but received no replies [2]
> >
> > i have a full patch that corrects the whole family if you would like to
> > take it instead. contact me offline if you are interested.
> >
> > 1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > 2. http://mails.dpdk.org/archives/dev/2021-March/201868.html
> >
> > the change stand-alone is correct so
> >
> > Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
>
> Thanks, I think the larger set should go into 21.11 where API/ABI break
> would be ok. My bit was all about fixing the bug where current code
> breaks C++ users.
Shouldn't we have a note in the API changes section of the release notes?
^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH v2] eal: fix argument to rte_bsf32_safe
2021-07-13 20:12 [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe Stephen Hemminger
2021-07-19 17:15 ` Tyler Retzlaff
@ 2021-07-23 0:52 ` Stephen Hemminger
2021-07-23 15:45 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2021-07-23 0:52 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, anatoly.burakov, Tyler Retzlaff
The first argument to rte_bsf32_safe was incorrectly declared as
a 64 bit value. The code only works on 32 bit values and the underlying
function rte_bsf32 only accepts 32 bit values. This was a mistake
introduced when the safe version was added and probaly cause
by copy/paste from the 64 bit version.
The bug passed silently under the radar until some other code was
built with -Wall and -Wextra in C++ and C++ complains about the
missing cast.
Yes, this is a API signature change, but the original code was wrong.
It is an inline so not an ABI change.
Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
Cc: anatoly.burakov@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
v2 - add suggested release note
doc/guides/rel_notes/release_21_08.rst | 4 ++++
lib/eal/include/rte_common.h | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/doc/guides/rel_notes/release_21_08.rst b/doc/guides/rel_notes/release_21_08.rst
index e2c5ccbf7d90..148405891fcb 100644
--- a/doc/guides/rel_notes/release_21_08.rst
+++ b/doc/guides/rel_notes/release_21_08.rst
@@ -196,6 +196,10 @@ API Changes
to be thread safe; all Rx queues affected by the API will now need to be
stopped before making any changes to the power management scheme.
+* eal: ``rte_bsf32_safe`` now takes a 32 bit value for its first
+ argument. This fixes warnings about loss of precision when used
+ with some compilers settings.
+
ABI Changes
-----------
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5fe..99eb5f1820ae 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -623,7 +623,7 @@ rte_bsf32(uint32_t v)
* Returns 0 if ``v`` was 0, otherwise returns 1.
*/
static inline int
-rte_bsf32_safe(uint64_t v, uint32_t *pos)
+rte_bsf32_safe(uint32_t v, uint32_t *pos)
{
if (v == 0)
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH v3] eal: fix argument to rte_bsf32_safe
2021-07-13 20:12 [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe Stephen Hemminger
2021-07-19 17:15 ` Tyler Retzlaff
2021-07-23 0:52 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
@ 2021-07-23 15:45 ` Stephen Hemminger
2021-07-24 7:58 ` Thomas Monjalon
2 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2021-07-23 15:45 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, anatoly.burakov, Tyler Retzlaff
The first argument to rte_bsf32_safe was incorrectly declared as
a 64 bit value. The code only works on 32 bit values and the underlying
function rte_bsf32 only accepts 32 bit values. This was a mistake
introduced when the safe version was added and probably cause
by copy/paste from the 64 bit version.
The bug passed silently under the radar until some other code was
built with -Wall and -Wextra in C++ and C++ complains about the
missing cast.
Yes, this is a API signature change, but the original code was wrong.
It is an inline so not an ABI change.
Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
Cc: anatoly.burakov@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
v3 - reword commit description for checkpatch
doc/guides/rel_notes/release_21_08.rst | 4 ++++
lib/eal/include/rte_common.h | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/doc/guides/rel_notes/release_21_08.rst b/doc/guides/rel_notes/release_21_08.rst
index e2c5ccbf7d90..148405891fcb 100644
--- a/doc/guides/rel_notes/release_21_08.rst
+++ b/doc/guides/rel_notes/release_21_08.rst
@@ -196,6 +196,10 @@ API Changes
to be thread safe; all Rx queues affected by the API will now need to be
stopped before making any changes to the power management scheme.
+* eal: ``rte_bsf32_safe`` now takes a 32 bit value for its first
+ argument. This fixes warnings about loss of precision when used
+ with some compilers settings.
+
ABI Changes
-----------
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5fe..99eb5f1820ae 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -623,7 +623,7 @@ rte_bsf32(uint32_t v)
* Returns 0 if ``v`` was 0, otherwise returns 1.
*/
static inline int
-rte_bsf32_safe(uint64_t v, uint32_t *pos)
+rte_bsf32_safe(uint32_t v, uint32_t *pos)
{
if (v == 0)
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH v3] eal: fix argument to rte_bsf32_safe
2021-07-23 15:45 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
@ 2021-07-24 7:58 ` Thomas Monjalon
2021-07-24 23:50 ` Stephen Hemminger
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-07-24 7:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, anatoly.burakov, Tyler Retzlaff
23/07/2021 17:45, Stephen Hemminger:
> The first argument to rte_bsf32_safe was incorrectly declared as
> a 64 bit value. The code only works on 32 bit values and the underlying
> function rte_bsf32 only accepts 32 bit values. This was a mistake
> introduced when the safe version was added and probably cause
> by copy/paste from the 64 bit version.
>
> The bug passed silently under the radar until some other code was
> built with -Wall and -Wextra in C++ and C++ complains about the
> missing cast.
>
> Yes, this is a API signature change, but the original code was wrong.
> It is an inline so not an ABI change.
>
> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
> Cc: anatoly.burakov@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
+Cc: stable@dpdk.org
Applied, thanks.
I think these functions lack a reference to the name Bit Scan Forward.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH v3] eal: fix argument to rte_bsf32_safe
2021-07-24 7:58 ` Thomas Monjalon
@ 2021-07-24 23:50 ` Stephen Hemminger
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2021-07-24 23:50 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, anatoly.burakov, Tyler Retzlaff
On Sat, 24 Jul 2021 09:58:44 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:
> 23/07/2021 17:45, Stephen Hemminger:
> > The first argument to rte_bsf32_safe was incorrectly declared as
> > a 64 bit value. The code only works on 32 bit values and the underlying
> > function rte_bsf32 only accepts 32 bit values. This was a mistake
> > introduced when the safe version was added and probably cause
> > by copy/paste from the 64 bit version.
> >
> > The bug passed silently under the radar until some other code was
> > built with -Wall and -Wextra in C++ and C++ complains about the
> > missing cast.
> >
> > Yes, this is a API signature change, but the original code was wrong.
> > It is an inline so not an ABI change.
> >
> > Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
> > Cc: anatoly.burakov@intel.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>
> +Cc: stable@dpdk.org
>
> Applied, thanks.
>
> I think these functions lack a reference to the name Bit Scan Forward.
>
>
>
>
Tyler wanted to fix a bunch more stuff in these for 21.11 where it will
be a bigger API change.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-24 23:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 20:12 [dpdk-dev] [PATCH] eal: fix argument to rte_bsf32_safe Stephen Hemminger
2021-07-19 17:15 ` Tyler Retzlaff
2021-07-19 22:00 ` Stephen Hemminger
2021-07-20 13:26 ` Thomas Monjalon
2021-07-23 0:52 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
2021-07-23 15:45 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2021-07-24 7:58 ` Thomas Monjalon
2021-07-24 23:50 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).