[dpdk-dev] [PATCH] doc: propose correction rte_bsf64 return type declaration

[dpdk-dev] [PATCH] doc: propose correction rte_bsf64 return type declaration

[Tyler Retzlaff]

It is clear from the change that introduced the rte_bsf64 inline
function and an evaluation of usage that the initial revision was
intended to return uint32_t and not int.

It is proposed the return type of int be changed to uint32_t.

-static inline int
+static inline uint32_t
 rte_bsf64(uint64_t v)
 {
        return (uint32_t)__builtin_ctzll(v);
 }

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 64629e064..7ba86024a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -17,6 +17,9 @@ Deprecation Notices
 * eal: The function ``rte_eal_remote_launch`` will return new error codes
   after read or write error on the pipe, instead of calling ``rte_panic``.
 
+* eal: The inline function ``rte_bsf64`` will be changed to return ``uint32_t``
+  instead of ``int`` as originally intended.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers
-- 
2.30.0.vfs.0.2

[dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Tyler Retzlaff]

The proposal has resulted from request to review [1] the following
functions where there appeared to be inconsistency in return type
or parameter type selections for the following inline functions.

rte_bsf32()
rte_bsf32_safe()
rte_bsf64()
rte_bsf64_safe()
rte_fls_u32()
rte_fls_u64()
rte_log2_u32()
rte_log2_u64()

[1] http://mails.dpdk.org/archives/dev/2021-March/201590.html

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 64629e064..4934f4da4 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -17,6 +17,13 @@ Deprecation Notices
 * eal: The function ``rte_eal_remote_launch`` will return new error codes
   after read or write error on the pipe, instead of calling ``rte_panic``.
 
+* eal: Fix inline function return and parameter types for rte_{bsf,fls}
+  inline functions to be consistent.
+  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
+  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers
-- 
2.30.0.vfs.0.2

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Thomas Monjalon]

15/03/2021 20:34, Tyler Retzlaff:
> The proposal has resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
> 
> rte_bsf32()
> rte_bsf32_safe()
> rte_bsf64()
> rte_bsf64_safe()
> rte_fls_u32()
> rte_fls_u64()
> rte_log2_u32()
> rte_log2_u64()
> 
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* eal: Fix inline function return and parameter types for rte_{bsf,fls}
> +  inline functions to be consistent.
> +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
> +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.

It seems we completely forgot this.
How critical is it?

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, 25 October 2021 21.14
> 
> 15/03/2021 20:34, Tyler Retzlaff:
> > The proposal has resulted from request to review [1] the following
> > functions where there appeared to be inconsistency in return type
> > or parameter type selections for the following inline functions.
> >
> > rte_bsf32()
> > rte_bsf32_safe()
> > rte_bsf64()
> > rte_bsf64_safe()
> > rte_fls_u32()
> > rte_fls_u64()
> > rte_log2_u32()
> > rte_log2_u64()
> >
> > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > +* eal: Fix inline function return and parameter types for
> rte_{bsf,fls}
> > +  inline functions to be consistent.
> > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> ``uint32_t``.
> > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> ``int``.
> > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> ``int``.
> > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> ``int``.
> 
> It seems we completely forgot this.
> How critical is it?

Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.

Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.

The update's only tangible benefit is API consistency. :-)

-Morten

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Tyler Retzlaff]

On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, 25 October 2021 21.14
> > 
> > 15/03/2021 20:34, Tyler Retzlaff:
> > > The proposal has resulted from request to review [1] the following
> > > functions where there appeared to be inconsistency in return type
> > > or parameter type selections for the following inline functions.
> > >
> > > rte_bsf32()
> > > rte_bsf32_safe()
> > > rte_bsf64()
> > > rte_bsf64_safe()
> > > rte_fls_u32()
> > > rte_fls_u64()
> > > rte_log2_u32()
> > > rte_log2_u64()
> > >
> > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > +* eal: Fix inline function return and parameter types for
> > rte_{bsf,fls}
> > > +  inline functions to be consistent.
> > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > ``uint32_t``.
> > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > ``int``.
> > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > ``int``.
> > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > ``int``.
> > 
> > It seems we completely forgot this.
> > How critical is it?
> 

our organization as a matter of internal security policy requires these
sorts of things to be fixed. while i didn't see any bugs in the dpdk
code there is an opportunity for users of these functions to
accidentally write code that is prone to integer and buffer overflow
class bugs.

there is no urgency, but why leave things sloppy? though i do wish this
had been responded to in a more timely manner 7 months for something
that should have almost been rubber stamped.

> Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.
> 
> Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.
> 
> The update's only tangible benefit is API consistency. :-)
> 
> -Morten

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Thomas Monjalon]

11/11/2021 05:15, Tyler Retzlaff:
> On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > Sent: Monday, 25 October 2021 21.14
> > > 
> > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > The proposal has resulted from request to review [1] the following
> > > > functions where there appeared to be inconsistency in return type
> > > > or parameter type selections for the following inline functions.
> > > >
> > > > rte_bsf32()
> > > > rte_bsf32_safe()
> > > > rte_bsf64()
> > > > rte_bsf64_safe()
> > > > rte_fls_u32()
> > > > rte_fls_u64()
> > > > rte_log2_u32()
> > > > rte_log2_u64()
> > > >
> > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > +* eal: Fix inline function return and parameter types for
> > > rte_{bsf,fls}
> > > > +  inline functions to be consistent.
> > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > > ``uint32_t``.
> > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > ``int``.
> > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > 
> > > It seems we completely forgot this.
> > > How critical is it?
> > 
> 
> our organization as a matter of internal security policy requires these
> sorts of things to be fixed. while i didn't see any bugs in the dpdk
> code there is an opportunity for users of these functions to
> accidentally write code that is prone to integer and buffer overflow
> class bugs.
> 
> there is no urgency, but why leave things sloppy? though i do wish this
> had been responded to in a more timely manner 7 months for something
> that should have almost been rubber stamped.

It's difficult to be on all topics.
The best way to avoid such miss is to ping when you see no progress.

So what's next?
They are only inline functions, right? so no ABI breakage.
Is it going to require any change on application-side? I guess no.
Is it acceptable in 21.11-rc3? maybe too late?
Is it acceptable in 22.02?

RE: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 11 November 2021 12.55
> 
> 11/11/2021 05:15, Tyler Retzlaff:
> > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> Monjalon
> > > > Sent: Monday, 25 October 2021 21.14
> > > >
> > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > The proposal has resulted from request to review [1] the
> following
> > > > > functions where there appeared to be inconsistency in return
> type
> > > > > or parameter type selections for the following inline
> functions.
> > > > >
> > > > > rte_bsf32()
> > > > > rte_bsf32_safe()
> > > > > rte_bsf64()
> > > > > rte_bsf64_safe()
> > > > > rte_fls_u32()
> > > > > rte_fls_u64()
> > > > > rte_log2_u32()
> > > > > rte_log2_u64()
> > > > >
> > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > >
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > ---
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > +* eal: Fix inline function return and parameter types for
> > > > rte_{bsf,fls}
> > > > > +  inline functions to be consistent.
> > > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> to
> > > > ``uint32_t``.
> > > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > > ``int``.
> > > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > >
> > > > It seems we completely forgot this.
> > > > How critical is it?
> > >
> >
> > our organization as a matter of internal security policy requires
> these
> > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > code there is an opportunity for users of these functions to
> > accidentally write code that is prone to integer and buffer overflow
> > class bugs.
> >
> > there is no urgency, but why leave things sloppy? though i do wish
> this
> > had been responded to in a more timely manner 7 months for something
> > that should have almost been rubber stamped.
> 
> It's difficult to be on all topics.
> The best way to avoid such miss is to ping when you see no progress.
> 
> So what's next?
> They are only inline functions, right? so no ABI breakage.
> Is it going to require any change on application-side? I guess no.
> Is it acceptable in 21.11-rc3? maybe too late?
> Is it acceptable in 22.02?

If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.

-Morten

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Jerin Jacob]

On Thu, Nov 11, 2021 at 6:11 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, 11 November 2021 12.55
> >
> > 11/11/2021 05:15, Tyler Retzlaff:
> > > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > Monjalon
> > > > > Sent: Monday, 25 October 2021 21.14
> > > > >
> > > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > > The proposal has resulted from request to review [1] the
> > following
> > > > > > functions where there appeared to be inconsistency in return
> > type
> > > > > > or parameter type selections for the following inline
> > functions.
> > > > > >
> > > > > > rte_bsf32()
> > > > > > rte_bsf32_safe()
> > > > > > rte_bsf64()
> > > > > > rte_bsf64_safe()
> > > > > > rte_fls_u32()
> > > > > > rte_fls_u64()
> > > > > > rte_log2_u32()
> > > > > > rte_log2_u64()
> > > > > >
> > > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > > >
> > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>


Acked-by: Jerin Jacob <jerinj@marvell.com>



> > > > > > ---
> > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > +* eal: Fix inline function return and parameter types for
> > > > > rte_{bsf,fls}
> > > > > > +  inline functions to be consistent.
> > > > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> > to
> > > > > ``uint32_t``.
> > > > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > > > ``int``.
> > > > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > >
> > > > > It seems we completely forgot this.
> > > > > How critical is it?
> > > >
> > >
> > > our organization as a matter of internal security policy requires
> > these
> > > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > > code there is an opportunity for users of these functions to
> > > accidentally write code that is prone to integer and buffer overflow
> > > class bugs.
> > >
> > > there is no urgency, but why leave things sloppy? though i do wish
> > this
> > > had been responded to in a more timely manner 7 months for something
> > > that should have almost been rubber stamped.
> >
> > It's difficult to be on all topics.
> > The best way to avoid such miss is to ping when you see no progress.
> >
> > So what's next?
> > They are only inline functions, right? so no ABI breakage.
> > Is it going to require any change on application-side? I guess no.
> > Is it acceptable in 21.11-rc3? maybe too late?
> > Is it acceptable in 22.02?
>
> If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.
>
> -Morten
>

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Thomas Monjalon]

Tyler, there was no progress on this topic during the past year.
Please could you send the patch to fix API inconsistencies,
so we will merge it in 22.11?

Re: [dpdk-dev] [PATCH v2] doc: propose correction rte_{bsf, fls} inline functions type use

[Tyler Retzlaff]

Hi Thomas,

I will see if i can carve out some time budget for it, it's a pretty old
patch so it could take some time to review.

On Wed, Jul 13, 2022 at 12:13:20PM +0200, Thomas Monjalon wrote:
> Tyler, there was no progress on this topic during the past year.
> Please could you send the patch to fix API inconsistencies,
> so we will merge it in 22.11?
> 

[PATCH 0/3] cleanup bsf and fls inline function return types

[Tyler Retzlaff]

The cleanup resulted from request to review [1] the following
functions where there appeared to be inconsistency in return type
or parameter type selections for the following inline functions.

Since the original request some instances have been fixed so this
series completes the outstanding return value type updates.

[1] http://mails.dpdk.org/archives/dev/2021-March/201590.html

Tyler Retzlaff (3):
  doc: announce cleanup of rte_{bsf, fls} inline functions type use
  eal: change rte_fls and rte_bsf to return uint32_t
  test: fix sign compare warning for rte_bsf64 return type change

 app/test/test_mbuf.c                 | 2 +-
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 lib/eal/include/rte_common.h         | 6 +++---
 3 files changed, 10 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] doc: announce cleanup of rte_{bsf, fls} inline functions type use

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@microsoft.com>

The cleanup resulted from request to review [1] the following
functions where there appeared to be inconsistency in return type
or parameter type selections for the following inline functions.

rte_bsf32()
rte_bsf32_safe()
rte_bsf64()
rte_bsf64_safe()
rte_fls_u32()
rte_fls_u64()
rte_log2_u32()
rte_log2_u64()

[1] http://mails.dpdk.org/archives/dev/2021-March/201590.html

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e7583ca..58f4c24 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -17,6 +17,12 @@ Deprecation Notices
 * eal: The function ``rte_eal_remote_launch`` will return new error codes
   after read or write error on the pipe, instead of calling ``rte_panic``.
 
+* eal: Fix inline function return and parameter types for rte_{bsf,fls}
+  inline functions to be consistent in DPDK 22.11.
+  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK has adopted the atomic operations from
-- 
1.8.3.1

Re: [PATCH 1/3] doc: announce cleanup of rte_{bsf, fls} inline functions type use

[Thomas Monjalon]

08/08/2022 23:21, Tyler Retzlaff:
> From: Tyler Retzlaff <roretzla@microsoft.com>
> 
> The cleanup resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
> 
> rte_bsf32()
> rte_bsf32_safe()
> rte_bsf64()
> rte_bsf64_safe()

Why _safe functions are not changed?

> rte_fls_u32()
> rte_fls_u64()
> rte_log2_u32()
> rte_log2_u64()
> 
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index e7583ca..58f4c24 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* eal: Fix inline function return and parameter types for rte_{bsf,fls}
> +  inline functions to be consistent in DPDK 22.11.
> +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.

It is too late for the announce,
but the release notes requires to be updated.

[PATCH 2/3] eal: change rte_fls and rte_bsf to return uint32_t

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@microsoft.com>

return fixed width uint32_t to be consistent with what appears to
be the original authors intent. it doesn't make much sense to return
signed integers for these functions.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index a96cc2a..bd4184d 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -707,7 +707,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *     The last (most-significant) bit set, or 0 if the input is 0.
  */
-static inline int
+static inline uint32_t
 rte_fls_u32(uint32_t x)
 {
 	return (x == 0) ? 0 : 32 - __builtin_clz(x);
@@ -724,7 +724,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *     least significant set bit in the input parameter.
  */
-static inline int
+static inline uint32_t
 rte_bsf64(uint64_t v)
 {
 	return (uint32_t)__builtin_ctzll(v);
@@ -766,7 +766,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *     The last (most-significant) bit set, or 0 if the input is 0.
  */
-static inline int
+static inline uint32_t
 rte_fls_u64(uint64_t x)
 {
 	return (x == 0) ? 0 : 64 - __builtin_clzll(x);
-- 
1.8.3.1

Re: [PATCH 2/3] eal: change rte_fls and rte_bsf to return uint32_t

[Thomas Monjalon]

08/08/2022 23:21, Tyler Retzlaff:
> From: Tyler Retzlaff <roretzla@microsoft.com>
> 
> return fixed width uint32_t to be consistent with what appears to
> be the original authors intent. it doesn't make much sense to return
> signed integers for these functions.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index a96cc2a..bd4184d 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -707,7 +707,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @return
>   *     The last (most-significant) bit set, or 0 if the input is 0.
>   */
> -static inline int
> +static inline uint32_t
>  rte_fls_u32(uint32_t x)
>  {
>  	return (x == 0) ? 0 : 32 - __builtin_clz(x);
> @@ -724,7 +724,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @return
>   *     least significant set bit in the input parameter.
>   */
> -static inline int
> +static inline uint32_t
>  rte_bsf64(uint64_t v)
>  {
>  	return (uint32_t)__builtin_ctzll(v);
> @@ -766,7 +766,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @return
>   *     The last (most-significant) bit set, or 0 if the input is 0.
>   */
> -static inline int
> +static inline uint32_t
>  rte_fls_u64(uint64_t x)
>  {
>  	return (x == 0) ? 0 : 64 - __builtin_clzll(x);
> 

You forgot the _safe versions:

--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline int
+static inline uint32_t
 rte_bsf32_safe(uint32_t v, uint32_t *pos)
 {
        if (v == 0)
@@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline int
+static inline uint32_t
 rte_bsf64_safe(uint64_t v, uint32_t *pos)
 {
        if (v == 0)

Re: [PATCH 2/3] eal: change rte_fls and rte_bsf to return uint32_t

[Tyler Retzlaff]

On Wed, Oct 05, 2022 at 11:02:45AM +0200, Thomas Monjalon wrote:
> 08/08/2022 23:21, Tyler Retzlaff:
> > From: Tyler Retzlaff <roretzla@microsoft.com>
> > 
> 
> You forgot the _safe versions:

> 
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
>   * @return
>   *     Returns 0 if ``v`` was 0, otherwise returns 1.
>   */
> -static inline int
> +static inline uint32_t
>  rte_bsf32_safe(uint32_t v, uint32_t *pos)
>  {
>         if (v == 0)
> @@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
>   * @return
>   *     Returns 0 if ``v`` was 0, otherwise returns 1.
>   */
> -static inline int
> +static inline uint32_t
>  rte_bsf64_safe(uint64_t v, uint32_t *pos)
>  {
>         if (v == 0)
> 
> 
> 
> 

the return values from the _safe versions are `int' treated
like a bool type. they have been left as is to be consistent
with the rest of dpdk return value types.

the non-safe version returns were returning actual values
and not an indication of success or failure.

they could certainly be changed to C99 fixed width types but
if they are changed at all perhaps they should be changed to
_Bool or bool from stdbool.h?

it looks like this change has been merged already but if you
would like to make any further changes let me know i'll take
care of it.

thanks

Re: [PATCH 2/3] eal: change rte_fls and rte_bsf to return uint32_t

[Thomas Monjalon]

05/10/2022 17:15, Tyler Retzlaff:
> On Wed, Oct 05, 2022 at 11:02:45AM +0200, Thomas Monjalon wrote:
> > 08/08/2022 23:21, Tyler Retzlaff:
> > > From: Tyler Retzlaff <roretzla@microsoft.com>
> > > 
> > 
> > You forgot the _safe versions:
> 
> > 
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
> >   * @return
> >   *     Returns 0 if ``v`` was 0, otherwise returns 1.
> >   */
> > -static inline int
> > +static inline uint32_t
> >  rte_bsf32_safe(uint32_t v, uint32_t *pos)
> >  {
> >         if (v == 0)
> > @@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
> >   * @return
> >   *     Returns 0 if ``v`` was 0, otherwise returns 1.
> >   */
> > -static inline int
> > +static inline uint32_t
> >  rte_bsf64_safe(uint64_t v, uint32_t *pos)
> >  {
> >         if (v == 0)
> > 
> > 
> > 
> > 
> 
> the return values from the _safe versions are `int' treated
> like a bool type. they have been left as is to be consistent
> with the rest of dpdk return value types.
> 
> the non-safe version returns were returning actual values
> and not an indication of success or failure.
> 
> they could certainly be changed to C99 fixed width types but
> if they are changed at all perhaps they should be changed to
> _Bool or bool from stdbool.h?
> 
> it looks like this change has been merged already but if you
> would like to make any further changes let me know i'll take
> care of it.

Sorry, it's my mistake, I went too fast.
I'll revert them to int.

[PATCH] eal: fix return type of bsf safe functions

[Thomas Monjalon]

In a recent commit, changing return type from int to uint32_t,
I did a last minute change to functions rte_bsf32_safe and rte_bsf64_safe,
because thought they were forgotten.
Actually these functions are returning 0 or 1, so it should be int.
The return type is reverted to the original type for these 2 functions.

Fixes: 4b81c145ae37 ("eal: change return type of bsf/fls functions")

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/eal/include/rte_common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 23d9e05cbb..15765b408d 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline uint32_t
+static inline int
 rte_bsf32_safe(uint32_t v, uint32_t *pos)
 {
 	if (v == 0)
@@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline uint32_t
+static inline int
 rte_bsf64_safe(uint64_t v, uint32_t *pos)
 {
 	if (v == 0)
-- 
2.36.1

Re: [PATCH] eal: fix return type of bsf safe functions

[David Marchand]

On Wed, Oct 5, 2022 at 5:41 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> In a recent commit, changing return type from int to uint32_t,
> I did a last minute change to functions rte_bsf32_safe and rte_bsf64_safe,
> because thought they were forgotten.
> Actually these functions are returning 0 or 1, so it should be int.
> The return type is reverted to the original type for these 2 functions.
>
> Fixes: 4b81c145ae37 ("eal: change return type of bsf/fls functions")
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH] eal: fix return type of bsf safe functions

[Tyler Retzlaff]

On Wed, Oct 05, 2022 at 09:41:26PM +0200, David Marchand wrote:
> On Wed, Oct 5, 2022 at 5:41 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > In a recent commit, changing return type from int to uint32_t,
> > I did a last minute change to functions rte_bsf32_safe and rte_bsf64_safe,
> > because thought they were forgotten.
> > Actually these functions are returning 0 or 1, so it should be int.
> > The return type is reverted to the original type for these 2 functions.
> >
> > Fixes: 4b81c145ae37 ("eal: change return type of bsf/fls functions")
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH] eal: fix return type of bsf safe functions

[Thomas Monjalon]

06/10/2022 00:20, Tyler Retzlaff:
> On Wed, Oct 05, 2022 at 09:41:26PM +0200, David Marchand wrote:
> > On Wed, Oct 5, 2022 at 5:41 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > In a recent commit, changing return type from int to uint32_t,
> > > I did a last minute change to functions rte_bsf32_safe and rte_bsf64_safe,
> > > because thought they were forgotten.
> > > Actually these functions are returning 0 or 1, so it should be int.
> > > The return type is reverted to the original type for these 2 functions.
> > >
> > > Fixes: 4b81c145ae37 ("eal: change return type of bsf/fls functions")
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > 
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Applied, sorry for the noise.

Re: [PATCH] eal: fix return type of bsf safe functions

[Mattias Rönnblom]

On 2022-10-05 17:40, Thomas Monjalon wrote:
> In a recent commit, changing return type from int to uint32_t,
> I did a last minute change to functions rte_bsf32_safe and rte_bsf64_safe,
> because thought they were forgotten.
> Actually these functions are returning 0 or 1, so it should be int.

Wouldn't bool be a better choice?

> The return type is reverted to the original type for these 2 functions.
> 
> Fixes: 4b81c145ae37 ("eal: change return type of bsf/fls functions")
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   lib/eal/include/rte_common.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 23d9e05cbb..15765b408d 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
>    * @return
>    *     Returns 0 if ``v`` was 0, otherwise returns 1.
>    */
> -static inline uint32_t
> +static inline int
>   rte_bsf32_safe(uint32_t v, uint32_t *pos)
>   {
>   	if (v == 0)
> @@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
>    * @return
>    *     Returns 0 if ``v`` was 0, otherwise returns 1.
>    */
> -static inline uint32_t
> +static inline int
>   rte_bsf64_safe(uint64_t v, uint32_t *pos)
>   {
>   	if (v == 0)

[PATCH 3/3] test: fix sign compare warning for rte_bsf64 return type change

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@microsoft.com>

rte_bsf64 return type has been changed insert cast to suppress
sign-comapre warning with int.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_mbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index e09b254..be4c3ff 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -2677,7 +2677,7 @@ struct test_case {
 
 	flag3 = rte_mbuf_dynflag_register_bitnum(&dynflag3,
 						rte_bsf64(RTE_MBUF_F_LAST_FREE));
-	if (flag3 != rte_bsf64(RTE_MBUF_F_LAST_FREE))
+	if ((uint32_t)flag3 != rte_bsf64(RTE_MBUF_F_LAST_FREE))
 		GOTO_FAIL("failed to register dynamic flag 3, flag3=%d: %s",
 			flag3, strerror(errno));
 
-- 
1.8.3.1

Re: [PATCH 0/3] cleanup bsf and fls inline function return types

[Stephen Hemminger]

On Mon,  8 Aug 2022 14:21:29 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> The cleanup resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
> 
> Since the original request some instances have been fixed so this
> series completes the outstanding return value type updates.
> 
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 
> Tyler Retzlaff (3):
>   doc: announce cleanup of rte_{bsf, fls} inline functions type use
>   eal: change rte_fls and rte_bsf to return uint32_t
>   test: fix sign compare warning for rte_bsf64 return type change
> 
>  app/test/test_mbuf.c                 | 2 +-
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  lib/eal/include/rte_common.h         | 6 +++---
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 


Series-Acked-by: Stephen Hemminger <stephen@networkplumber.org>

RE: [PATCH 0/3] cleanup bsf and fls inline function return types

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 8 August 2022 23.21
> 
> The cleanup resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
> 
> Since the original request some instances have been fixed so this
> series completes the outstanding return value type updates.
> 
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 
> Tyler Retzlaff (3):
>   doc: announce cleanup of rte_{bsf, fls} inline functions type use
>   eal: change rte_fls and rte_bsf to return uint32_t
>   test: fix sign compare warning for rte_bsf64 return type change
> 
>  app/test/test_mbuf.c                 | 2 +-
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  lib/eal/include/rte_common.h         | 6 +++---
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> --
> 1.8.3.1
> 

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 0/3] cleanup bsf and fls inline function return types

[Thomas Monjalon]

09/08/2022 10:26, Morten Brørup:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Monday, 8 August 2022 23.21
> > 
> > The cleanup resulted from request to review [1] the following
> > functions where there appeared to be inconsistency in return type
> > or parameter type selections for the following inline functions.
> > 
> > Since the original request some instances have been fixed so this
> > series completes the outstanding return value type updates.
> > 
> > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > 
> > Tyler Retzlaff (3):
> >   doc: announce cleanup of rte_{bsf, fls} inline functions type use
> >   eal: change rte_fls and rte_bsf to return uint32_t
> >   test: fix sign compare warning for rte_bsf64 return type change
> > 
> >  app/test/test_mbuf.c                 | 2 +-
> >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >  lib/eal/include/rte_common.h         | 6 +++---
> >  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

Applied and completed.
Release notes added.
All squashed.