[dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

based on the original commit and the usage of rte_bsf64 it appears the
function should always have returned uint32_t instead of int which is
consistent with the cast introduced in the return statement.

Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
functions")
Cc: anatoly.burakov@intel.com

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/include/rte_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
index 1b630baf1..5e70ee7a8 100644
--- a/lib/librte_eal/include/rte_common.h
+++ b/lib/librte_eal/include/rte_common.h
@@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
  * @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);
-- 
2.30.0.vfs.0.2

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Ranjit Menon]

On 3/9/2021 10:41 PM, Tyler Retzlaff wrote:
> based on the original commit and the usage of rte_bsf64 it appears the
> function should always have returned uint32_t instead of int which is
> consistent with the cast introduced in the return statement.
>
> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> functions")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   lib/librte_eal/include/rte_common.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> index 1b630baf1..5e70ee7a8 100644
> --- a/lib/librte_eal/include/rte_common.h
> +++ b/lib/librte_eal/include/rte_common.h
> @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
>    * @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);
Acked-by: Ranjit Menon <ranjit.menon@intel.com>

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Stephen Hemminger]

On Tue,  9 Mar 2021 22:41:06 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> based on the original commit and the usage of rte_bsf64 it appears the
> function should always have returned uint32_t instead of int which is
> consistent with the cast introduced in the return statement.
> 
> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> functions")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/librte_eal/include/rte_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> index 1b630baf1..5e70ee7a8 100644
> --- a/lib/librte_eal/include/rte_common.h
> +++ b/lib/librte_eal/include/rte_common.h
> @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
>   * @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);

The cast is no longer needed, it should be removed.

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote:
> On Tue,  9 Mar 2021 22:41:06 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > based on the original commit and the usage of rte_bsf64 it appears the
> > function should always have returned uint32_t instead of int which is
> > consistent with the cast introduced in the return statement.
> > 
> > Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> > functions")
> > Cc: anatoly.burakov@intel.com
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/librte_eal/include/rte_common.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> > index 1b630baf1..5e70ee7a8 100644
> > --- a/lib/librte_eal/include/rte_common.h
> > +++ b/lib/librte_eal/include/rte_common.h
> > @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
> >   * @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);
> 
> The cast is no longer needed, it should be removed.

it's not so much about making it compile. it's about making it correct
with respect to original author intent, consistent with the rte_bsf32
declaration, other consumers of the inline function inside rte_common.h
and whether or not it makes sense to have a function that returns a
count of bits signed. based on those factors i'm asserting that the
cast is actually correct and it is the return type that is wrong.

your suggestion however would avoid having to deal with the downside of
changing the return type which is that an api change is necessary since
the function is exposed to applications. but even for something small
like this i think it is best to pursue the correct change rather than
sprinkle casts to (uint32_t) at various call-sites.

i'm in the process of sending the proposal to deprecate/change the
return type unless others feel the above evaluation missed the mark.

thanks!

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Morten Brørup]

CC: ABI Policy maintainers. You might have an opinion. Or not. :-)

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Wednesday, March 10, 2021 11:53 PM
> 
> On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote:
> > On Tue,  9 Mar 2021 22:41:06 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > based on the original commit and the usage of rte_bsf64 it appears
> the
> > > function should always have returned uint32_t instead of int which
> is
> > > consistent with the cast introduced in the return statement.
> > >
> > > Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> > > functions")
> > > Cc: anatoly.burakov@intel.com
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/librte_eal/include/rte_common.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_eal/include/rte_common.h
> b/lib/librte_eal/include/rte_common.h
> > > index 1b630baf1..5e70ee7a8 100644
> > > --- a/lib/librte_eal/include/rte_common.h
> > > +++ b/lib/librte_eal/include/rte_common.h
> > > @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
> > >   * @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);
> >
> > The cast is no longer needed, it should be removed.
> 
> it's not so much about making it compile. it's about making it correct
> with respect to original author intent, consistent with the rte_bsf32
> declaration, other consumers of the inline function inside rte_common.h
> and whether or not it makes sense to have a function that returns a
> count of bits signed. based on those factors i'm asserting that the
> cast is actually correct and it is the return type that is wrong.
> 
> your suggestion however would avoid having to deal with the downside of
> changing the return type which is that an api change is necessary since
> the function is exposed to applications. but even for something small
> like this i think it is best to pursue the correct change rather than
> sprinkle casts to (uint32_t) at various call-sites.
> 
> i'm in the process of sending the proposal to deprecate/change the
> return type unless others feel the above evaluation missed the mark.
> 
> thanks!

Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions:
- rte_bsf32()
- rte_bsf32_safe()
- rte_fls_u32()
- rte_bsf64()
- rte_fls_u64()
- rte_log2_u32()
- rte_log2_u64()

They should all return either int or uint32_t.

Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation.

For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from.

I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file.

(Please note that the functions returning a Boolean value as an int type should keep doing that.)


Med venlig hilsen / kind regards
- Morten Brørup

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Kinsella, Ray]

On 12/03/2021 07:34, Morten Brørup wrote:
> CC: ABI Policy maintainers. You might have an opinion. Or not. :-)

My 2c is that this is affecting inlined functions from include/rte_common.h.
Although not ideal, the practical effect on Binary Compatibility is therefore likely to be _nil_.

Just a small comment - when Steve points out "The cast is no longer needed, it should be removed."
I think Tyler _may_ have mis-understood his point when he said 

"it's not so much about making it compile. it's about making it correct
with respect to original author intent, consistent with the rte_bsf32
declaration ... "

My interpretation of what Steve said, is that once you change the return type
to uint32_t ... there is no need for the cast any more, that is all. 

> 
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
>> Sent: Wednesday, March 10, 2021 11:53 PM
>>
>> On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote:
>>> On Tue,  9 Mar 2021 22:41:06 -0800
>>> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
>>>
>>>> based on the original commit and the usage of rte_bsf64 it appears
>> the
>>>> function should always have returned uint32_t instead of int which
>> is
>>>> consistent with the cast introduced in the return statement.
>>>>
>>>> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
>>>> functions")
>>>> Cc: anatoly.burakov@intel.com
>>>>
>>>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>>> ---
>>>>  lib/librte_eal/include/rte_common.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_eal/include/rte_common.h
>> b/lib/librte_eal/include/rte_common.h
>>>> index 1b630baf1..5e70ee7a8 100644
>>>> --- a/lib/librte_eal/include/rte_common.h
>>>> +++ b/lib/librte_eal/include/rte_common.h
>>>> @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
>>>>   * @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);
>>>
>>> The cast is no longer needed, it should be removed.
>>
>> it's not so much about making it compile. it's about making it correct
>> with respect to original author intent, consistent with the rte_bsf32
>> declaration, other consumers of the inline function inside rte_common.h
>> and whether or not it makes sense to have a function that returns a
>> count of bits signed. based on those factors i'm asserting that the
>> cast is actually correct and it is the return type that is wrong.
>>
>> your suggestion however would avoid having to deal with the downside of
>> changing the return type which is that an api change is necessary since
>> the function is exposed to applications. but even for something small
>> like this i think it is best to pursue the correct change rather than
>> sprinkle casts to (uint32_t) at various call-sites.
>>
>> i'm in the process of sending the proposal to deprecate/change the
>> return type unless others feel the above evaluation missed the mark.
>>
>> thanks!
> 
> Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions:
> - rte_bsf32()
> - rte_bsf32_safe()
> - rte_fls_u32()
> - rte_bsf64()
> - rte_fls_u64()
> - rte_log2_u32()
> - rte_log2_u64()
> 
> They should all return either int or uint32_t.
> 
> Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation.
> 
> For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from.
> 
> I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file.
> 
> (Please note that the functions returning a Boolean value as an int type should keep doing that.)
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup
> 

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

On Fri, Mar 12, 2021 at 11:46:39AM +0000, Kinsella, Ray wrote:
> 
> 
> On 12/03/2021 07:34, Morten Brørup wrote:
> > CC: ABI Policy maintainers. You might have an opinion. Or not. :-)
> 
> My 2c is that this is affecting inlined functions from include/rte_common.h.
> Although not ideal, the practical effect on Binary Compatibility is therefore likely to be _nil_.
> 
> Just a small comment - when Steve points out "The cast is no longer needed, it should be removed."
> I think Tyler _may_ have mis-understood his point when he said 
> 
> "it's not so much about making it compile. it's about making it correct
> with respect to original author intent, consistent with the rte_bsf32
> declaration ... "
> 
> My interpretation of what Steve said, is that once you change the return type
> to uint32_t ... there is no need for the cast any more, that is all. 

yes, but saying it to what end if not to imply that should be the change
made? or are you suggesting it was just a passive observation? in this
instance it could have been formed as a question instead of a statement.

  "why not just remove the cast?"

based on the context of the patch in isolation i would agree but proper
due-diligence (review of commit history and usage) gives weight to the
alternative patch supplied.

For example:

1. Removed bsf64 function from testpmd.c. Original return type was
   uint32_t

// copy from testpmd.c
-static inline uint32_t
-bsf64(uint64_t v)
-{
-       return (uint32_t)__builtin_ctzll(v);
-}
-

2. Removed bsf64 function from eal_memalloc.c. Original return type was
   uint32_t

-static inline uint32_t
-bsf64(uint64_t v)
-{
-       return (uint32_t)__builtin_ctzll(v);
-}
-

3. Existing usage adapted to rte_bsf64 inline function. Return type is
   assumed to be uint32_t.

 static inline uint32_t
 log2_u64(uint64_t v)
 {
        if (v == 0)
                return 0;
        v = rte_align64pow2(v);
-       return bsf64(v);
+       return rte_bsf64(v);
 }


4. Existing usage adapted to rte_bsf64 inline function. Return type is
   assumed to be uint32_t.

 static inline uint32_t
 log2_u64(uint64_t v)
 {
        if (v == 0)
                return 0;
        v = rte_align64pow2(v);
-       return bsf64(v);
+       return rte_bsf64(v);
 }

5. Existing usage adapted to rte_bsf64 inline function. Return type is
   assumed to be uint32_t.

@@ -502,7 +543,7 @@ rte_bsf64_safe(uint64_t v, uint32_t *pos)
        if (v == 0)
                return 0;

-       *pos = __builtin_ctzll(v);
+       *pos = rte_bsf64(v);
        return 1;
 }

6. I will also concede that the test code _does_ use explicit casts to
   uint32_t but i evaluated their use to be the likely product of cut &
   paste because people get bored writing test code. Further, the cast
   for the test code on the rte_bsf32 is completely superfluous.

In addition to the commit history.

7. Does it make any sense for the value to be represented as signed? I
   would argue no. Additionally, the function as implemented does not
   attempt to report failure which would necessitate overloading the return
   value with -1 sentinel to indicate failure.

Based on these items it is difficult for me to believe that just
discarding the cast is correct.  Instead it feels more like papering
over a problem to avoid breaking API (ABI is not impacted).

> 
> > 
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> >> Sent: Wednesday, March 10, 2021 11:53 PM
> >>
> >> On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote:
> >>> On Tue,  9 Mar 2021 22:41:06 -0800
> >>> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >>>
> >>>> based on the original commit and the usage of rte_bsf64 it appears
> >> the
> >>>> function should always have returned uint32_t instead of int which
> >> is
> >>>> consistent with the cast introduced in the return statement.
> >>>>
> >>>> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> >>>> functions")
> >>>> Cc: anatoly.burakov@intel.com
> >>>>
> >>>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >>>> ---
> >>>>  lib/librte_eal/include/rte_common.h | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/lib/librte_eal/include/rte_common.h
> >> b/lib/librte_eal/include/rte_common.h
> >>>> index 1b630baf1..5e70ee7a8 100644
> >>>> --- a/lib/librte_eal/include/rte_common.h
> >>>> +++ b/lib/librte_eal/include/rte_common.h
> >>>> @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
> >>>>   * @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);
> >>>
> >>> The cast is no longer needed, it should be removed.
> >>
> >> it's not so much about making it compile. it's about making it correct
> >> with respect to original author intent, consistent with the rte_bsf32
> >> declaration, other consumers of the inline function inside rte_common.h
> >> and whether or not it makes sense to have a function that returns a
> >> count of bits signed. based on those factors i'm asserting that the
> >> cast is actually correct and it is the return type that is wrong.
> >>
> >> your suggestion however would avoid having to deal with the downside of
> >> changing the return type which is that an api change is necessary since
> >> the function is exposed to applications. but even for something small
> >> like this i think it is best to pursue the correct change rather than
> >> sprinkle casts to (uint32_t) at various call-sites.
> >>
> >> i'm in the process of sending the proposal to deprecate/change the
> >> return type unless others feel the above evaluation missed the mark.
> >>
> >> thanks!
> > 
> > Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions:
> > - rte_bsf32()
> > - rte_bsf32_safe()
> > - rte_fls_u32()
> > - rte_bsf64()
> > - rte_fls_u64()
> > - rte_log2_u32()
> > - rte_log2_u64()
> > 
> > They should all return either int or uint32_t.
> > 
> > Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation.
> > 
> > For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from.
> > 
> > I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file.
> > 
> > (Please note that the functions returning a Boolean value as an int type should keep doing that.)
> > 
> > 
> > Med venlig hilsen / kind regards
> > - Morten Brørup
> > 

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

On Fri, Mar 12, 2021 at 08:34:50AM +0100, Morten Brørup wrote:
> CC: ABI Policy maintainers. You might have an opinion. Or not. :-)
> 
> 
> Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions:
> - rte_bsf32()
> - rte_bsf32_safe()
> - rte_fls_u32()
> - rte_bsf64()
> - rte_fls_u64()
> - rte_log2_u32()
> - rte_log2_u64()

agreed, happy to review the whole set and deal with it all at once.

> 
> They should all return either int or uint32_t.
> 
> Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation.

yes, i suspect gcc builtins return int because of the default type promotion. probably historical be interesting to get an old gcc hand to tell us a story.

> 
> For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from.

horses for courses, if it doesn't make sense to interpret as signed then
i don't see a lot of value in using signed and it can open up avenues of
exploit.

> 
> I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file.

agree completely, consistency.

> 
> (Please note that the functions returning a Boolean value as an int type should keep doing that.)

i'm not planning on changing int -> _Bool. but i am curious about your
comment. stdbool.h is already used in the code base is there a compiler
in use that does not support _Bool. this is purely my interest, i don't
propose any change.

> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup
> 

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Friday, March 12, 2021 7:24 PM
> 
> On Fri, Mar 12, 2021 at 08:34:50AM +0100, Morten Brørup wrote:
> > CC: ABI Policy maintainers. You might have an opinion. Or not. :-)
> >
> >
> > Please also update the similar math functions in rte_common.h, so the
> return type is consistent across these functions:
> > - rte_bsf32()
> > - rte_bsf32_safe()
> > - rte_fls_u32()
> > - rte_bsf64()
> > - rte_fls_u64()
> > - rte_log2_u32()
> > - rte_log2_u64()
> 
> agreed, happy to review the whole set and deal with it all at once.

Ups. I should have omitted rte_bsf32_safe() from the list. It returns a Boolean.

> 
> >
> > They should all return either int or uint32_t.
> >
> > Standard C conventions would have them all return int (probably due to
> C's default type promotion to int when used in calculations), which is also
> the type returned by their underlying implementation.
> 
> yes, i suspect gcc builtins return int because of the default type
> promotion. probably historical be interesting to get an old gcc hand to
> tell us a story.
> 
> >
> > For some unknown reason, DPDK often uses uint32_t where you would
> normally use int. I guess it was inspired by MISRA C (for embedded
> systems); but it is not a documented conventions, and often deviated from.
> 
> horses for courses, if it doesn't make sense to interpret as signed then
> i don't see a lot of value in using signed and it can open up avenues of
> exploit.

I agree with you on this. The best return type is determined by considering how the return value is going to be used.

I could argue that these are mathematical functions, so they can be used for any kind of math, including math involving negative numbers. On the other hand, DPDK generally uses uint32_t for positive integers; and this also seems to be the original author's intention.

> 
> >
> > I don't have a personal preference for int or uint32_t here. But at least
> follow the same convention in the same header file.
> 
> agree completely, consistency.

Looking closer at it, uint32_t is probably closer to general DPDK consistency.

> 
> >
> > (Please note that the functions returning a Boolean value as an int type
> should keep doing that.)
> 
> i'm not planning on changing int -> _Bool. but i am curious about your
> comment. stdbool.h is already used in the code base is there a compiler
> in use that does not support _Bool. this is purely my interest, i don't
> propose any change.

I only mentioned this to ensure that you don't change the Boolean return values from int to uint32_t.

For arguments sate, they could be changed to bool, which is an acceptable type in DPDK, ref.:
https://elixir.bootlin.com/dpdk/latest/C/ident/bool

However, I agree with you to not propose any change here!

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

On Fri, Mar 12, 2021 at 10:13:30PM +0100, Morten Brørup wrote:
> > >
> > > Please also update the similar math functions in rte_common.h, so the
> > return type is consistent across these functions:
> > > - rte_bsf32()
> > > - rte_bsf32_safe()
> > > - rte_fls_u32()
> > > - rte_bsf64()
> > > - rte_fls_u64()
> > > - rte_log2_u32()
> > > - rte_log2_u64()
> > 
> > agreed, happy to review the whole set and deal with it all at once.
> 
> Ups. I should have omitted rte_bsf32_safe() from the list. It returns a Boolean.

if we can agree that we can use C11 as a base we could just do away with
all this dumb 32-bit vs 64-bit static selection at the call site (at
least for rte_bsf32() and rte_bsf64() and probably others).

i could just introduce the following macro and deprecate the current
inline functions.

#define rte_bsf(v) \
    (uint32_t)_Generic((v), \
        uint8_t: __builtin_ctz, \
        uint16_t: __builtin_ctz, \
        uint32_t: __builtin_ctz, \
        default: __builtin_ctzll)(v)

uint8_t a = ...;
uint16_t b = ...;
uint32_t c = ...;
uint64_t d = ...;

the following would jw as intended, though given the range of the value
that may be returned we could narrow the cast to uint8_t so we don't
have to sprinkle casts in places where usage like uint8_t x = rte_bsf(v);
exists.

rte_bsf(a);
rte_bsf(b);
rte_bsf(c);
rte_bsf(d);

anyway, if people care let me know otherwise i'm just going to review
and fix what is already there.

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Saturday, March 13, 2021 2:10 AM
> 
> On Fri, Mar 12, 2021 at 10:13:30PM +0100, Morten Brørup wrote:
> > > >
> > > > Please also update the similar math functions in rte_common.h, so the
> > > return type is consistent across these functions:
> > > > - rte_bsf32()
> > > > - rte_bsf32_safe()
> > > > - rte_fls_u32()
> > > > - rte_bsf64()
> > > > - rte_fls_u64()
> > > > - rte_log2_u32()
> > > > - rte_log2_u64()
> > >
> > > agreed, happy to review the whole set and deal with it all at once.
> >
> > Ups. I should have omitted rte_bsf32_safe() from the list. It returns a
> Boolean.
> 
> if we can agree that we can use C11 as a base we could just do away with
> all this dumb 32-bit vs 64-bit static selection at the call site (at
> least for rte_bsf32() and rte_bsf64() and probably others).
> 
> i could just introduce the following macro and deprecate the current
> inline functions.
> 
> #define rte_bsf(v) \
>     (uint32_t)_Generic((v), \
>         uint8_t: __builtin_ctz, \
>         uint16_t: __builtin_ctz, \
>         uint32_t: __builtin_ctz, \
>         default: __builtin_ctzll)(v)
> 
> uint8_t a = ...;
> uint16_t b = ...;
> uint32_t c = ...;
> uint64_t d = ...;
> 
> the following would jw as intended, though given the range of the value
> that may be returned we could narrow the cast to uint8_t so we don't
> have to sprinkle casts in places where usage like uint8_t x = rte_bsf(v);
> exists.
> 
> rte_bsf(a);
> rte_bsf(b);
> rte_bsf(c);
> rte_bsf(d);
> 
> anyway, if people care let me know otherwise i'm just going to review
> and fix what is already there.

That would certainly be a nice improvement. But going down that road should include reviewing and fixing all DPDK libraries with 32/64 versions of the same functions, e.g. rte_bitops.h - which probably requires a significant effort.

So just limit the task to fixing what is already there.

Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Tyler Retzlaff]

On Sat, Mar 13, 2021 at 08:29:10AM +0100, Morten Brørup wrote:
> 
> That would certainly be a nice improvement. But going down that road should include reviewing and fixing all DPDK libraries with 32/64 versions of the same functions, e.g. rte_bitops.h - which probably requires a significant effort.

i only have time available for smaller incremental improvements so
unless it is small pieces working toward the final goal it will be
difficult to get time approved.

> 
> So just limit the task to fixing what is already there.
> 

good enough for me, we'll just do that for now.

thanks!