Re: [dpdk-dev] [PATCH v6 1/6] lib/eal: implement the family of rte bit operation APIs

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> > Subject: [PATCH v6 1/6] lib/eal: implement the family of rte bit
> > operation APIs
> >
> > There are a lot functions of bit operations scattered and duplicated
> > in PMDs, consolidating them into a common API family is necessary.
> > Furthermore, when the bit operation is applied to the IO devices, use
> > __ATOMIC_ACQ_REL to ensure the ordering for io bit operation.
> >
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  MAINTAINERS                                |   5 +
> >  doc/api/doxy-api-index.md                  |   5 +-
> >  lib/librte_eal/common/Makefile             |   1 +
> >  lib/librte_eal/common/include/rte_bitops.h | 474
> > +++++++++++++++++++++++++++++
> >  lib/librte_eal/common/meson.build          |   3 +-
> >  5 files changed, 485 insertions(+), 3 deletions(-)  create mode
> > 100644 lib/librte_eal/common/include/rte_bitops.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 4395d8d..d2a29a2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -236,6 +236,11 @@ M: Cristian Dumitrescu
> > <cristian.dumitrescu@intel.com>
> >  F: lib/librte_eal/common/include/rte_bitmap.h
> >  F: app/test/test_bitmap.c
> >
> > +Bitops
> > +M: Joyce Kong <joyce.kong@arm.com>
> > +F: lib/librte_eal/common/include/rte_bitops.h
> > +F: app/test/test_bitops.c
> > +
> >  MCSlock - EXPERIMENTAL
> >  M: Phil Yang <phil.yang@arm.com>
> >  F: lib/librte_eal/common/include/generic/rte_mcslock.h
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index
> > dff496b..ade7c01 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -133,12 +133,13 @@ The public API headers are grouped by topics:
> >    [BPF]                (@ref rte_bpf.h)
> >
> >  - **containers**:
> > +  [bitmap]             (@ref rte_bitmap.h),
> > +  [bitops]             (@ref rte_bitops.h),
> >    [mbuf]               (@ref rte_mbuf.h),
> >    [mbuf pool ops]      (@ref rte_mbuf_pool_ops.h),
> >    [ring]               (@ref rte_ring.h),
> >    [stack]              (@ref rte_stack.h),
> > -  [tailq]              (@ref rte_tailq.h),
> > -  [bitmap]             (@ref rte_bitmap.h)
> > +  [tailq]              (@ref rte_tailq.h)
> >
> >  - **packet framework**:
> >    * [port]             (@ref rte_port.h):
> > diff --git a/lib/librte_eal/common/Makefile
> > b/lib/librte_eal/common/Makefile index c2c6d92..dd025c1 100644
> > --- a/lib/librte_eal/common/Makefile
> > +++ b/lib/librte_eal/common/Makefile
> > @@ -19,6 +19,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h  INC
> > += rte_service.h rte_service_component.h  INC += rte_bitmap.h
> > rte_vfio.h rte_hypervisor.h rte_test.h  INC += rte_reciprocal.h
> > rte_fbarray.h rte_uuid.h
> > +INC += rte_bitops.h
> >
> >  GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h
> > rte_prefetch.h GENERIC_INC += rte_memcpy.h rte_cpuflags.h diff --git
> > a/lib/librte_eal/common/include/rte_bitops.h
> > b/lib/librte_eal/common/include/rte_bitops.h
> > new file mode 100644
> > index 0000000..34158d1
> > --- /dev/null
> > +++ b/lib/librte_eal/common/include/rte_bitops.h
> > @@ -0,0 +1,474 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 Arm Limited
> > + */
> > +
> > +#ifndef _RTE_BITOPS_H_
> > +#define _RTE_BITOPS_H_
> > +
> > +/**
> > + * @file
> > + * Bit Operations
> > + *
> > + * This file defines a API for bit operations without/with memory ordering.
> > + */
> > +
> > +#include <stdint.h>
> > +#include <rte_debug.h>
> > +#include <rte_compat.h>
> > +
> > +/*---------------------------- 32 bit operations
> > +----------------------------*/
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + *
> > + * Get the target bit from a 32-bit value without memory ordering.
> > + *
> > + * @param nr
> > + *   The target bit to get.
> > + * @param addr
> > + *   The address holding the bit.
> > + * @return
> > + *   The target bit.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_get_bit32_relaxed(unsigned int nr, uint32_t *addr) {
> Why not pass the memory order as a parameter? It would reduce the number
> of API calls by half.
I think these APIs should be modelled according to C11 __atomic_xxx APIs. Otherwise, the programmers have to understand another interface. It will also help reduce the number of APIs.
Converting these into macros will help remove the size based duplication of APIs. I came up with the following macro:

#define RTE_GET_BIT(nr, var, ret, memorder) \
({ \
    if (sizeof(var) == sizeof(uint32_t)) { \
        uint32_t mask1 = 1U << (nr)%32; \
        ret = __atomic_load_n(&var, (memorder)) & mask1;\
    } \
    else {\
        uint64_t mask2 = 1UL << (nr)%64;\
        ret = __atomic_load_n(&var, (memorder)) & mask2;\
    } \
})

The '%' is required to avoid a compiler warning/error. But the '%' operation will get removed by the compiler since 'nr' is a constant.
IMO, the macro itself is not complex and should not be a pain for debugging.

Currently, we have 20 APIs in this patch (possibly more coming in the future and creating an explosion with memory order/size combinations). The above macro will reduce this to 5 macros without further explosion in number of combinations.

Any thoughts? What do others think?