diff mbox series

[v2,1/2] eal: provide leading and trailing zero bit count abstraction

Message ID 1669246997-30592-2-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series eal: provide leading and trailing zero bit count |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff Nov. 23, 2022, 11:43 p.m. UTC 
  Provide an abstraction for leading and trailing zero bit counting
functions to hide compiler specific intrinsics and builtins.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/meson.build    |   1 +
 lib/eal/include/rte_bitcount.h | 265 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 266 insertions(+)
 create mode 100644 lib/eal/include/rte_bitcount.h

Comments

Morten Brørup Nov. 24, 2022, 10:17 a.m. UTC | #1 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 24 November 2022 00.43
> 
> Provide an abstraction for leading and trailing zero bit counting
> functions to hide compiler specific intrinsics and builtins.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Related functions already exist in lib/eal/include/rte_common.h.

If some functions are missing, it would be good to add them.

Also, it makes sense moving them out of rte_common.h.
Tyler Retzlaff Nov. 28, 2022, 5:13 p.m. UTC | #2 
On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 24 November 2022 00.43
> > 
> > Provide an abstraction for leading and trailing zero bit counting
> > functions to hide compiler specific intrinsics and builtins.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> 

let me unpack what is being asked for here.

> Related functions already exist in lib/eal/include/rte_common.h.

i don't understand. are you saying these inline functions duplicate
existing bit counting functionality? if so i'll relocate any you
identify.

> 
> If some functions are missing, it would be good to add them.

the change here is to address a specific family of functions that are
impeding portability of the rte_common.h header and nothing more.

i'm open to having a separate mail thread discussing what other
functions we might want and whether to relocate code out of rte_common.h
since that is likely to be a drawn out discussion it shouldn't be a
blocking dependency of this series.

> 
> Also, it makes sense moving them out of rte_common.h.

thanks!
Morten Brørup Nov. 28, 2022, 5:22 p.m. UTC | #3 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 28 November 2022 18.14
> 
> On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Thursday, 24 November 2022 00.43
> > >
> > > Provide an abstraction for leading and trailing zero bit counting
> > > functions to hide compiler specific intrinsics and builtins.
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> >
> 
> let me unpack what is being asked for here.
> 
> > Related functions already exist in lib/eal/include/rte_common.h.
> 
> i don't understand. are you saying these inline functions duplicate
> existing bit counting functionality? if so i'll relocate any you
> identify.

Sorry about not being clear about my intentions with the feedback.

I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.

> 
> >
> > If some functions are missing, it would be good to add them.
> 
> the change here is to address a specific family of functions that are
> impeding portability of the rte_common.h header and nothing more.
> 
> i'm open to having a separate mail thread discussing what other
> functions we might want and whether to relocate code out of
> rte_common.h
> since that is likely to be a drawn out discussion it shouldn't be a
> blocking dependency of this series.

I do not request any new functions; I'll leave it up to you to decide if anything would be good to add.

> 
> >
> > Also, it makes sense moving them out of rte_common.h.
> 
> thanks!

Thank you! Cleaning up is always appreciated.
Tyler Retzlaff Nov. 28, 2022, 5:27 p.m. UTC | #4 
On Mon, Nov 28, 2022 at 06:22:10PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Monday, 28 November 2022 18.14
> > 
> > On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > Sent: Thursday, 24 November 2022 00.43
> > > >
> > > > Provide an abstraction for leading and trailing zero bit counting
> > > > functions to hide compiler specific intrinsics and builtins.
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > >
> > 
> > let me unpack what is being asked for here.
> > 
> > > Related functions already exist in lib/eal/include/rte_common.h.
> > 
> > i don't understand. are you saying these inline functions duplicate
> > existing bit counting functionality? if so i'll relocate any you
> > identify.
> 
> Sorry about not being clear about my intentions with the feedback.
> 
> I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.

oh! not a problem. i'm very keen to catch any mistakes, thought i had
missed something.

> 
> > 
> > >
> > > If some functions are missing, it would be good to add them.
> > 
> > the change here is to address a specific family of functions that are
> > impeding portability of the rte_common.h header and nothing more.
> > 
> > i'm open to having a separate mail thread discussing what other
> > functions we might want and whether to relocate code out of
> > rte_common.h
> > since that is likely to be a drawn out discussion it shouldn't be a
> > blocking dependency of this series.
> 
> I do not request any new functions; I'll leave it up to you to decide if anything would be good to add.
> 
> > 
> > >
> > > Also, it makes sense moving them out of rte_common.h.
> > 
> > thanks!
> 
> Thank you! Cleaning up is always appreciated.

yes, if i can get more rapid approvals there is a lot more in the
pipeline. i'd like gain some momentum if possible.

ty
Morten Brørup Jan. 5, 2023, 7:09 a.m. UTC | #5 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 24 November 2022 00.43
> 
> Provide an abstraction for leading and trailing zero bit counting
> functions to hide compiler specific intrinsics and builtins.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/meson.build    |   1 +
>  lib/eal/include/rte_bitcount.h | 265
> +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 266 insertions(+)
>  create mode 100644 lib/eal/include/rte_bitcount.h
> 
> diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> index cfcd40a..8ff1d65 100644
> --- a/lib/eal/include/meson.build
> +++ b/lib/eal/include/meson.build
> @@ -5,6 +5,7 @@ includes += include_directories('.')
> 
>  headers += files(
>          'rte_alarm.h',
> +        'rte_bitcount.h',
>          'rte_bitmap.h',
>          'rte_bitops.h',
>          'rte_branch_prediction.h',
> diff --git a/lib/eal/include/rte_bitcount.h
> b/lib/eal/include/rte_bitcount.h
> new file mode 100644
> index 0000000..587de52
> --- /dev/null
> +++ b/lib/eal/include/rte_bitcount.h
> @@ -0,0 +1,265 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (C) 2022 Microsoft Corporation
> + */
> +
> +#ifndef _RTE_BITCOUNT_H_
> +#define _RTE_BITCOUNT_H_
> +
> +#include <rte_compat.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#ifdef RTE_TOOLCHAIN_MSVC
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Get the count of leading 0-bits in v.
> + *
> + * @param v
> + *   The value.
> + * @return
> + *   The count of leading zero bits.
> + */
> +__rte_experimental
> +static inline unsigned int
> +rte_clz(unsigned int v)
> +{
> +	unsigned long rv;
> +
> +	(void)_BitScanReverse(&rv, v);
> +
> +	return (unsigned int)rv;
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Get the count of leading 0-bits in v.
> + *
> + * @param v
> + *   The value.
> + * @return
> + *   The count of leading zero bits.
> + */
> +__rte_experimental
> +static inline unsigned int
> +rte_clzl(unsigned long v)

Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.

E.g.: rte_clz32(uint32_t v)

> +{
> +	unsigned long rv;
> +
> +	(void)_BitScanReverse(&rv, v);
> +
> +	return (unsigned int)rv;
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Get the count of leading 0-bits in v.
> + *
> + * @param v
> + *   The value.
> + * @return
> + *   The count of leading zero bits.
> + */
> +__rte_experimental
> +static inline unsigned int
> +rte_clzll(unsigned long long v)

Same comment as above: e.g. rte_clz64(uint64_t v)
Thomas Monjalon Jan. 5, 2023, 9:01 a.m. UTC | #6 
05/01/2023 08:09, Morten Brørup:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Get the count of leading 0-bits in v.
> > + *
> > + * @param v
> > + *   The value.
> > + * @return
> > + *   The count of leading zero bits.
> > + */
> > +__rte_experimental
> > +static inline unsigned int
> > +rte_clzl(unsigned long v)
> 
> Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> 
> E.g.: rte_clz32(uint32_t v)

I agree on using numbers.
Thomas Monjalon Jan. 5, 2023, 9:04 a.m. UTC | #7 
28/11/2022 18:27, Tyler Retzlaff:
> On Mon, Nov 28, 2022 at 06:22:10PM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Monday, 28 November 2022 18.14
> > > 
> > > On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > Sent: Thursday, 24 November 2022 00.43
> > > > >
> > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > functions to hide compiler specific intrinsics and builtins.
> > > > >
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > 
> > > let me unpack what is being asked for here.
> > > 
> > > > Related functions already exist in lib/eal/include/rte_common.h.
> > > 
> > > i don't understand. are you saying these inline functions duplicate
> > > existing bit counting functionality? if so i'll relocate any you
> > > identify.
> > 
> > Sorry about not being clear about my intentions with the feedback.
> > 
> > I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.
> 
> oh! not a problem. i'm very keen to catch any mistakes, thought i had
> missed something.

I think we should move all bit-related functions together.
Please could you add another patch to your series
moving "ms1b"/"bsf"/"fls" functions in this file?
Tyler Retzlaff Jan. 5, 2023, 5:21 p.m. UTC | #8 
On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> 05/01/2023 08:09, Morten Brørup:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > notice
> > > + *
> > > + * Get the count of leading 0-bits in v.
> > > + *
> > > + * @param v
> > > + *   The value.
> > > + * @return
> > > + *   The count of leading zero bits.
> > > + */
> > > +__rte_experimental
> > > +static inline unsigned int
> > > +rte_clzl(unsigned long v)
> > 
> > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > 
> > E.g.: rte_clz32(uint32_t v)
> 
> I agree on using numbers.
> 

love the idea, fewer functions too.

though it is a shame we cannot adopt C11 standard because we could just
do away with the bit suffixes entirely.
Tyler Retzlaff Jan. 5, 2023, 5:23 p.m. UTC | #9 
On Thu, Jan 05, 2023 at 10:04:46AM +0100, Thomas Monjalon wrote:
> 28/11/2022 18:27, Tyler Retzlaff:
> > On Mon, Nov 28, 2022 at 06:22:10PM +0100, Morten Brørup wrote:
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > Sent: Monday, 28 November 2022 18.14
> > > > 
> > > > On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > Sent: Thursday, 24 November 2022 00.43
> > > > > >
> > > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > > functions to hide compiler specific intrinsics and builtins.
> > > > > >
> > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > 
> > > > let me unpack what is being asked for here.
> > > > 
> > > > > Related functions already exist in lib/eal/include/rte_common.h.
> > > > 
> > > > i don't understand. are you saying these inline functions duplicate
> > > > existing bit counting functionality? if so i'll relocate any you
> > > > identify.
> > > 
> > > Sorry about not being clear about my intentions with the feedback.
> > > 
> > > I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.
> > 
> > oh! not a problem. i'm very keen to catch any mistakes, thought i had
> > missed something.
> 
> I think we should move all bit-related functions together.
> Please could you add another patch to your series
> moving "ms1b"/"bsf"/"fls" functions in this file?
> 
> 

okay, so there is already a rte_bitops.h. i guess everything should go
there including the leading/trailing count functions instead of adding a
new header.

i'll introduce a new patch to the series that gathers the existing
functions into rte_bitops.h and place the new functions there too.

thanks
Tyler Retzlaff Jan. 5, 2023, 5:27 p.m. UTC | #10 
On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> On Thu, Jan 05, 2023 at 10:04:46AM +0100, Thomas Monjalon wrote:
> > 28/11/2022 18:27, Tyler Retzlaff:
> > > On Mon, Nov 28, 2022 at 06:22:10PM +0100, Morten Brørup wrote:
> > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > Sent: Monday, 28 November 2022 18.14
> > > > > 
> > > > > On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > Sent: Thursday, 24 November 2022 00.43
> > > > > > >
> > > > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > > > functions to hide compiler specific intrinsics and builtins.
> > > > > > >
> > > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > 
> > > > > let me unpack what is being asked for here.
> > > > > 
> > > > > > Related functions already exist in lib/eal/include/rte_common.h.
> > > > > 
> > > > > i don't understand. are you saying these inline functions duplicate
> > > > > existing bit counting functionality? if so i'll relocate any you
> > > > > identify.
> > > > 
> > > > Sorry about not being clear about my intentions with the feedback.
> > > > 
> > > > I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.
> > > 
> > > oh! not a problem. i'm very keen to catch any mistakes, thought i had
> > > missed something.
> > 
> > I think we should move all bit-related functions together.
> > Please could you add another patch to your series
> > moving "ms1b"/"bsf"/"fls" functions in this file?
> > 
> > 
> 
> okay, so there is already a rte_bitops.h. i guess everything should go
> there including the leading/trailing count functions instead of adding a
> new header.
> 
> i'll introduce a new patch to the series that gathers the existing
> functions into rte_bitops.h and place the new functions there too.
> 
> thanks

just as a further follow up, you do understand that this is technically
an api break?

moving functions from rte_common.h to rte_bitops.h will make translation
units that included only rte_common.h but used these functions will
fail to compile without being updated to include rte_bitops.h.

anyway, i'll submit v3 with this change anyway.
Tyler Retzlaff Jan. 5, 2023, 8:57 p.m. UTC | #11 
On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote:
> On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> > > > oh! not a problem. i'm very keen to catch any mistakes, thought i had
> > > > missed something.
> > > 
> > > I think we should move all bit-related functions together.
> > > Please could you add another patch to your series
> > > moving "ms1b"/"bsf"/"fls" functions in this file?
> > > 
> > > 
> > 
> > okay, so there is already a rte_bitops.h. i guess everything should go
> > there including the leading/trailing count functions instead of adding a
> > new header.
> > 
> > i'll introduce a new patch to the series that gathers the existing
> > functions into rte_bitops.h and place the new functions there too.
> > 
> > thanks
> 
> just as a further follow up, you do understand that this is technically
> an api break?
> 
> moving functions from rte_common.h to rte_bitops.h will make translation
> units that included only rte_common.h but used these functions will
> fail to compile without being updated to include rte_bitops.h.
> 
> anyway, i'll submit v3 with this change anyway.

so when attempting to do this it became immediately obvious that moving
just the bit op functions out is going to create a circular dependency
between rte_common.h, rte_bitops.h

once the bit ops are moved out of common there are still other inline
functions that remain in comman that require bringing bitops back in,
but bitops depends on common.

my compromise will be to break log2 and pow2 inline functions into their
own files to break the cycle (common no longer depends on bitops). i'll
submit patches for this but it ends up touching a lot more of the
tree to add back includes for log/pow inline use.

alternatively i can just not move the remaining bit manipulation
functions, let me know which is preferred.

thanks
Morten Brørup Jan. 5, 2023, 9:34 p.m. UTC | #12 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 5 January 2023 21.58
> 
> On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote:
> > On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> > > > > oh! not a problem. i'm very keen to catch any mistakes, thought
> i had
> > > > > missed something.
> > > >
> > > > I think we should move all bit-related functions together.
> > > > Please could you add another patch to your series
> > > > moving "ms1b"/"bsf"/"fls" functions in this file?
> > > >
> > > >
> > >
> > > okay, so there is already a rte_bitops.h. i guess everything should
> go
> > > there including the leading/trailing count functions instead of
> adding a
> > > new header.
> > >
> > > i'll introduce a new patch to the series that gathers the existing
> > > functions into rte_bitops.h and place the new functions there too.
> > >
> > > thanks
> >
> > just as a further follow up, you do understand that this is
> technically
> > an api break?
> >
> > moving functions from rte_common.h to rte_bitops.h will make
> translation
> > units that included only rte_common.h but used these functions will
> > fail to compile without being updated to include rte_bitops.h.
> >
> > anyway, i'll submit v3 with this change anyway.
> 
> so when attempting to do this it became immediately obvious that moving
> just the bit op functions out is going to create a circular dependency
> between rte_common.h, rte_bitops.h
> 
> once the bit ops are moved out of common there are still other inline
> functions that remain in comman that require bringing bitops back in,
> but bitops depends on common.
> 
> my compromise will be to break log2 and pow2 inline functions into
> their
> own files to break the cycle (common no longer depends on bitops). i'll
> submit patches for this but it ends up touching a lot more of the
> tree to add back includes for log/pow inline use.
> 
> alternatively i can just not move the remaining bit manipulation
> functions, let me know which is preferred.

It seems that no perfect solution exists, so we will have to live with a compromise. Here is another proposal for a compromise, for yours and Thomas's consideration:

I noticed that rte_bitops.h is mainly for setting/getting bits, used for accessing hardware.

Your functions are mathematical functions, and so are the similar functions in rte_common.h (which is why it makes sense to keep them together with yours). If we cannot clean up rte_common.h by moving them out, perhaps we should accept the current situation (until we find a way to move them out) and just add your mathematical functions where the existing mathematical functions reside, i.e. in rte_common.h.

This proposal only makes the existing mess slightly larger; it doesn't create a new kind of mess.

-Morten
Tyler Retzlaff Jan. 5, 2023, 10:06 p.m. UTC | #13 
On Thu, Jan 05, 2023 at 10:34:55PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 5 January 2023 21.58
> > 
> > On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote:
> > > On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> > > > > > oh! not a problem. i'm very keen to catch any mistakes, thought
> > i had
> > > > > > missed something.
> > > > >
> > > > > I think we should move all bit-related functions together.
> > > > > Please could you add another patch to your series
> > > > > moving "ms1b"/"bsf"/"fls" functions in this file?
> > > > >
> > > > >
> > > >
> > > > okay, so there is already a rte_bitops.h. i guess everything should
> > go
> > > > there including the leading/trailing count functions instead of
> > adding a
> > > > new header.
> > > >
> > > > i'll introduce a new patch to the series that gathers the existing
> > > > functions into rte_bitops.h and place the new functions there too.
> > > >
> > > > thanks
> > >
> > > just as a further follow up, you do understand that this is
> > technically
> > > an api break?
> > >
> > > moving functions from rte_common.h to rte_bitops.h will make
> > translation
> > > units that included only rte_common.h but used these functions will
> > > fail to compile without being updated to include rte_bitops.h.
> > >
> > > anyway, i'll submit v3 with this change anyway.
> > 
> > so when attempting to do this it became immediately obvious that moving
> > just the bit op functions out is going to create a circular dependency
> > between rte_common.h, rte_bitops.h
> > 
> > once the bit ops are moved out of common there are still other inline
> > functions that remain in comman that require bringing bitops back in,
> > but bitops depends on common.
> > 
> > my compromise will be to break log2 and pow2 inline functions into
> > their
> > own files to break the cycle (common no longer depends on bitops). i'll
> > submit patches for this but it ends up touching a lot more of the
> > tree to add back includes for log/pow inline use.
> > 
> > alternatively i can just not move the remaining bit manipulation
> > functions, let me know which is preferred.
> 
> It seems that no perfect solution exists, so we will have to live with a compromise. Here is another proposal for a compromise, for yours and Thomas's consideration:
> 
> I noticed that rte_bitops.h is mainly for setting/getting bits, used for accessing hardware.
> 
> Your functions are mathematical functions, and so are the similar functions in rte_common.h (which is why it makes sense to keep them together with yours). If we cannot clean up rte_common.h by moving them out, perhaps we should accept the current situation (until we find a way to move them out) and just add your mathematical functions where the existing mathematical functions reside, i.e. in rte_common.h.
> 
> This proposal only makes the existing mess slightly larger; it doesn't create a new kind of mess.

so i fudged around a bit to see if i could get a happy medium. i ended
up with this.

remove include of rte_debug.h from rte_bitops.h

  * had to remove the RTE_ASSERT from existing rte_bitops.h functions
  * this breaks a good piece of the cycle debug -> log -> common -> bitops -> debug
  * deal breaker? i don't think it was right that we were getting all
    of log, common just for using bitops anyway.

move pow2 functions from rte_common.h -> rte_pow2ops.h
  * new header includes rte_bitops.h

move log2 functions from rte_common.h -> rte_log2ops.h
  * new header includes rte_bitops.h, rte_pow2ops.h

include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into
rte_common.h

  * this is done to reduce the impact of compatibility break by
    continuing to expose the pow2/log2/bitops via rte_common.h

so we end up with 3 standalone headers, where the whole tree builds
without having to add a pile of includes for the new headers. we can
later deprecate the exposure of the inline functions when including
rte_common.h

  * one caveat is that there was some contamination coming in via the
    removed rte_debug.h where rte_bitops.h was used. so technically
    a break of api too.

objections?

if this is no good i'll just fold my new functions into rte_common.h and
leave the mess for the next person, though i am trying not to do that.

thanks for the discussion.
Morten Brørup Jan. 5, 2023, 11:10 p.m. UTC | #14 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 5 January 2023 23.06
> 
> On Thu, Jan 05, 2023 at 10:34:55PM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Thursday, 5 January 2023 21.58
> > >
> > > On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote:
> > > > On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> > > > > > > oh! not a problem. i'm very keen to catch any mistakes,
> thought
> > > i had
> > > > > > > missed something.
> > > > > >
> > > > > > I think we should move all bit-related functions together.
> > > > > > Please could you add another patch to your series
> > > > > > moving "ms1b"/"bsf"/"fls" functions in this file?
> > > > > >
> > > > > >
> > > > >
> > > > > okay, so there is already a rte_bitops.h. i guess everything
> should
> > > go
> > > > > there including the leading/trailing count functions instead of
> > > adding a
> > > > > new header.
> > > > >
> > > > > i'll introduce a new patch to the series that gathers the
> existing
> > > > > functions into rte_bitops.h and place the new functions there
> too.
> > > > >
> > > > > thanks
> > > >
> > > > just as a further follow up, you do understand that this is
> > > technically
> > > > an api break?
> > > >
> > > > moving functions from rte_common.h to rte_bitops.h will make
> > > translation
> > > > units that included only rte_common.h but used these functions
> will
> > > > fail to compile without being updated to include rte_bitops.h.
> > > >
> > > > anyway, i'll submit v3 with this change anyway.
> > >
> > > so when attempting to do this it became immediately obvious that
> moving
> > > just the bit op functions out is going to create a circular
> dependency
> > > between rte_common.h, rte_bitops.h
> > >
> > > once the bit ops are moved out of common there are still other
> inline
> > > functions that remain in comman that require bringing bitops back
> in,
> > > but bitops depends on common.
> > >
> > > my compromise will be to break log2 and pow2 inline functions into
> > > their
> > > own files to break the cycle (common no longer depends on bitops).
> i'll
> > > submit patches for this but it ends up touching a lot more of the
> > > tree to add back includes for log/pow inline use.
> > >
> > > alternatively i can just not move the remaining bit manipulation
> > > functions, let me know which is preferred.
> >
> > It seems that no perfect solution exists, so we will have to live
> with a compromise. Here is another proposal for a compromise, for yours
> and Thomas's consideration:
> >
> > I noticed that rte_bitops.h is mainly for setting/getting bits, used
> for accessing hardware.
> >
> > Your functions are mathematical functions, and so are the similar
> functions in rte_common.h (which is why it makes sense to keep them
> together with yours). If we cannot clean up rte_common.h by moving them
> out, perhaps we should accept the current situation (until we find a
> way to move them out) and just add your mathematical functions where
> the existing mathematical functions reside, i.e. in rte_common.h.
> >
> > This proposal only makes the existing mess slightly larger; it
> doesn't create a new kind of mess.
> 
> so i fudged around a bit to see if i could get a happy medium. i ended
> up with this.
> 
> remove include of rte_debug.h from rte_bitops.h
> 
>   * had to remove the RTE_ASSERT from existing rte_bitops.h functions
>   * this breaks a good piece of the cycle debug -> log -> common ->
> bitops -> debug
>   * deal breaker? i don't think it was right that we were getting all
>     of log, common just for using bitops anyway.
> 
> move pow2 functions from rte_common.h -> rte_pow2ops.h
>   * new header includes rte_bitops.h
> 
> move log2 functions from rte_common.h -> rte_log2ops.h
>   * new header includes rte_bitops.h, rte_pow2ops.h
> 
> include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into
> rte_common.h
> 
>   * this is done to reduce the impact of compatibility break by
>     continuing to expose the pow2/log2/bitops via rte_common.h
> 
> so we end up with 3 standalone headers, where the whole tree builds
> without having to add a pile of includes for the new headers. we can
> later deprecate the exposure of the inline functions when including
> rte_common.h
> 
>   * one caveat is that there was some contamination coming in via the
>     removed rte_debug.h where rte_bitops.h was used. so technically
>     a break of api too.
> 
> objections?
> 
> if this is no good i'll just fold my new functions into rte_common.h
> and
> leave the mess for the next person, though i am trying not to do that.
> 
> thanks for the discussion.

Here's some long term thinking: EAL has grown into a trashcan where too much is thrown in. It should only be a thin shim to abstract the underlying hardware and O/S environment. A step in that direction could be splitting the current EAL into a true EAL and a Utils library. Not now, but perhaps some day in a rosy future.

Your proposal effectively makes rte_common.h even bigger by including rte_bitops.h (which was intended for accessing hardware). I am not sure it is a step in the right direction. On the other hand, introducing yet another header file for bit-mathematical functions is probably worse than adding them to rte_bitops.h.

I can't come up with something good myself, but I lean towards simply adding your functions to rte_common.h and live with the existing mess. If you think your proposal is better, I will not object. I'm only voicing my thoughts.

@Thomas may have another perspective on the matter.

Thanks for all the work you put into this.

-Morten
Stephen Hemminger Jan. 6, 2023, 12:32 a.m. UTC | #15 
On Thu, 5 Jan 2023 09:21:18 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > 05/01/2023 08:09, Morten Brørup:  
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > notice
> > > > + *
> > > > + * Get the count of leading 0-bits in v.
> > > > + *
> > > > + * @param v
> > > > + *   The value.
> > > > + * @return
> > > > + *   The count of leading zero bits.
> > > > + */
> > > > +__rte_experimental
> > > > +static inline unsigned int
> > > > +rte_clzl(unsigned long v)  
> > > 
> > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > 
> > > E.g.: rte_clz32(uint32_t v)  
> > 
> > I agree on using numbers.
> >   
> 
> love the idea, fewer functions too.
> 
> though it is a shame we cannot adopt C11 standard because we could just
> do away with the bit suffixes entirely.

We could but the project needs to support older RHEL releases
which have older tool sets. Though probably this is moot point given
how much meson seems to change.
Tyler Retzlaff Jan. 6, 2023, 1:04 a.m. UTC | #16 
On Fri, Jan 06, 2023 at 12:10:45AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 5 January 2023 23.06
> > 
> > doesn't create a new kind of mess.
> > 
> > so i fudged around a bit to see if i could get a happy medium. i ended
> > up with this.
> > 
> > remove include of rte_debug.h from rte_bitops.h
> > 
> >   * had to remove the RTE_ASSERT from existing rte_bitops.h functions
> >   * this breaks a good piece of the cycle debug -> log -> common ->
> > bitops -> debug
> >   * deal breaker? i don't think it was right that we were getting all
> >     of log, common just for using bitops anyway.
> > 
> > move pow2 functions from rte_common.h -> rte_pow2ops.h
> >   * new header includes rte_bitops.h
> > 
> > move log2 functions from rte_common.h -> rte_log2ops.h
> >   * new header includes rte_bitops.h, rte_pow2ops.h
> > 
> > include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into
> > rte_common.h
> > 
> >   * this is done to reduce the impact of compatibility break by
> >     continuing to expose the pow2/log2/bitops via rte_common.h
> > 
> > so we end up with 3 standalone headers, where the whole tree builds
> > without having to add a pile of includes for the new headers. we can
> > later deprecate the exposure of the inline functions when including
> > rte_common.h
> > 
> >   * one caveat is that there was some contamination coming in via the
> >     removed rte_debug.h where rte_bitops.h was used. so technically
> >     a break of api too.
> > 
> > objections?
> > 
> > if this is no good i'll just fold my new functions into rte_common.h
> > and
> > leave the mess for the next person, though i am trying not to do that.
> > 
> > thanks for the discussion.
> 
> Here's some long term thinking: EAL has grown into a trashcan where too much is thrown in. It should only be a thin shim to abstract the underlying hardware and O/S environment. A step in that direction could be splitting the current EAL into a true EAL and a Utils library. Not now, but perhaps some day in a rosy future.
> 
> Your proposal effectively makes rte_common.h even bigger by including rte_bitops.h (which was intended for accessing hardware). I am not sure it is a step in the right direction. On the other hand, introducing yet another header file for bit-mathematical functions is probably worse than adding them to rte_bitops.h.
> 
> I can't come up with something good myself, but I lean towards simply adding your functions to rte_common.h and live with the existing mess. If you think your proposal is better, I will not object. I'm only voicing my thoughts.

it's hard when we are starting with something monolithic and trying to
split it. you always end up with these iterative transitions toward what
you want because you have to keep some kind of compat.

> 
> @Thomas may have another perspective on the matter.

Thomas any last word after this back and forth?  This change isn't
necessarily blocking me but is a distraction i'd like to finish/offload.
> 
> Thanks for all the work you put into this.

np, it is somewhat out of necessity since i need this stuff to be
portable.  but having it move in the right direction at the same time is
a benefit for anyone who comes later.

ty
Thomas Monjalon Jan. 6, 2023, 10 a.m. UTC | #17 
05/01/2023 18:27, Tyler Retzlaff:
> On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote:
> > On Thu, Jan 05, 2023 at 10:04:46AM +0100, Thomas Monjalon wrote:
> > > 28/11/2022 18:27, Tyler Retzlaff:
> > > > On Mon, Nov 28, 2022 at 06:22:10PM +0100, Morten Brørup wrote:
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > Sent: Monday, 28 November 2022 18.14
> > > > > > 
> > > > > > On Thu, Nov 24, 2022 at 11:17:23AM +0100, Morten Brørup wrote:
> > > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > > Sent: Thursday, 24 November 2022 00.43
> > > > > > > >
> > > > > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > > > > functions to hide compiler specific intrinsics and builtins.
> > > > > > > >
> > > > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > 
> > > > > > let me unpack what is being asked for here.
> > > > > > 
> > > > > > > Related functions already exist in lib/eal/include/rte_common.h.
> > > > > > 
> > > > > > i don't understand. are you saying these inline functions duplicate
> > > > > > existing bit counting functionality? if so i'll relocate any you
> > > > > > identify.
> > > > > 
> > > > > Sorry about not being clear about my intentions with the feedback.
> > > > > 
> > > > > I'm not asking for anything; I only wanted to point at the similar family of functions in rte_common.h, to make sure that you were aware of them.
> > > > 
> > > > oh! not a problem. i'm very keen to catch any mistakes, thought i had
> > > > missed something.
> > > 
> > > I think we should move all bit-related functions together.
> > > Please could you add another patch to your series
> > > moving "ms1b"/"bsf"/"fls" functions in this file?
> > > 
> > > 
> > 
> > okay, so there is already a rte_bitops.h. i guess everything should go
> > there including the leading/trailing count functions instead of adding a
> > new header.

Yes good idea to gather all in rte_bitops.h.

> > i'll introduce a new patch to the series that gathers the existing
> > functions into rte_bitops.h and place the new functions there too.
> > 
> > thanks
> 
> just as a further follow up, you do understand that this is technically
> an api break?

Yes

> moving functions from rte_common.h to rte_bitops.h will make translation
> units that included only rte_common.h but used these functions will
> fail to compile without being updated to include rte_bitops.h.

These functions are not used a lot, it is a "small" API break.
Thomas Monjalon Jan. 6, 2023, 10:09 a.m. UTC | #18 
06/01/2023 02:04, Tyler Retzlaff:
> On Fri, Jan 06, 2023 at 12:10:45AM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Thursday, 5 January 2023 23.06
> > > 
> > > doesn't create a new kind of mess.
> > > 
> > > so i fudged around a bit to see if i could get a happy medium. i ended
> > > up with this.
> > > 
> > > remove include of rte_debug.h from rte_bitops.h
> > > 
> > >   * had to remove the RTE_ASSERT from existing rte_bitops.h functions
> > >   * this breaks a good piece of the cycle debug -> log -> common ->
> > > bitops -> debug
> > >   * deal breaker? i don't think it was right that we were getting all
> > >     of log, common just for using bitops anyway.
> > > 
> > > move pow2 functions from rte_common.h -> rte_pow2ops.h
> > >   * new header includes rte_bitops.h
> > > 
> > > move log2 functions from rte_common.h -> rte_log2ops.h
> > >   * new header includes rte_bitops.h, rte_pow2ops.h
> > > 
> > > include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into
> > > rte_common.h
> > > 
> > >   * this is done to reduce the impact of compatibility break by
> > >     continuing to expose the pow2/log2/bitops via rte_common.h
> > > 
> > > so we end up with 3 standalone headers, where the whole tree builds
> > > without having to add a pile of includes for the new headers. we can
> > > later deprecate the exposure of the inline functions when including
> > > rte_common.h
> > > 
> > >   * one caveat is that there was some contamination coming in via the
> > >     removed rte_debug.h where rte_bitops.h was used. so technically
> > >     a break of api too.
> > > 
> > > objections?
> > > 
> > > if this is no good i'll just fold my new functions into rte_common.h
> > > and
> > > leave the mess for the next person, though i am trying not to do that.
> > > 
> > > thanks for the discussion.
> > 
> > Here's some long term thinking: EAL has grown into a trashcan where too much is thrown in. It should only be a thin shim to abstract the underlying hardware and O/S environment. A step in that direction could be splitting the current EAL into a true EAL and a Utils library. Not now, but perhaps some day in a rosy future.
> > 
> > Your proposal effectively makes rte_common.h even bigger by including rte_bitops.h (which was intended for accessing hardware). I am not sure it is a step in the right direction. On the other hand, introducing yet another header file for bit-mathematical functions is probably worse than adding them to rte_bitops.h.
> > 
> > I can't come up with something good myself, but I lean towards simply adding your functions to rte_common.h and live with the existing mess. If you think your proposal is better, I will not object. I'm only voicing my thoughts.
> 
> it's hard when we are starting with something monolithic and trying to
> split it. you always end up with these iterative transitions toward what
> you want because you have to keep some kind of compat.
> 
> > 
> > @Thomas may have another perspective on the matter.
> 
> Thomas any last word after this back and forth?  This change isn't
> necessarily blocking me but is a distraction i'd like to finish/offload.

I think it is better to move all in rte_bitops.h
rte_common must have almost zero dependency.
If log2 and pow2 are based on bitops operations, it is not a bad fit for rte_bitops.h.
Bruce Richardson Jan. 6, 2023, 11:48 a.m. UTC | #19 
On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> On Thu, 5 Jan 2023 09:21:18 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > 05/01/2023 08:09, Morten Brørup:  
> > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > +/**
> > > > > + * @warning
> > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > notice
> > > > > + *
> > > > > + * Get the count of leading 0-bits in v.
> > > > > + *
> > > > > + * @param v
> > > > > + *   The value.
> > > > > + * @return
> > > > > + *   The count of leading zero bits.
> > > > > + */
> > > > > +__rte_experimental
> > > > > +static inline unsigned int
> > > > > +rte_clzl(unsigned long v)  
> > > > 
> > > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > > 
> > > > E.g.: rte_clz32(uint32_t v)  
> > > 
> > > I agree on using numbers.
> > >   
> > 
> > love the idea, fewer functions too.
> > 
> > though it is a shame we cannot adopt C11 standard because we could just
> > do away with the bit suffixes entirely.
> 
> We could but the project needs to support older RHEL releases
> which have older tool sets. Though probably this is moot point given
> how much meson seems to change.

True, though meson tends to be a bit easier to update than GCC on a system
- no "pip3 install --upgrade gcc", sadly :-)

If we can't go all the way to C11 support, how about at least going to C99
support? As far as I know DPDK has never updated its minimum C-standard
version, and it might be a good idea to start the process of doing so, even
if it is a baby step.

/Bruce
Morten Brørup Jan. 6, 2023, 12:41 p.m. UTC | #20 
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 6 January 2023 12.48
> 
> On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > On Thu, 5 Jan 2023 09:21:18 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > 05/01/2023 08:09, Morten Brørup:
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > +/**
> > > > > > + * @warning
> > > > > > + * @b EXPERIMENTAL: this API may change, or be removed,
> without prior
> > > > > > notice
> > > > > > + *
> > > > > > + * Get the count of leading 0-bits in v.
> > > > > > + *
> > > > > > + * @param v
> > > > > > + *   The value.
> > > > > > + * @return
> > > > > > + *   The count of leading zero bits.
> > > > > > + */
> > > > > > +__rte_experimental
> > > > > > +static inline unsigned int
> > > > > > +rte_clzl(unsigned long v)
> > > > >
> > > > > Don't use l (long) and ll (long long) for names (and types),
> use explicit bit lengths, 32 and 64.
> > > > >
> > > > > E.g.: rte_clz32(uint32_t v)
> > > >
> > > > I agree on using numbers.
> > > >
> > >
> > > love the idea, fewer functions too.
> > >
> > > though it is a shame we cannot adopt C11 standard because we could
> just
> > > do away with the bit suffixes entirely.
> >
> > We could but the project needs to support older RHEL releases
> > which have older tool sets. Though probably this is moot point given
> > how much meson seems to change.
> 
> True, though meson tends to be a bit easier to update than GCC on a
> system
> - no "pip3 install --upgrade gcc", sadly :-)
> 
> If we can't go all the way to C11 support, how about at least going to
> C99
> support? As far as I know DPDK has never updated its minimum C-standard
> version, and it might be a good idea to start the process of doing so,
> even
> if it is a baby step.
> 
> /Bruce

The DPDK Getting Started Guide [1] says:

"Required Tools and Libraries:
[...]
a supported C compiler such as gcc (version 4.9+)"

GCC version 4.9 supports C11 [2]:
"GCC 4.9 Changes: "ISO C11 support is now at a similar level of completeness to ISO C99 support [...]"

So why are we not going to support C11?

Probably because of RHEL 7, which only provides GCC 4.8 [3].

RHEL 7 was released for GA on June 10, 2014 [4]. If someone has a server with a nine year old distro still used in production, it is probably because it is running some legacy application, which is difficult to get up and running on a newer distro. Partial conclusion: RHEL 7 is probably still widely used in production.

However, I have a hard time understanding why anyone would build and/or deploy a brand new DPDK application (based on DPDK 23.03) on such a server. Can someone please justify this?

Are we really going to postpone C11 support in DPDK until June 30, 2026, when RHEL 7 ends its Extended Life Cycle Support [4]?

If so, then the GCC version mentioned in the DPDK Getting Started Guide should be corrected accordingly.

[1]: https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk
[2]: https://gcc.gnu.org/wiki/C11Status
[3]: https://access.redhat.com/solutions/19458
[4]: https://access.redhat.com/support/policy/updates/errata#Life_Cycle_Dates
Thomas Monjalon Jan. 6, 2023, 1:40 p.m. UTC | #21 
06/01/2023 13:41, Morten Brørup:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 6 January 2023 12.48
> > 
> > On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > > On Thu, 5 Jan 2023 09:21:18 -0800
> > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > >
> > > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > > 05/01/2023 08:09, Morten Brørup:
> > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > +/**
> > > > > > > + * @warning
> > > > > > > + * @b EXPERIMENTAL: this API may change, or be removed,
> > without prior
> > > > > > > notice
> > > > > > > + *
> > > > > > > + * Get the count of leading 0-bits in v.
> > > > > > > + *
> > > > > > > + * @param v
> > > > > > > + *   The value.
> > > > > > > + * @return
> > > > > > > + *   The count of leading zero bits.
> > > > > > > + */
> > > > > > > +__rte_experimental
> > > > > > > +static inline unsigned int
> > > > > > > +rte_clzl(unsigned long v)
> > > > > >
> > > > > > Don't use l (long) and ll (long long) for names (and types),
> > use explicit bit lengths, 32 and 64.
> > > > > >
> > > > > > E.g.: rte_clz32(uint32_t v)
> > > > >
> > > > > I agree on using numbers.
> > > > >
> > > >
> > > > love the idea, fewer functions too.
> > > >
> > > > though it is a shame we cannot adopt C11 standard because we could
> > just
> > > > do away with the bit suffixes entirely.
> > >
> > > We could but the project needs to support older RHEL releases
> > > which have older tool sets. Though probably this is moot point given
> > > how much meson seems to change.
> > 
> > True, though meson tends to be a bit easier to update than GCC on a
> > system
> > - no "pip3 install --upgrade gcc", sadly :-)
> > 
> > If we can't go all the way to C11 support, how about at least going to
> > C99
> > support? As far as I know DPDK has never updated its minimum C-standard
> > version, and it might be a good idea to start the process of doing so,
> > even
> > if it is a baby step.

I am in favor of this baby step: define -std=c99 porject-wise
and see what are the effects during the year.


> The DPDK Getting Started Guide [1] says:
> 
> "Required Tools and Libraries:
> [...]
> a supported C compiler such as gcc (version 4.9+)"
> 
> GCC version 4.9 supports C11 [2]:
> "GCC 4.9 Changes: "ISO C11 support is now at a similar level of completeness to ISO C99 support [...]"
> 
> So why are we not going to support C11?

We should make a plan to switch to C11 during next year.


> Probably because of RHEL 7, which only provides GCC 4.8 [3].
> 
> RHEL 7 was released for GA on June 10, 2014 [4]. If someone has a server with a nine year old distro still used in production, it is probably because it is running some legacy application, which is difficult to get up and running on a newer distro. Partial conclusion: RHEL 7 is probably still widely used in production.
> 
> However, I have a hard time understanding why anyone would build and/or deploy a brand new DPDK application (based on DPDK 23.03) on such a server. Can someone please justify this?
> 
> Are we really going to postpone C11 support in DPDK until June 30, 2026, when RHEL 7 ends its Extended Life Cycle Support [4]?

RHEL does its own choices to support old software for long.
Upstream development should move forward.


> If so, then the GCC version mentioned in the DPDK Getting Started Guide should be corrected accordingly.

No let's keep GCC 4.9 as a minimum for now.
If needed we could upgrade it later.
Tyler Retzlaff Jan. 6, 2023, 6:47 p.m. UTC | #22 
On Fri, Jan 06, 2023 at 11:48:17AM +0000, Bruce Richardson wrote:
> On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > On Thu, 5 Jan 2023 09:21:18 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > 
> > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > 05/01/2023 08:09, Morten Brørup:  
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > +/**
> > > > > > + * @warning
> > > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > > notice
> > > > > > + *
> > > > > > + * Get the count of leading 0-bits in v.
> > > > > > + *
> > > > > > + * @param v
> > > > > > + *   The value.
> > > > > > + * @return
> > > > > > + *   The count of leading zero bits.
> > > > > > + */
> > > > > > +__rte_experimental
> > > > > > +static inline unsigned int
> > > > > > +rte_clzl(unsigned long v)  
> > > > > 
> > > > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > > > 
> > > > > E.g.: rte_clz32(uint32_t v)  
> > > > 
> > > > I agree on using numbers.
> > > >   
> > > 
> > > love the idea, fewer functions too.
> > > 
> > > though it is a shame we cannot adopt C11 standard because we could just
> > > do away with the bit suffixes entirely.
> > 
> > We could but the project needs to support older RHEL releases
> > which have older tool sets. Though probably this is moot point given
> > how much meson seems to change.
> 
> True, though meson tends to be a bit easier to update than GCC on a system
> - no "pip3 install --upgrade gcc", sadly :-)

* on linux. :)

> 
> If we can't go all the way to C11 support, how about at least going to C99
> support? As far as I know DPDK has never updated its minimum C-standard
> version, and it might be a good idea to start the process of doing so, even
> if it is a baby step.

the thing that blurs the line a bit is how the gcc version that is
holding us back does actually allow the use of some C99 optional
features. for example we use the C99 fixed width integer types so
technically some of the code already requires C99.

i also notice at least one driver is explicitly specifying -std=gnu99 so
maybe that driver just isn't being built when the old gcc is detected?

anyway, i think we are stuck pre-c99 so long as we have RHEL 7 to
contend with. the rationale is if we could use a compiler conforming to
the new standard we could just directly use those features, but so long
as we have to support non conforming compiler at a particular level we
have to introduce an abstraction and that is where all the extra work
comes from.

a prominent example is atomics from C11, but for other reasons in our
code base that i won't go into even if we required C11 we would still
need to abstract atomics. fwiw i'm working on this patch series now and
hope to provide a first draft in the next week or two.

finally, i do think it would be good to document a minimum C compiler
conformance level rather than specific gcc versions. though i admit we
also have a hard dependency on gcc right now. my goal is to improve
portability to a level where we could just state "you need a C<N>
compliant compiler" (or as close to it as possible).

ty
Tyler Retzlaff Jan. 6, 2023, 6:58 p.m. UTC | #23 
On Fri, Jan 06, 2023 at 02:40:59PM +0100, Thomas Monjalon wrote:
> 06/01/2023 13:41, Morten Brørup:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Friday, 6 January 2023 12.48
> > > 
> > > On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > > > On Thu, 5 Jan 2023 09:21:18 -0800
> > > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > >
> > > > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > > > 05/01/2023 08:09, Morten Brørup:
> > > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > > +/**
> > > > > > > > + * @warning
> > > > > > > > + * @b EXPERIMENTAL: this API may change, or be removed,
> > > without prior
> > > > > > > > notice
> > > > > > > > + *
> > > > > > > > + * Get the count of leading 0-bits in v.
> > > > > > > > + *
> > > > > > > > + * @param v
> > > > > > > > + *   The value.
> > > > > > > > + * @return
> > > > > > > > + *   The count of leading zero bits.
> > > > > > > > + */
> > > > > > > > +__rte_experimental
> > > > > > > > +static inline unsigned int
> > > > > > > > +rte_clzl(unsigned long v)
> > > > > > >
> > > > > > > Don't use l (long) and ll (long long) for names (and types),
> > > use explicit bit lengths, 32 and 64.
> > > > > > >
> > > > > > > E.g.: rte_clz32(uint32_t v)
> > > > > >
> > > > > > I agree on using numbers.
> > > > > >
> > > > >
> > > > > love the idea, fewer functions too.
> > > > >
> > > > > though it is a shame we cannot adopt C11 standard because we could
> > > just
> > > > > do away with the bit suffixes entirely.
> > > >
> > > > We could but the project needs to support older RHEL releases
> > > > which have older tool sets. Though probably this is moot point given
> > > > how much meson seems to change.
> > > 
> > > True, though meson tends to be a bit easier to update than GCC on a
> > > system
> > > - no "pip3 install --upgrade gcc", sadly :-)
> > > 
> > > If we can't go all the way to C11 support, how about at least going to
> > > C99
> > > support? As far as I know DPDK has never updated its minimum C-standard
> > > version, and it might be a good idea to start the process of doing so,
> > > even
> > > if it is a baby step.
> 
> I am in favor of this baby step: define -std=c99 porject-wise
> and see what are the effects during the year.
> 
> 
> > The DPDK Getting Started Guide [1] says:
> > 
> > "Required Tools and Libraries:
> > [...]
> > a supported C compiler such as gcc (version 4.9+)"
> > 
> > GCC version 4.9 supports C11 [2]:
> > "GCC 4.9 Changes: "ISO C11 support is now at a similar level of completeness to ISO C99 support [...]"
> > 
> > So why are we not going to support C11?
> 
> We should make a plan to switch to C11 during next year.
> 
> 
> > Probably because of RHEL 7, which only provides GCC 4.8 [3].
> > 
> > RHEL 7 was released for GA on June 10, 2014 [4]. If someone has a server with a nine year old distro still used in production, it is probably because it is running some legacy application, which is difficult to get up and running on a newer distro. Partial conclusion: RHEL 7 is probably still widely used in production.
> > 
> > However, I have a hard time understanding why anyone would build and/or deploy a brand new DPDK application (based on DPDK 23.03) on such a server. Can someone please justify this?
> > 
> > Are we really going to postpone C11 support in DPDK until June 30, 2026, when RHEL 7 ends its Extended Life Cycle Support [4]?
> 
> RHEL does its own choices to support old software for long.
> Upstream development should move forward.
> 
> 
> > If so, then the GCC version mentioned in the DPDK Getting Started Guide should be corrected accordingly.
> 
> No let's keep GCC 4.9 as a minimum for now.
> If needed we could upgrade it later.

but i think the point Morten is making is that RHEL 7 gcc is 4.8 and
therefore we implicitly no longer support it if we document requiring
gcc 4.9.

> 
> 

so i think the way to do this is through clarification at the next
release / ltsc release. starting with dpdk 23.xx will require
compiler conforming to standard X and optional features / annexes from
standard X. anyone building applications targeting that version of dpdk
release will need a conforming implementation.

from there we just need to take care not to backport any code to
stable branches that depend on standard features that exceed the
requirements documented for that release.
Thomas Monjalon Jan. 6, 2023, 8:51 p.m. UTC | #24 
06/01/2023 19:58, Tyler Retzlaff:
> On Fri, Jan 06, 2023 at 02:40:59PM +0100, Thomas Monjalon wrote:
> > 06/01/2023 13:41, Morten Brørup:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Friday, 6 January 2023 12.48
> > > > 
> > > > On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > > > > On Thu, 5 Jan 2023 09:21:18 -0800
> > > > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > > >
> > > > > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > > > > 05/01/2023 08:09, Morten Brørup:
> > > > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > > > +/**
> > > > > > > > > + * @warning
> > > > > > > > > + * @b EXPERIMENTAL: this API may change, or be removed,
> > > > without prior
> > > > > > > > > notice
> > > > > > > > > + *
> > > > > > > > > + * Get the count of leading 0-bits in v.
> > > > > > > > > + *
> > > > > > > > > + * @param v
> > > > > > > > > + *   The value.
> > > > > > > > > + * @return
> > > > > > > > > + *   The count of leading zero bits.
> > > > > > > > > + */
> > > > > > > > > +__rte_experimental
> > > > > > > > > +static inline unsigned int
> > > > > > > > > +rte_clzl(unsigned long v)
> > > > > > > >
> > > > > > > > Don't use l (long) and ll (long long) for names (and types),
> > > > use explicit bit lengths, 32 and 64.
> > > > > > > >
> > > > > > > > E.g.: rte_clz32(uint32_t v)
> > > > > > >
> > > > > > > I agree on using numbers.
> > > > > > >
> > > > > >
> > > > > > love the idea, fewer functions too.
> > > > > >
> > > > > > though it is a shame we cannot adopt C11 standard because we could
> > > > just
> > > > > > do away with the bit suffixes entirely.
> > > > >
> > > > > We could but the project needs to support older RHEL releases
> > > > > which have older tool sets. Though probably this is moot point given
> > > > > how much meson seems to change.
> > > > 
> > > > True, though meson tends to be a bit easier to update than GCC on a
> > > > system
> > > > - no "pip3 install --upgrade gcc", sadly :-)
> > > > 
> > > > If we can't go all the way to C11 support, how about at least going to
> > > > C99
> > > > support? As far as I know DPDK has never updated its minimum C-standard
> > > > version, and it might be a good idea to start the process of doing so,
> > > > even
> > > > if it is a baby step.
> > 
> > I am in favor of this baby step: define -std=c99 porject-wise
> > and see what are the effects during the year.
> > 
> > 
> > > The DPDK Getting Started Guide [1] says:
> > > 
> > > "Required Tools and Libraries:
> > > [...]
> > > a supported C compiler such as gcc (version 4.9+)"
> > > 
> > > GCC version 4.9 supports C11 [2]:
> > > "GCC 4.9 Changes: "ISO C11 support is now at a similar level of completeness to ISO C99 support [...]"
> > > 
> > > So why are we not going to support C11?
> > 
> > We should make a plan to switch to C11 during next year.
> > 
> > 
> > > Probably because of RHEL 7, which only provides GCC 4.8 [3].
> > > 
> > > RHEL 7 was released for GA on June 10, 2014 [4]. If someone has a server with a nine year old distro still used in production, it is probably because it is running some legacy application, which is difficult to get up and running on a newer distro. Partial conclusion: RHEL 7 is probably still widely used in production.
> > > 
> > > However, I have a hard time understanding why anyone would build and/or deploy a brand new DPDK application (based on DPDK 23.03) on such a server. Can someone please justify this?
> > > 
> > > Are we really going to postpone C11 support in DPDK until June 30, 2026, when RHEL 7 ends its Extended Life Cycle Support [4]?
> > 
> > RHEL does its own choices to support old software for long.
> > Upstream development should move forward.
> > 
> > 
> > > If so, then the GCC version mentioned in the DPDK Getting Started Guide should be corrected accordingly.
> > 
> > No let's keep GCC 4.9 as a minimum for now.
> > If needed we could upgrade it later.
> 
> but i think the point Morten is making is that RHEL 7 gcc is 4.8 and
> therefore we implicitly no longer support it if we document requiring
> gcc 4.9.

Yes I got it.
I think everything in DPDK works on RHEL 7 today,
but I believe RHEL 7 is not a strong requirement anymore for the mainline.
Asking for confirmation here.

> so i think the way to do this is through clarification at the next
> release / ltsc release. starting with dpdk 23.xx will require
> compiler conforming to standard X and optional features / annexes from
> standard X. anyone building applications targeting that version of dpdk
> release will need a conforming implementation.
> 
> from there we just need to take care not to backport any code to
> stable branches that depend on standard features that exceed the
> requirements documented for that release.

We could even start testing C99 requirement in 23.03 I think.
Bruce Richardson Jan. 9, 2023, 8:50 a.m. UTC | #25 
On Fri, Jan 06, 2023 at 10:47:06AM -0800, Tyler Retzlaff wrote:
> On Fri, Jan 06, 2023 at 11:48:17AM +0000, Bruce Richardson wrote:
> > On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
> > > On Thu, 5 Jan 2023 09:21:18 -0800
> > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > 
> > > > On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
> > > > > 05/01/2023 08:09, Morten Brørup:  
> > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > +/**
> > > > > > > + * @warning
> > > > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > > > notice
> > > > > > > + *
> > > > > > > + * Get the count of leading 0-bits in v.
> > > > > > > + *
> > > > > > > + * @param v
> > > > > > > + *   The value.
> > > > > > > + * @return
> > > > > > > + *   The count of leading zero bits.
> > > > > > > + */
> > > > > > > +__rte_experimental
> > > > > > > +static inline unsigned int
> > > > > > > +rte_clzl(unsigned long v)  
> > > > > > 
> > > > > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > > > > 
> > > > > > E.g.: rte_clz32(uint32_t v)  
> > > > > 
> > > > > I agree on using numbers.
> > > > >   
> > > > 
> > > > love the idea, fewer functions too.
> > > > 
> > > > though it is a shame we cannot adopt C11 standard because we could just
> > > > do away with the bit suffixes entirely.
> > > 
> > > We could but the project needs to support older RHEL releases
> > > which have older tool sets. Though probably this is moot point given
> > > how much meson seems to change.
> > 
> > True, though meson tends to be a bit easier to update than GCC on a system
> > - no "pip3 install --upgrade gcc", sadly :-)
> 
> * on linux. :)
> 
> > 
> > If we can't go all the way to C11 support, how about at least going to C99
> > support? As far as I know DPDK has never updated its minimum C-standard
> > version, and it might be a good idea to start the process of doing so, even
> > if it is a baby step.
> 
> the thing that blurs the line a bit is how the gcc version that is
> holding us back does actually allow the use of some C99 optional
> features. for example we use the C99 fixed width integer types so
> technically some of the code already requires C99.
> 
> i also notice at least one driver is explicitly specifying -std=gnu99 so
> maybe that driver just isn't being built when the old gcc is detected?
> 
> anyway, i think we are stuck pre-c99 so long as we have RHEL 7 to
> contend with. the rationale is if we could use a compiler conforming to
> the new standard we could just directly use those features, but so long
> as we have to support non conforming compiler at a particular level we
> have to introduce an abstraction and that is where all the extra work
> comes from.
> 

AFAIK the gcc 4.8 compiler on RHEL 7/Centos 7 does support C99, it's just
missing full support for C11 (which comes in 4.9). Therefore I see using
C99 as hopefully being unproblematic, and allowing us to try moving forward
revision and seeing what issues - if any - we hit. It should also allow us
to put in place infrastructure to support such version changes, since, for
example, we likely need to ensure that our headers still work with C90
compilation.

/Bruce
Ferruh Yigit Jan. 10, 2023, 9:18 a.m. UTC | #26 
On 1/6/2023 8:51 PM, Thomas Monjalon wrote:
> 06/01/2023 19:58, Tyler Retzlaff:
>> On Fri, Jan 06, 2023 at 02:40:59PM +0100, Thomas Monjalon wrote:
>>> 06/01/2023 13:41, Morten Brørup:
>>>>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
>>>>> Sent: Friday, 6 January 2023 12.48
>>>>>
>>>>> On Thu, Jan 05, 2023 at 04:32:40PM -0800, Stephen Hemminger wrote:
>>>>>> On Thu, 5 Jan 2023 09:21:18 -0800
>>>>>> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
>>>>>>
>>>>>>> On Thu, Jan 05, 2023 at 10:01:31AM +0100, Thomas Monjalon wrote:
>>>>>>>> 05/01/2023 08:09, Morten Brørup:
>>>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>>>>>>>> +/**
>>>>>>>>>> + * @warning
>>>>>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed,
>>>>> without prior
>>>>>>>>>> notice
>>>>>>>>>> + *
>>>>>>>>>> + * Get the count of leading 0-bits in v.
>>>>>>>>>> + *
>>>>>>>>>> + * @param v
>>>>>>>>>> + *   The value.
>>>>>>>>>> + * @return
>>>>>>>>>> + *   The count of leading zero bits.
>>>>>>>>>> + */
>>>>>>>>>> +__rte_experimental
>>>>>>>>>> +static inline unsigned int
>>>>>>>>>> +rte_clzl(unsigned long v)
>>>>>>>>>
>>>>>>>>> Don't use l (long) and ll (long long) for names (and types),
>>>>> use explicit bit lengths, 32 and 64.
>>>>>>>>>
>>>>>>>>> E.g.: rte_clz32(uint32_t v)
>>>>>>>>
>>>>>>>> I agree on using numbers.
>>>>>>>>
>>>>>>>
>>>>>>> love the idea, fewer functions too.
>>>>>>>
>>>>>>> though it is a shame we cannot adopt C11 standard because we could
>>>>> just
>>>>>>> do away with the bit suffixes entirely.
>>>>>>
>>>>>> We could but the project needs to support older RHEL releases
>>>>>> which have older tool sets. Though probably this is moot point given
>>>>>> how much meson seems to change.
>>>>>
>>>>> True, though meson tends to be a bit easier to update than GCC on a
>>>>> system
>>>>> - no "pip3 install --upgrade gcc", sadly :-)
>>>>>
>>>>> If we can't go all the way to C11 support, how about at least going to
>>>>> C99
>>>>> support? As far as I know DPDK has never updated its minimum C-standard
>>>>> version, and it might be a good idea to start the process of doing so,
>>>>> even
>>>>> if it is a baby step.
>>>
>>> I am in favor of this baby step: define -std=c99 porject-wise
>>> and see what are the effects during the year.
>>>
>>>
>>>> The DPDK Getting Started Guide [1] says:
>>>>
>>>> "Required Tools and Libraries:
>>>> [...]
>>>> a supported C compiler such as gcc (version 4.9+)"
>>>>
>>>> GCC version 4.9 supports C11 [2]:
>>>> "GCC 4.9 Changes: "ISO C11 support is now at a similar level of completeness to ISO C99 support [...]"
>>>>
>>>> So why are we not going to support C11?
>>>
>>> We should make a plan to switch to C11 during next year.
>>>
>>>
>>>> Probably because of RHEL 7, which only provides GCC 4.8 [3].
>>>>
>>>> RHEL 7 was released for GA on June 10, 2014 [4]. If someone has a server with a nine year old distro still used in production, it is probably because it is running some legacy application, which is difficult to get up and running on a newer distro. Partial conclusion: RHEL 7 is probably still widely used in production.
>>>>
>>>> However, I have a hard time understanding why anyone would build and/or deploy a brand new DPDK application (based on DPDK 23.03) on such a server. Can someone please justify this?
>>>>
>>>> Are we really going to postpone C11 support in DPDK until June 30, 2026, when RHEL 7 ends its Extended Life Cycle Support [4]?
>>>
>>> RHEL does its own choices to support old software for long.
>>> Upstream development should move forward.
>>>
>>>
>>>> If so, then the GCC version mentioned in the DPDK Getting Started Guide should be corrected accordingly.
>>>
>>> No let's keep GCC 4.9 as a minimum for now.
>>> If needed we could upgrade it later.
>>
>> but i think the point Morten is making is that RHEL 7 gcc is 4.8 and
>> therefore we implicitly no longer support it if we document requiring
>> gcc 4.9.
> 
> Yes I got it.
> I think everything in DPDK works on RHEL 7 today,
> but I believe RHEL 7 is not a strong requirement anymore for the mainline.
> Asking for confirmation here.
> 
>> so i think the way to do this is through clarification at the next
>> release / ltsc release. starting with dpdk 23.xx will require
>> compiler conforming to standard X and optional features / annexes from
>> standard X. anyone building applications targeting that version of dpdk
>> release will need a conforming implementation.
>>
>> from there we just need to take care not to backport any code to
>> stable branches that depend on standard features that exceed the
>> requirements documented for that release.
> 
> We could even start testing C99 requirement in 23.03 I think.
> 
> 

I missed discussion in this thread, but replied in other thread,
replying here too.


+1 to switch C99 support.
There are bunch of C99 features used within DPDK already, and there were
some drivers compiled with c89 support, that restriction is removed now,
so it should be OK to move to C99 support.
Tyler Retzlaff April 4, 2023, 9:23 p.m. UTC | #27 
On Thu, Jan 05, 2023 at 08:09:19AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 24 November 2022 00.43
> > 
> > Provide an abstraction for leading and trailing zero bit counting
> > functions to hide compiler specific intrinsics and builtins.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/include/meson.build    |   1 +
> >  lib/eal/include/rte_bitcount.h | 265
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 266 insertions(+)
> >  create mode 100644 lib/eal/include/rte_bitcount.h
> > 
> > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> > index cfcd40a..8ff1d65 100644
> > --- a/lib/eal/include/meson.build
> > +++ b/lib/eal/include/meson.build
> > @@ -5,6 +5,7 @@ includes += include_directories('.')
> > 
> >  headers += files(
> >          'rte_alarm.h',
> > +        'rte_bitcount.h',
> >          'rte_bitmap.h',
> >          'rte_bitops.h',
> >          'rte_branch_prediction.h',
> > diff --git a/lib/eal/include/rte_bitcount.h
> > b/lib/eal/include/rte_bitcount.h
> > new file mode 100644
> > index 0000000..587de52
> > --- /dev/null
> > +++ b/lib/eal/include/rte_bitcount.h
> > @@ -0,0 +1,265 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (C) 2022 Microsoft Corporation
> > + */
> > +
> > +#ifndef _RTE_BITCOUNT_H_
> > +#define _RTE_BITCOUNT_H_
> > +
> > +#include <rte_compat.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#ifdef RTE_TOOLCHAIN_MSVC
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Get the count of leading 0-bits in v.
> > + *
> > + * @param v
> > + *   The value.
> > + * @return
> > + *   The count of leading zero bits.
> > + */
> > +__rte_experimental
> > +static inline unsigned int
> > +rte_clz(unsigned int v)
> > +{
> > +	unsigned long rv;
> > +
> > +	(void)_BitScanReverse(&rv, v);
> > +
> > +	return (unsigned int)rv;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Get the count of leading 0-bits in v.
> > + *
> > + * @param v
> > + *   The value.
> > + * @return
> > + *   The count of leading zero bits.
> > + */
> > +__rte_experimental
> > +static inline unsigned int
> > +rte_clzl(unsigned long v)
> 
> Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> 
> E.g.: rte_clz32(uint32_t v)

so i just noticed this, but sometimes these functions receive size_t so
naming them specifically 32/64 bit becomes problematic because are going
to end up with promotion on sizeof(size_t) == sizeof(long) == 4
platforms.

i.e.
size_t s = ...;
x = rte_clz64(s); // assume 64-bit today

this code is now broken because on 32-bit platform s will get promoted
and the extra 32 zero-bits will be returned in the result breaking
calculations.

any thoughts? should we go back to l, ll?

ty
Bruce Richardson April 5, 2023, 8:44 a.m. UTC | #28 
On Tue, Apr 04, 2023 at 02:23:22PM -0700, Tyler Retzlaff wrote:
> On Thu, Jan 05, 2023 at 08:09:19AM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Thursday, 24 November 2022 00.43
> > > 
> > > Provide an abstraction for leading and trailing zero bit counting
> > > functions to hide compiler specific intrinsics and builtins.
> > > 
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/eal/include/meson.build    |   1 +
> > >  lib/eal/include/rte_bitcount.h | 265
> > > +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 266 insertions(+)
> > >  create mode 100644 lib/eal/include/rte_bitcount.h
> > > 
> > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> > > index cfcd40a..8ff1d65 100644
> > > --- a/lib/eal/include/meson.build
> > > +++ b/lib/eal/include/meson.build
> > > @@ -5,6 +5,7 @@ includes += include_directories('.')
> > > 
> > >  headers += files(
> > >          'rte_alarm.h',
> > > +        'rte_bitcount.h',
> > >          'rte_bitmap.h',
> > >          'rte_bitops.h',
> > >          'rte_branch_prediction.h',
> > > diff --git a/lib/eal/include/rte_bitcount.h
> > > b/lib/eal/include/rte_bitcount.h
> > > new file mode 100644
> > > index 0000000..587de52
> > > --- /dev/null
> > > +++ b/lib/eal/include/rte_bitcount.h
> > > @@ -0,0 +1,265 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright (C) 2022 Microsoft Corporation
> > > + */
> > > +
> > > +#ifndef _RTE_BITCOUNT_H_
> > > +#define _RTE_BITCOUNT_H_
> > > +
> > > +#include <rte_compat.h>
> > > +
> > > +#ifdef __cplusplus
> > > +extern "C" {
> > > +#endif
> > > +
> > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > +
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > notice
> > > + *
> > > + * Get the count of leading 0-bits in v.
> > > + *
> > > + * @param v
> > > + *   The value.
> > > + * @return
> > > + *   The count of leading zero bits.
> > > + */
> > > +__rte_experimental
> > > +static inline unsigned int
> > > +rte_clz(unsigned int v)
> > > +{
> > > +	unsigned long rv;
> > > +
> > > +	(void)_BitScanReverse(&rv, v);
> > > +
> > > +	return (unsigned int)rv;
> > > +}
> > > +
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > notice
> > > + *
> > > + * Get the count of leading 0-bits in v.
> > > + *
> > > + * @param v
> > > + *   The value.
> > > + * @return
> > > + *   The count of leading zero bits.
> > > + */
> > > +__rte_experimental
> > > +static inline unsigned int
> > > +rte_clzl(unsigned long v)
> > 
> > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > 
> > E.g.: rte_clz32(uint32_t v)
> 
> so i just noticed this, but sometimes these functions receive size_t so
> naming them specifically 32/64 bit becomes problematic because are going
> to end up with promotion on sizeof(size_t) == sizeof(long) == 4
> platforms.
> 
> i.e.
> size_t s = ...;
> x = rte_clz64(s); // assume 64-bit today
> 
> this code is now broken because on 32-bit platform s will get promoted
> and the extra 32 zero-bits will be returned in the result breaking
> calculations.
> 
> any thoughts? should we go back to l, ll?
> 

Yes, promotion will happen, but I still think that the 32 and 64 versions
are far clearer here in all cases. Anyone looking at the code will
recognise that the result will be the leading zero count of a 64-bit number
irrespective of the type actually passed in. It's less confusing now IMHO.

/Bruce
Tyler Retzlaff April 5, 2023, 3:22 p.m. UTC | #29 
On Wed, Apr 05, 2023 at 09:44:54AM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 02:23:22PM -0700, Tyler Retzlaff wrote:
> > On Thu, Jan 05, 2023 at 08:09:19AM +0100, Morten Brørup wrote:
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > Sent: Thursday, 24 November 2022 00.43
> > > > 
> > > > Provide an abstraction for leading and trailing zero bit counting
> > > > functions to hide compiler specific intrinsics and builtins.
> > > > 
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > >  lib/eal/include/meson.build    |   1 +
> > > >  lib/eal/include/rte_bitcount.h | 265
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 266 insertions(+)
> > > >  create mode 100644 lib/eal/include/rte_bitcount.h
> > > > 
> > > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> > > > index cfcd40a..8ff1d65 100644
> > > > --- a/lib/eal/include/meson.build
> > > > +++ b/lib/eal/include/meson.build
> > > > @@ -5,6 +5,7 @@ includes += include_directories('.')
> > > > 
> > > >  headers += files(
> > > >          'rte_alarm.h',
> > > > +        'rte_bitcount.h',
> > > >          'rte_bitmap.h',
> > > >          'rte_bitops.h',
> > > >          'rte_branch_prediction.h',
> > > > diff --git a/lib/eal/include/rte_bitcount.h
> > > > b/lib/eal/include/rte_bitcount.h
> > > > new file mode 100644
> > > > index 0000000..587de52
> > > > --- /dev/null
> > > > +++ b/lib/eal/include/rte_bitcount.h
> > > > @@ -0,0 +1,265 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright (C) 2022 Microsoft Corporation
> > > > + */
> > > > +
> > > > +#ifndef _RTE_BITCOUNT_H_
> > > > +#define _RTE_BITCOUNT_H_
> > > > +
> > > > +#include <rte_compat.h>
> > > > +
> > > > +#ifdef __cplusplus
> > > > +extern "C" {
> > > > +#endif
> > > > +
> > > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > > +
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > notice
> > > > + *
> > > > + * Get the count of leading 0-bits in v.
> > > > + *
> > > > + * @param v
> > > > + *   The value.
> > > > + * @return
> > > > + *   The count of leading zero bits.
> > > > + */
> > > > +__rte_experimental
> > > > +static inline unsigned int
> > > > +rte_clz(unsigned int v)
> > > > +{
> > > > +	unsigned long rv;
> > > > +
> > > > +	(void)_BitScanReverse(&rv, v);
> > > > +
> > > > +	return (unsigned int)rv;
> > > > +}
> > > > +
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > notice
> > > > + *
> > > > + * Get the count of leading 0-bits in v.
> > > > + *
> > > > + * @param v
> > > > + *   The value.
> > > > + * @return
> > > > + *   The count of leading zero bits.
> > > > + */
> > > > +__rte_experimental
> > > > +static inline unsigned int
> > > > +rte_clzl(unsigned long v)
> > > 
> > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > 
> > > E.g.: rte_clz32(uint32_t v)
> > 
> > so i just noticed this, but sometimes these functions receive size_t so
> > naming them specifically 32/64 bit becomes problematic because are going
> > to end up with promotion on sizeof(size_t) == sizeof(long) == 4
> > platforms.
> > 
> > i.e.
> > size_t s = ...;
> > x = rte_clz64(s); // assume 64-bit today
> > 
> > this code is now broken because on 32-bit platform s will get promoted
> > and the extra 32 zero-bits will be returned in the result breaking
> > calculations.
> > 
> > any thoughts? should we go back to l, ll?
> > 
> 
> Yes, promotion will happen, but I still think that the 32 and 64 versions
> are far clearer here in all cases. Anyone looking at the code will
> recognise that the result will be the leading zero count of a 64-bit number
> irrespective of the type actually passed in. It's less confusing now IMHO.

here's an example in the code that would result in a bad calculation or
at least i believe so at a glance. switching to rte_clz32() would break
on 64-bit since it would truncate.

lib/eal/common/malloc_elem.c

-log2 = sizeof(size) * 8 - __builtin_clzl(size);
+log2 = sizeof(size) * 8 - rte_clz64(size);

if i'm right you'd have to conditionally compile at the site.

#ifdef 64-bit
rte_clz64()
#else
rte_clz32()
#endif

and that seems very undesirable. another solution is to defer this
change until post 23.07 release (where C11 can be used) and we could
then just provide a single generic.

with C11 i can provide a single macro that doesn't need 8/16/32/64
suffix.

size_t v;
n = rte_clz(v); // sizeof(v) doesn't matter.

> 
> /Bruce
Bruce Richardson April 5, 2023, 3:51 p.m. UTC | #30 
On Wed, Apr 05, 2023 at 08:22:16AM -0700, Tyler Retzlaff wrote:
> On Wed, Apr 05, 2023 at 09:44:54AM +0100, Bruce Richardson wrote:
> > On Tue, Apr 04, 2023 at 02:23:22PM -0700, Tyler Retzlaff wrote:
> > > On Thu, Jan 05, 2023 at 08:09:19AM +0100, Morten Brørup wrote:
> > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > Sent: Thursday, 24 November 2022 00.43
> > > > > 
> > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > functions to hide compiler specific intrinsics and builtins.
> > > > > 
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > ---
> > > > >  lib/eal/include/meson.build    |   1 +
> > > > >  lib/eal/include/rte_bitcount.h | 265
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 266 insertions(+)
> > > > >  create mode 100644 lib/eal/include/rte_bitcount.h
> > > > > 
> > > > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> > > > > index cfcd40a..8ff1d65 100644
> > > > > --- a/lib/eal/include/meson.build
> > > > > +++ b/lib/eal/include/meson.build
> > > > > @@ -5,6 +5,7 @@ includes += include_directories('.')
> > > > > 
> > > > >  headers += files(
> > > > >          'rte_alarm.h',
> > > > > +        'rte_bitcount.h',
> > > > >          'rte_bitmap.h',
> > > > >          'rte_bitops.h',
> > > > >          'rte_branch_prediction.h',
> > > > > diff --git a/lib/eal/include/rte_bitcount.h
> > > > > b/lib/eal/include/rte_bitcount.h
> > > > > new file mode 100644
> > > > > index 0000000..587de52
> > > > > --- /dev/null
> > > > > +++ b/lib/eal/include/rte_bitcount.h
> > > > > @@ -0,0 +1,265 @@
> > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > + * Copyright (C) 2022 Microsoft Corporation
> > > > > + */
> > > > > +
> > > > > +#ifndef _RTE_BITCOUNT_H_
> > > > > +#define _RTE_BITCOUNT_H_
> > > > > +
> > > > > +#include <rte_compat.h>
> > > > > +
> > > > > +#ifdef __cplusplus
> > > > > +extern "C" {
> > > > > +#endif
> > > > > +
> > > > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > > > +
> > > > > +/**
> > > > > + * @warning
> > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > notice
> > > > > + *
> > > > > + * Get the count of leading 0-bits in v.
> > > > > + *
> > > > > + * @param v
> > > > > + *   The value.
> > > > > + * @return
> > > > > + *   The count of leading zero bits.
> > > > > + */
> > > > > +__rte_experimental
> > > > > +static inline unsigned int
> > > > > +rte_clz(unsigned int v)
> > > > > +{
> > > > > +	unsigned long rv;
> > > > > +
> > > > > +	(void)_BitScanReverse(&rv, v);
> > > > > +
> > > > > +	return (unsigned int)rv;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * @warning
> > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > notice
> > > > > + *
> > > > > + * Get the count of leading 0-bits in v.
> > > > > + *
> > > > > + * @param v
> > > > > + *   The value.
> > > > > + * @return
> > > > > + *   The count of leading zero bits.
> > > > > + */
> > > > > +__rte_experimental
> > > > > +static inline unsigned int
> > > > > +rte_clzl(unsigned long v)
> > > > 
> > > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > > 
> > > > E.g.: rte_clz32(uint32_t v)
> > > 
> > > so i just noticed this, but sometimes these functions receive size_t so
> > > naming them specifically 32/64 bit becomes problematic because are going
> > > to end up with promotion on sizeof(size_t) == sizeof(long) == 4
> > > platforms.
> > > 
> > > i.e.
> > > size_t s = ...;
> > > x = rte_clz64(s); // assume 64-bit today
> > > 
> > > this code is now broken because on 32-bit platform s will get promoted
> > > and the extra 32 zero-bits will be returned in the result breaking
> > > calculations.
> > > 
> > > any thoughts? should we go back to l, ll?
> > > 
> > 
> > Yes, promotion will happen, but I still think that the 32 and 64 versions
> > are far clearer here in all cases. Anyone looking at the code will
> > recognise that the result will be the leading zero count of a 64-bit number
> > irrespective of the type actually passed in. It's less confusing now IMHO.
> 
> here's an example in the code that would result in a bad calculation or
> at least i believe so at a glance. switching to rte_clz32() would break
> on 64-bit since it would truncate.
> 
> lib/eal/common/malloc_elem.c
> 
> -log2 = sizeof(size) * 8 - __builtin_clzl(size);
> +log2 = sizeof(size) * 8 - rte_clz64(size);
> 
> if i'm right you'd have to conditionally compile at the site.
> 
> #ifdef 64-bit
> rte_clz64()
> #else
> rte_clz32()
> #endif
> 

Why can clz64 not be used in both 32 and 64 bit cases? You know the result
will always include zeros in high bits of a 64-bit value, and the result
will still fit inside even an 8-bit variable?

/Bruce
Tyler Retzlaff April 5, 2023, 5:25 p.m. UTC | #31 
On Wed, Apr 05, 2023 at 04:51:42PM +0100, Bruce Richardson wrote:
> On Wed, Apr 05, 2023 at 08:22:16AM -0700, Tyler Retzlaff wrote:
> > On Wed, Apr 05, 2023 at 09:44:54AM +0100, Bruce Richardson wrote:
> > > On Tue, Apr 04, 2023 at 02:23:22PM -0700, Tyler Retzlaff wrote:
> > > > On Thu, Jan 05, 2023 at 08:09:19AM +0100, Morten Brørup wrote:
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > Sent: Thursday, 24 November 2022 00.43
> > > > > > 
> > > > > > Provide an abstraction for leading and trailing zero bit counting
> > > > > > functions to hide compiler specific intrinsics and builtins.
> > > > > > 
> > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > ---
> > > > > >  lib/eal/include/meson.build    |   1 +
> > > > > >  lib/eal/include/rte_bitcount.h | 265
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 266 insertions(+)
> > > > > >  create mode 100644 lib/eal/include/rte_bitcount.h
> > > > > > 
> > > > > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> > > > > > index cfcd40a..8ff1d65 100644
> > > > > > --- a/lib/eal/include/meson.build
> > > > > > +++ b/lib/eal/include/meson.build
> > > > > > @@ -5,6 +5,7 @@ includes += include_directories('.')
> > > > > > 
> > > > > >  headers += files(
> > > > > >          'rte_alarm.h',
> > > > > > +        'rte_bitcount.h',
> > > > > >          'rte_bitmap.h',
> > > > > >          'rte_bitops.h',
> > > > > >          'rte_branch_prediction.h',
> > > > > > diff --git a/lib/eal/include/rte_bitcount.h
> > > > > > b/lib/eal/include/rte_bitcount.h
> > > > > > new file mode 100644
> > > > > > index 0000000..587de52
> > > > > > --- /dev/null
> > > > > > +++ b/lib/eal/include/rte_bitcount.h
> > > > > > @@ -0,0 +1,265 @@
> > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > + * Copyright (C) 2022 Microsoft Corporation
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef _RTE_BITCOUNT_H_
> > > > > > +#define _RTE_BITCOUNT_H_
> > > > > > +
> > > > > > +#include <rte_compat.h>
> > > > > > +
> > > > > > +#ifdef __cplusplus
> > > > > > +extern "C" {
> > > > > > +#endif
> > > > > > +
> > > > > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > > > > +
> > > > > > +/**
> > > > > > + * @warning
> > > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > > notice
> > > > > > + *
> > > > > > + * Get the count of leading 0-bits in v.
> > > > > > + *
> > > > > > + * @param v
> > > > > > + *   The value.
> > > > > > + * @return
> > > > > > + *   The count of leading zero bits.
> > > > > > + */
> > > > > > +__rte_experimental
> > > > > > +static inline unsigned int
> > > > > > +rte_clz(unsigned int v)
> > > > > > +{
> > > > > > +	unsigned long rv;
> > > > > > +
> > > > > > +	(void)_BitScanReverse(&rv, v);
> > > > > > +
> > > > > > +	return (unsigned int)rv;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * @warning
> > > > > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > > > > > notice
> > > > > > + *
> > > > > > + * Get the count of leading 0-bits in v.
> > > > > > + *
> > > > > > + * @param v
> > > > > > + *   The value.
> > > > > > + * @return
> > > > > > + *   The count of leading zero bits.
> > > > > > + */
> > > > > > +__rte_experimental
> > > > > > +static inline unsigned int
> > > > > > +rte_clzl(unsigned long v)
> > > > > 
> > > > > Don't use l (long) and ll (long long) for names (and types), use explicit bit lengths, 32 and 64.
> > > > > 
> > > > > E.g.: rte_clz32(uint32_t v)
> > > > 
> > > > so i just noticed this, but sometimes these functions receive size_t so
> > > > naming them specifically 32/64 bit becomes problematic because are going
> > > > to end up with promotion on sizeof(size_t) == sizeof(long) == 4
> > > > platforms.
> > > > 
> > > > i.e.
> > > > size_t s = ...;
> > > > x = rte_clz64(s); // assume 64-bit today
> > > > 
> > > > this code is now broken because on 32-bit platform s will get promoted
> > > > and the extra 32 zero-bits will be returned in the result breaking
> > > > calculations.
> > > > 
> > > > any thoughts? should we go back to l, ll?
> > > > 
> > > 
> > > Yes, promotion will happen, but I still think that the 32 and 64 versions
> > > are far clearer here in all cases. Anyone looking at the code will
> > > recognise that the result will be the leading zero count of a 64-bit number
> > > irrespective of the type actually passed in. It's less confusing now IMHO.
> > 
> > here's an example in the code that would result in a bad calculation or
> > at least i believe so at a glance. switching to rte_clz32() would break
> > on 64-bit since it would truncate.
> > 
> > lib/eal/common/malloc_elem.c
> > 
> > -log2 = sizeof(size) * 8 - __builtin_clzl(size);
> > +log2 = sizeof(size) * 8 - rte_clz64(size);
> > 
> > if i'm right you'd have to conditionally compile at the site.
> > 
> > #ifdef 64-bit
> > rte_clz64()
> > #else
> > rte_clz32()
> > #endif
> > 
> 
> Why can clz64 not be used in both 32 and 64 bit cases? You know the result
> will always include zeros in high bits of a 64-bit value, and the result
> will still fit inside even an 8-bit variable?

of course. doh.

sorry for the noise!

and thank you as always.

> 
> /Bruce
diff mbox series

Patch

diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
index cfcd40a..8ff1d65 100644
--- a/lib/eal/include/meson.build
+++ b/lib/eal/include/meson.build
@@ -5,6 +5,7 @@  includes += include_directories('.')
 
 headers += files(
         'rte_alarm.h',
+        'rte_bitcount.h',
         'rte_bitmap.h',
         'rte_bitops.h',
         'rte_branch_prediction.h',
diff --git a/lib/eal/include/rte_bitcount.h b/lib/eal/include/rte_bitcount.h
new file mode 100644
index 0000000..587de52
--- /dev/null
+++ b/lib/eal/include/rte_bitcount.h
@@ -0,0 +1,265 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (C) 2022 Microsoft Corporation
+ */
+
+#ifndef _RTE_BITCOUNT_H_
+#define _RTE_BITCOUNT_H_
+
+#include <rte_compat.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#ifdef RTE_TOOLCHAIN_MSVC
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clz(unsigned int v)
+{
+	unsigned long rv;
+
+	(void)_BitScanReverse(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clzl(unsigned long v)
+{
+	unsigned long rv;
+
+	(void)_BitScanReverse(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clzll(unsigned long long v)
+{
+	unsigned long rv;
+
+	(void)_BitScanReverse64(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctz(unsigned int v)
+{
+	unsigned long rv;
+
+	(void)_BitScanForward(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctzl(unsigned long v)
+{
+	unsigned long rv;
+
+	(void)_BitScanForward(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctzll(unsigned long long v)
+{
+	unsigned long rv;
+
+	(void)_BitScanForward64(&rv, v);
+
+	return (unsigned int)rv;
+}
+
+#else
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clz(unsigned int v)
+{
+	return (unsigned int)__builtin_clz(v);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clzl(unsigned long v)
+{
+	return (unsigned int)__builtin_clzl(v);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of leading 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of leading zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_clzll(unsigned long v)
+{
+	return (unsigned int)__builtin_clzll(v);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctz(unsigned int v)
+{
+	return (unsigned int)__builtin_ctz(v);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctzl(unsigned long v)
+{
+	return (unsigned int)__builtin_ctzl(v);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the count of trailing 0-bits in v.
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   The count of trailing zero bits.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ctzll(unsigned long v)
+{
+	return (unsigned int)__builtin_ctzll(v);
+}
+
+#endif
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BITCOUNT_H_ */
+