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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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