[v6,0/2] eal: provide leading and trailing zero bit count abstraction

  v6:
  * remove stray #include <stdio.h>

v5:
  * fix implementation of msvc versions of rte_clz{32,64}
    incorrect use of _BitscanReverse{,64} index.
  * fix and expand unit test to exercise full range of counting
    over uint{32,64}_t input values. (which would have caught
    above mistake).
  * reduce commit title length
  * correct commit author

v4:
  * combine unit test commit into function addition commit

v3:
  * rename to use 32/64 instead of l/ll suffixes
  * add new functions to rte_bitops.h instead of new header
  * move other bit functions from rte_common.h to rte_bitops.h

v2:
  * use unsigned int instead of unsigned (checkpatches)
  * match multiple include guard naming convention to rte_common.h
  * add explicit extern "C" linkage to rte_bitcount.h
    note: not really needed but checkpatches required
  * add missing space around '-'

Tyler Retzlaff (2):
  eal: move bit operation common to bitops header
  eal: provide leading and trailing zero bit count abstraction

 app/test/meson.build            |   2 +
 app/test/test_bitcount.c        |  93 ++++++++
 app/test/test_common.c          |   1 +
 lib/eal/common/rte_reciprocal.c |   1 +
 lib/eal/include/rte_bitops.h    | 460 ++++++++++++++++++++++++++++++++++++++++
 lib/eal/include/rte_common.h    | 293 -------------------------
 6 files changed, 557 insertions(+), 293 deletions(-)
 create mode 100644 app/test/test_bitcount.c
  

hi folks,

i think this one can probably be merged?

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

thanks!

On Tue, Jan 10, 2023 at 11:46:39AM -0800, Tyler Retzlaff wrote:
> v6:
>   * remove stray #include <stdio.h>
> 
> v5:
>   * fix implementation of msvc versions of rte_clz{32,64}
>     incorrect use of _BitscanReverse{,64} index.
>   * fix and expand unit test to exercise full range of counting
>     over uint{32,64}_t input values. (which would have caught
>     above mistake).
>   * reduce commit title length
>   * correct commit author
> 
> v4:
>   * combine unit test commit into function addition commit
> 
> v3:
>   * rename to use 32/64 instead of l/ll suffixes
>   * add new functions to rte_bitops.h instead of new header
>   * move other bit functions from rte_common.h to rte_bitops.h
> 
> v2:
>   * use unsigned int instead of unsigned (checkpatches)
>   * match multiple include guard naming convention to rte_common.h
>   * add explicit extern "C" linkage to rte_bitcount.h
>     note: not really needed but checkpatches required
>   * add missing space around '-'
> 
> Tyler Retzlaff (2):
>   eal: move bit operation common to bitops header
>   eal: provide leading and trailing zero bit count abstraction
> 
>  app/test/meson.build            |   2 +
>  app/test/test_bitcount.c        |  93 ++++++++
>  app/test/test_common.c          |   1 +
>  lib/eal/common/rte_reciprocal.c |   1 +
>  lib/eal/include/rte_bitops.h    | 460 ++++++++++++++++++++++++++++++++++++++++
>  lib/eal/include/rte_common.h    | 293 -------------------------
>  6 files changed, 557 insertions(+), 293 deletions(-)
>  create mode 100644 app/test/test_bitcount.c
> 
> -- 
> 
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
  

Hello Tyler,

On Fri, Jan 20, 2023 at 11:14 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> hi folks,
>
> i think this one can probably be merged?
>
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
> patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

I like the cleanup of rte_common.h and additional unit tests, but the
MSVC bits don't belong here.
Please move them in your MSVC enablement series
https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.


Thanks.
  

On Thu, Feb 2, 2023 at 10:14 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Hello Tyler,
>
> On Fri, Jan 20, 2023 at 11:14 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > hi folks,
> >
> > i think this one can probably be merged?
> >
> > Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>
> I like the cleanup of rte_common.h and additional unit tests, but the
> MSVC bits don't belong here.
> Please move them in your MSVC enablement series
> https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.

Addendum, running the series through my checks, I can see that
test_bitcount.c is left unattended wrt the MAINTAINERS file.
app/test/test_bitcount.c must be put under the "EAL API and common
code" section.

Thanks.
  

On Thu, Feb 02, 2023 at 10:14:41AM +0100, David Marchand wrote:
> Hello Tyler,
> 
> On Fri, Jan 20, 2023 at 11:14 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > hi folks,
> >
> > i think this one can probably be merged?
> >
> > Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> I like the cleanup of rte_common.h and additional unit tests, but the
> MSVC bits don't belong here.
> Please move them in your MSVC enablement series

hm, the way i'm approaching this is to keep specific features together
similar to if i added new platform functionality for threads i add
windows or linux in the same series. similarly, in this case i'm adding
msvc and gcc in the same series.

the msvc enablement series introduces the changes for the build system
so it's about enabling a compiler not adding functionality to dpdk so
this change/API really aren't related to the other, but i agree the other
series is part of converging on the platform, toolchain combination
being enabled overall.

reconsider? if not i guess i'll just withdraw the actual API for now
and have to resubmit and review later since i don't think it belongs
mixed in with the compiler enablement.

> https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.

if the msvc series were just merged, i think the above discussion would be
moot no?

anyway, i'll abide by whatever decision you go with.

thanks!
  

On Thu, Feb 02, 2023 at 11:56:41AM +0100, David Marchand wrote:
> On Thu, Feb 2, 2023 at 10:14 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > Hello Tyler,
> >
> > On Fri, Jan 20, 2023 at 11:14 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > hi folks,
> > >
> > > i think this one can probably be merged?
> > >
> > > Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >
> > I like the cleanup of rte_common.h and additional unit tests, but the
> > MSVC bits don't belong here.
> > Please move them in your MSVC enablement series
> > https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.
> 
> Addendum, running the series through my checks, I can see that
> test_bitcount.c is left unattended wrt the MAINTAINERS file.
> app/test/test_bitcount.c must be put under the "EAL API and common
> code" section.

is this an automated check i should have run prior to submission?

either way i'll fix that up, sorry for the mistake.

> 
> Thanks.
> -- 
> David Marchand
  

On Thu, Feb 2, 2023 at 4:57 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
> > > I like the cleanup of rte_common.h and additional unit tests, but the
> > > MSVC bits don't belong here.
> > > Please move them in your MSVC enablement series
> > > https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.
> >
> > Addendum, running the series through my checks, I can see that
> > test_bitcount.c is left unattended wrt the MAINTAINERS file.
> > app/test/test_bitcount.c must be put under the "EAL API and common
> > code" section.
>
> is this an automated check i should have run prior to submission?

The script is devtools/check-maintainers.sh.
But unfortunately it is not run in the CI as its author never made it
reliable enough: it has false positives / known issues / accepted
issues.
Users (I guess Thomas and I, maybe some subtree maintainers) keep a
local reference of this script output, and compare against it.

This is something that is rarely needed: in general additions are done
as a single component in MAINTAINERS, so people naturally add
themselves with the top level directory, and that's it.
So I guess our current check is good enough.


>
> either way i'll fix that up, sorry for the mistake.

No worries, this is not something you could easily find unless
grepping and looking how things were done for other unit tests.
  

On Thu, Feb 2, 2023 at 4:56 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Thu, Feb 02, 2023 at 10:14:41AM +0100, David Marchand wrote:
> > Hello Tyler,
> >
> > On Fri, Jan 20, 2023 at 11:14 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > hi folks,
> > >
> > > i think this one can probably be merged?
> > >
> > > Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > patch 1/2 Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >
> > I like the cleanup of rte_common.h and additional unit tests, but the
> > MSVC bits don't belong here.
> > Please move them in your MSVC enablement series
>
> hm, the way i'm approaching this is to keep specific features together
> similar to if i added new platform functionality for threads i add
> windows or linux in the same series. similarly, in this case i'm adding
> msvc and gcc in the same series.
>
> the msvc enablement series introduces the changes for the build system
> so it's about enabling a compiler not adding functionality to dpdk so
> this change/API really aren't related to the other, but i agree the other
> series is part of converging on the platform, toolchain combination
> being enabled overall.
>
> reconsider? if not i guess i'll just withdraw the actual API for now
> and have to resubmit and review later since i don't think it belongs
> mixed in with the compiler enablement.
>
> > https://patchwork.dpdk.org/project/dpdk/list/?series=26662&state=%2A&archive=both.
>
> if the msvc series were just merged, i think the above discussion would be
> moot no?

Indeed, but until we have support for MSVC, this part of the patch is
putting dead (untested) code in the tree.
It is also confusing to see references to MSVC in the tree while we
have nothing setting this config item, and no documentation explaining
how/when this will be actually usable.