Re: [dpdk-dev] [PATCH v1 1/5] lib/eal: implement the family of rte bitoperation APIs

["Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>]

Hi Honnappa,
> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Friday, November 1, 2019 9:48 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Morten
> Brørup <mb@smartsharesystems.com>; Joyce Kong (Arm Technology China)
> <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> ravi1.kumar@amd.com; xuanziyang2@huawei.com;
> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com;
> rmody@marvell.com; shshaikh@marvell.com; Stephen Hemminger
> <stephen@networkplumber.org>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v1 1/5] lib/eal: implement the family of rte
> bitoperation APIs
> 
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Joyce Kong
> > > (Arm
> > > > > Technology China)
> > > > > Sent: Wednesday, October 23, 2019 5:08 AM
> > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Joyce
> > > Kong
> > > > > > > > Sent: Tuesday, October 15, 2019 9:50 AM
> > > > > > > >
> > > > > > > > There are a lot functions of bit operations scattered and
> > > > > duplicated
> > > > > > > > in PMDs, consolidating them into a common API family is
> > > > > necessary.
> > > > > > > > Furthermore, the bit operation is mostly applied to the IO
> > > > > devices,
> > > > > > > > so use __ATOMIC_ACQ_REL to ensure the ordering.
> > > > > > >
> > > > > > > Good initiative.
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > > > > > > ---
> > > > > > > >  lib/librte_eal/common/Makefile             |  1 +
> > > > > > > >  lib/librte_eal/common/include/rte_bitops.h | 56
> > > > > > > > ++++++++++++++++++++++++++++++
> > > > > > > >  lib/librte_eal/common/meson.build          |  1 +
> > > > > > > >  3 files changed, 58 insertions(+)  create mode 100644
> > > > > > > > lib/librte_eal/common/include/rte_bitops.h
> > > > > > > >
> > > > > > > > diff --git a/lib/librte_eal/common/Makefile
> > > > > > > > b/lib/librte_eal/common/Makefile index a00d4fc..8586ca8
> > > > > > > > 100644
> > > > > > > > --- a/lib/librte_eal/common/Makefile
> > > > > > > > +++ b/lib/librte_eal/common/Makefile
> > > > > > > > @@ -18,6 +18,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..4d7c5a3
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_bitops.h
> > > > > > > > @@ -0,0 +1,56 @@
> > > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > + * Copyright(c) 2019 Arm Corporation  */
> > > > > > > > +
> > > > > > > > +#ifndef _RTE_BITOPS_H_
> > > > > > > > +#define _RTE_BITOPS_H_
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * @file
> > > > > > > > + * Bit Operations
> > > > > > > > + *
> > > > > > > > + * This file defines a generic API for bit operations.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <stdint.h>
> > > > > > > > +#include <rte_atomic.h>
> > > > > > > > +
> > > > > > > > +static inline void
> > > > > > > > +rte_set_bit(unsigned int nr, unsigned long *addr) {
> > > > > > > > +__atomic_fetch_or(addr, (1UL << nr), __ATOMIC_ACQ_REL); }
> > > > > > > > +
> > > > > > > > +static inline void
> > > > > > > > +rte_clear_bit(int nr, unsigned long *addr) {
> > > > > > > > +__atomic_fetch_and(addr, ~(1UL << nr),
> > > > __ATOMIC_ACQ_REL); }
> > > > > > > > +
> > > > > > > > +static inline int
> > > > > > > > +rte_test_bit(int nr, unsigned long *addr) { int res;
> > > > > > > > +rte_mb(); res = ((*addr) & (1UL << nr)) != 0; rte_mb();
> > > > > > > > +
> > > > > > > > +return res;
> > > > > > > > +}
> > > > > > >
> > > > > > > Why does rte_test_bit() not use any of the __atomic_xx
> > > > > > > functions
> > > > > instead?
> > > > > > > E.g.:
> > > > > > >
> > > > > > > static inline int
> > > > > > > rte_test_bit(int nr, unsigned long *addr) { return
> > > > > > > __atomic_load_n(addr, __ATOMIC_ACQUIRE); }
> > > > > > >
> > > > > > You re right, it's better to use __atomic_xx here to keep the
> > > > > consistent with
> > > > > > other APIs.
> > > > > >
> > > > > > > > +
> > > > > > > > +static inline int
> > > > > > > > +rte_test_and_set_bit(int nr, unsigned long *addr) {
> > > > > > > > +unsigned long mask = (1UL << nr);
> > > > > > > > +
> > > > > > > > +return __atomic_fetch_or(addr, mask, __ATOMIC_ACQ_REL) &
> > > > > > > mask; }
> > > > > > > > +
> > > > > > > > +static inline int
> > > > > > > > +rte_test_and_clear_bit(int nr, unsigned long *addr) {
> > > > > > > > +unsigned long mask = (1UL << nr);
> > > > > > > > +
> > > > > > > > +return __atomic_fetch_and(addr, ~mask, __ATOMIC_ACQ_REL)
> > > > &
> > > > > > > mask; }
> > > > > > > > +#endif /* _RTE_BITOPS_H_ */
> > > > > > > > diff --git a/lib/librte_eal/common/meson.build
> > > > > > > > b/lib/librte_eal/common/meson.build
> > > > > > > > index 386577c..a277cdf 100644
> > > > > > > > --- a/lib/librte_eal/common/meson.build
> > > > > > > > +++ b/lib/librte_eal/common/meson.build
> > > > > > > > @@ -52,6 +52,7 @@ common_headers = files(
> > > > > > > > 'include/rte_alarm.h',  'include/rte_branch_prediction.h',
> > > > > > > >  'include/rte_bus.h',
> > > > > > > > +'include/rte_bitops.h',
> > > > > > > >  'include/rte_bitmap.h',
> > > > > > > >  'include/rte_class.h',
> > > > > > > >  'include/rte_common.h',
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >
> > > > > > >
> > > > > > > These functions use unsigned long as the type of their value,
> > > > > > > like they do in the PMDs.
> > > > > > >
> > > > > > > However, a generic bit operations library should preferably
> > > > > > > work
> > > > > with
> > > > > > > multiple types, like the __atomic_xx functions. Or use an well
> > > > > defined
> > > > > > > uint_NN_t type. Or have individually named functions for each
> > > > > > > type
> > > > > size,
> > > > > > e.g.
> > > > > > > rte_set_bit_32() and rte_set_bit_64().
> > > > > > >
> > > > > > Good suggestion! And will do this in next version.
> > > > >
> > > > > The PMDs which use the common API now are all 32bit operation, so
> > > > > change the definition to uint_32_t type instead of individually
> > > > > naming functions for each type size.
> > > >
> > > > Unless you are certain that all current and future I/O devices only
> > > > need 32
> > > bit,
> > > > it should provide variants for different types, like the rte_atomic_xxx
> API.
> > > Why not do these using macros? The __atomic_xxx APIs anyway work
> with
> > > multiple types. Then we do not have to provide variants for all sizes.
> >
> > We really come to the point for the community to give a guideline: how to
> > generalize APIs to support multiple-sized arguments.
> > Looks like macros was disliked by the community, for readability and
> > debuggability reasons.
> IMO, it should not be considered as a blanket ban on using macros. It should
> be considered case by case basis. For ex: I do not see a point in writing the
> same API for 32b/64b/128b especially when the APIs are one liners.
Jerin and Morten have different opinions, they thought the MACRO based scheme only as of the last resort. 
Another argument is the API familiarity(similar to rte io read APIs).
Joyce made a new version and let's see how the community balance the duplication and other considerations. 
/Gavin
> 
> > Besides macros, there are an alternative: _Generic
> > https://gcc.gnu.org/onlinedocs/gccint/GENERIC.html, but it is not
> supported
> > by older gcc(<4.9), this made a hard requirement for gcc/clang.
> >
> > We have to compromise over all these: code duplication, readability and
> > debuggability.
> > /Gavin
> > > >
> > > > There might also be a need to support both big and little endian
> > > > byte
> > > ordering?
> > > > Perhaps the CPU uses a different byte ordering than the I/O device
> > > > being accessed through this API. I don't know; I'm only providing
> > > > half baked
> > > feedback
> > > > on this point.
> > >
> >
>