[dpdk-dev] [PATCH v1 0/5] implement common rte bit operation APIs in PMDs

[dpdk-dev] [PATCH v1 0/5] implement common rte bit operation APIs in PMDs

[Joyce Kong]

There are a lot functions of bit operations scattered in
PMDs, consolidate them into a common API family and applied
in different PMDs to reduce code duplication.

Joyce Kong (5):
  lib/eal: implement the family of rte bit operation APIs
  net/axgbe: use common rte bit operation APIs instead
  net/bnx2x: use common rte bit operation APIs instead
  net/hinic: use common rte bit operation APIs instead
  net/qede: use common rte bit operation APIs instead

 drivers/net/axgbe/axgbe_common.h           |  29 +----
 drivers/net/axgbe/axgbe_ethdev.c           |  14 +-
 drivers/net/axgbe/axgbe_mdio.c             |  14 +-
 drivers/net/bnx2x/bnx2x.c                  | 202 +++++++++++++----------------
 drivers/net/bnx2x/bnx2x.h                  |   5 +-
 drivers/net/bnx2x/ecore_sp.h               |   8 +-
 drivers/net/hinic/base/hinic_compat.h      |  35 +----
 drivers/net/hinic/hinic_pmd_ethdev.c       |  16 +--
 drivers/net/qede/base/bcm_osal.c           |  20 ---
 drivers/net/qede/base/bcm_osal.h           |  10 +-
 lib/librte_eal/common/Makefile             |   1 +
 lib/librte_eal/common/include/rte_bitops.h |  56 ++++++++
 lib/librte_eal/common/meson.build          |   1 +
 13 files changed, 180 insertions(+), 231 deletions(-)
 create mode 100644 lib/librte_eal/common/include/rte_bitops.h

-- 
2.7.4

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

[Joyce Kong]

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.

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;
+}
+
+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

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

[Stephen Hemminger]

On Tue, 15 Oct 2019 15:49:57 +0800
Joyce Kong <joyce.kong@arm.com> wrote:

> +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;
> +}
> +
> +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;
> +}

These functions need to be part of API, and have doxygen comments?

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

[Joyce Kong (Arm Technology China)]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, October 16, 2019 12:54 AM
> To: Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>
> Cc: dev@dpdk.org; 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; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v1 1/5] lib/eal: implement the family of rte
> bit operation APIs
> 
> On Tue, 15 Oct 2019 15:49:57 +0800
> Joyce Kong <joyce.kong@arm.com> wrote:
> 
> > +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;
> > +}
> > +
> > +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; }
> 
> These functions need to be part of API, and have doxygen comments?

Will add doxygen comments in next version.

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

[Jerin Jacob]

On Tue, Oct 15, 2019 at 1:20 PM Joyce Kong <joyce.kong@arm.com> wrote:
>
> 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.
>
> 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 +
> +
> +static inline void
> +rte_set_bit(unsigned int nr, unsigned long *addr)
> +{
> +       __atomic_fetch_or(addr, (1UL << nr), __ATOMIC_ACQ_REL);
> +}

If it is specific for IO the IMO, it makes sense call the API to
rte_io_set_bit() like rte_io_rmb
and change the header file to rte_io_bitops.h.

The barries are only needed for IO operations. Explicitly is not
conveying it in API name
would call for using it for normal cases.

Other option could be to introduce, generic and IO specific bit
operations operations
separately.

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

[Joyce Kong (Arm Technology China)]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, October 16, 2019 3:54 PM
> To: Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>
> Cc: dpdk-dev <dev@dpdk.org>; nd <nd@arm.com>; thomas@monjalon.net;
> jerinj@marvell.com; ravi1.kumar@amd.com; Ziyang Xuan
> <xuanziyang2@huawei.com>; Xiaoyun Wang
> <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
> <zhouguoyang@huawei.com>; Rasesh Mody <rmody@marvell.com>;
> Shahed Shaikh <shshaikh@marvell.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v1 1/5] lib/eal: implement the family of rte
> bit operation APIs
> 
> On Tue, Oct 15, 2019 at 1:20 PM Joyce Kong <joyce.kong@arm.com> wrote:
> >
> > 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.
> >
> > 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 +
> > +
> > +static inline void
> > +rte_set_bit(unsigned int nr, unsigned long *addr) {
> > +       __atomic_fetch_or(addr, (1UL << nr), __ATOMIC_ACQ_REL); }
> 
> If it is specific for IO the IMO, it makes sense call the API to
> rte_io_set_bit() like rte_io_rmb
> and change the header file to rte_io_bitops.h.
> 
> The barries are only needed for IO operations. Explicitly is not conveying it in
> API name would call for using it for normal cases.
> 
> Other option could be to introduce, generic and IO specific bit operations
> operations separately.

Would do some related changes in next version.

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

[Joyce Kong (Arm Technology China)]

> > On Tue, Oct 15, 2019 at 1:20 PM Joyce Kong <joyce.kong@arm.com> wrote:
> > >
> > > 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.
> > >
> > > 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 +
> > > +
> > > +static inline void
> > > +rte_set_bit(unsigned int nr, unsigned long *addr) {
> > > +       __atomic_fetch_or(addr, (1UL << nr), __ATOMIC_ACQ_REL); }
> >
> > If it is specific for IO the IMO, it makes sense call the API to
> > rte_io_set_bit() like rte_io_rmb
> > and change the header file to rte_io_bitops.h.
> >
> > The barries are only needed for IO operations. Explicitly is not
> > conveying it in API name would call for using it for normal cases.
> >
> > Other option could be to introduce, generic and IO specific bit
> > operations operations separately.
> 
> Would do some related changes in next version.

As bit operations are mostly applied to IO devices, change the header file
to rte_io_bitops.h to introduce IO specific bit operations now. And do this
change in v2.

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

[Stephen Hemminger]

On Tue, 15 Oct 2019 15:49:57 +0800
Joyce Kong <joyce.kong@arm.com> wrote:

> 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.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
	'include/rte_common.h',

Patchwork reports several build failures for this patch set.

/tmp/UB1604-64_K4.4.0_Clang3.8.0/x86_64-native-linuxapp-clang/62c86b2c1091439598f2f1688566632c/dpdk/x86_64-native-linuxapp-clang/lib/librte_pmd_bnx2x.a(bnx2x.o): In function `bnx2x_set_storm_rx_mode':
/tmp/UB1604-64_K4.4.0_Clang3.8.0/x86_64-native-linuxapp-clang/62c86b2c1091439598f2f1688566632c/dpdk/drivers/net/bnx2x/bnx2x.c:(.text+0x1602): undefined reference to `ret_set_bit'

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

[Morten Brørup]

> -----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);
}

> +
> +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().


Med venlig hilsen / kind regards
- Morten Brørup

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

[Joyce Kong (Arm Technology China)]

Hi Morten,

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, October 17, 2019 9:32 PM
> To: 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; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> 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
> > 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.

> Med venlig hilsen / kind regards
> - Morten Brørup

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

[Joyce Kong (Arm Technology China)]

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

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

[Morten Brørup]

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

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.

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

[Honnappa Nagarahalli]

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

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

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

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Thursday, October 24, 2019 1:30 AM
> To: 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; Gavin Hu (Arm Technology
> China) <Gavin.Hu@arm.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.
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.
> 

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

[Honnappa Nagarahalli]

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

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

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

[Gavin Hu (Arm Technology China)]

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

[dpdk-dev] [PATCH v1 2/5] net/axgbe: use common rte bit operation APIs instead

[Joyce Kong]

Remove its own bit operation APIs and use the common one,
this can reduce the code duplication largely.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
 drivers/net/axgbe/axgbe_common.h | 29 +----------------------------
 drivers/net/axgbe/axgbe_ethdev.c | 14 +++++++-------
 drivers/net/axgbe/axgbe_mdio.c   | 14 +++++++-------
 3 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/drivers/net/axgbe/axgbe_common.h b/drivers/net/axgbe/axgbe_common.h
index 34f60f1..9cabda8 100644
--- a/drivers/net/axgbe/axgbe_common.h
+++ b/drivers/net/axgbe/axgbe_common.h
@@ -22,6 +22,7 @@
 #include <pthread.h>
 
 #include <rte_byteorder.h>
+#include <rte_bitops.h>
 #include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_hexdump.h>
@@ -1674,34 +1675,6 @@ do {									\
 #define time_after_eq(a, b)     ((long)((a) - (b)) >= 0)
 #define time_before_eq(a, b)	time_after_eq(b, a)
 
-/*---bitmap support apis---*/
-static inline int axgbe_test_bit(int nr, volatile unsigned long *addr)
-{
-	int res;
-
-	rte_mb();
-	res = ((*addr) & (1UL << nr)) != 0;
-	rte_mb();
-	return res;
-}
-
-static inline void axgbe_set_bit(unsigned int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_or(addr, (1UL << nr));
-}
-
-static inline void axgbe_clear_bit(int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_and(addr, ~(1UL << nr));
-}
-
-static inline int axgbe_test_and_clear_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = (1UL << nr);
-
-	return __sync_fetch_and_and(addr, ~mask) & mask;
-}
-
 static inline unsigned long msecs_to_timer_cycles(unsigned int m)
 {
 	return rte_get_timer_hz() * (m / 1000);
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index d1f160e..cd990f5 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -201,8 +201,8 @@ axgbe_dev_start(struct rte_eth_dev *dev)
 	axgbe_dev_enable_tx(dev);
 	axgbe_dev_enable_rx(dev);
 
-	axgbe_clear_bit(AXGBE_STOPPED, &pdata->dev_state);
-	axgbe_clear_bit(AXGBE_DOWN, &pdata->dev_state);
+	rte_clear_bit(AXGBE_STOPPED, &pdata->dev_state);
+	rte_clear_bit(AXGBE_DOWN, &pdata->dev_state);
 	return 0;
 }
 
@@ -216,17 +216,17 @@ axgbe_dev_stop(struct rte_eth_dev *dev)
 
 	rte_intr_disable(&pdata->pci_dev->intr_handle);
 
-	if (axgbe_test_bit(AXGBE_STOPPED, &pdata->dev_state))
+	if (rte_test_bit(AXGBE_STOPPED, &pdata->dev_state))
 		return;
 
-	axgbe_set_bit(AXGBE_STOPPED, &pdata->dev_state);
+	rte_set_bit(AXGBE_STOPPED, &pdata->dev_state);
 	axgbe_dev_disable_tx(dev);
 	axgbe_dev_disable_rx(dev);
 
 	pdata->phy_if.phy_stop(pdata);
 	pdata->hw_if.exit(pdata);
 	memset(&dev->data->dev_link, 0, sizeof(struct rte_eth_link));
-	axgbe_set_bit(AXGBE_DOWN, &pdata->dev_state);
+	rte_set_bit(AXGBE_DOWN, &pdata->dev_state);
 }
 
 /* Clear all resources like TX/RX queues. */
@@ -598,8 +598,8 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 
 	pdata = eth_dev->data->dev_private;
 	/* initial state */
-	axgbe_set_bit(AXGBE_DOWN, &pdata->dev_state);
-	axgbe_set_bit(AXGBE_STOPPED, &pdata->dev_state);
+	rte_set_bit(AXGBE_DOWN, &pdata->dev_state);
+	rte_set_bit(AXGBE_STOPPED, &pdata->dev_state);
 	pdata->eth_dev = eth_dev;
 
 	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
diff --git a/drivers/net/axgbe/axgbe_mdio.c b/drivers/net/axgbe/axgbe_mdio.c
index 2721e5c..6f3b3f2 100644
--- a/drivers/net/axgbe/axgbe_mdio.c
+++ b/drivers/net/axgbe/axgbe_mdio.c
@@ -743,7 +743,7 @@ static int __axgbe_phy_config_aneg(struct axgbe_port *pdata)
 {
 	int ret;
 
-	axgbe_set_bit(AXGBE_LINK_INIT, &pdata->dev_state);
+	rte_set_bit(AXGBE_LINK_INIT, &pdata->dev_state);
 	pdata->link_check = rte_get_timer_cycles();
 
 	ret = pdata->phy_if.phy_impl.an_config(pdata);
@@ -807,9 +807,9 @@ static int axgbe_phy_config_aneg(struct axgbe_port *pdata)
 
 	ret = __axgbe_phy_config_aneg(pdata);
 	if (ret)
-		axgbe_set_bit(AXGBE_LINK_ERR, &pdata->dev_state);
+		rte_set_bit(AXGBE_LINK_ERR, &pdata->dev_state);
 	else
-		axgbe_clear_bit(AXGBE_LINK_ERR, &pdata->dev_state);
+		rte_clear_bit(AXGBE_LINK_ERR, &pdata->dev_state);
 
 	pthread_mutex_unlock(&pdata->an_mutex);
 
@@ -880,7 +880,7 @@ static void axgbe_phy_status(struct axgbe_port *pdata)
 	unsigned int link_aneg;
 	int an_restart;
 
-	if (axgbe_test_bit(AXGBE_LINK_ERR, &pdata->dev_state)) {
+	if (rte_test_bit(AXGBE_LINK_ERR, &pdata->dev_state)) {
 		pdata->phy.link = 0;
 		goto adjust_link;
 	}
@@ -900,10 +900,10 @@ static void axgbe_phy_status(struct axgbe_port *pdata)
 			return;
 		}
 		axgbe_phy_status_result(pdata);
-		if (axgbe_test_bit(AXGBE_LINK_INIT, &pdata->dev_state))
-			axgbe_clear_bit(AXGBE_LINK_INIT, &pdata->dev_state);
+		if (rte_test_bit(AXGBE_LINK_INIT, &pdata->dev_state))
+			rte_clear_bit(AXGBE_LINK_INIT, &pdata->dev_state);
 	} else {
-		if (axgbe_test_bit(AXGBE_LINK_INIT, &pdata->dev_state)) {
+		if (rte_test_bit(AXGBE_LINK_INIT, &pdata->dev_state)) {
 			axgbe_check_link_timeout(pdata);
 
 			if (link_aneg)
-- 
2.7.4

[dpdk-dev] [PATCH v1 3/5] net/bnx2x: use common rte bit operation APIs instead

[Joyce Kong]

Remove its own bit operation APIs and use the common one,
this can reduce the code duplication largely.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
 drivers/net/bnx2x/bnx2x.c    | 202 +++++++++++++++++++------------------------
 drivers/net/bnx2x/bnx2x.h    |   5 +-
 drivers/net/bnx2x/ecore_sp.h |   8 +-
 3 files changed, 94 insertions(+), 121 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
index e1dfe60..92c77d1 100644
--- a/drivers/net/bnx2x/bnx2x.c
+++ b/drivers/net/bnx2x/bnx2x.c
@@ -129,32 +129,6 @@ static void bnx2x_ack_sb(struct bnx2x_softc *sc, uint8_t igu_sb_id,
 			 uint8_t storm, uint16_t index, uint8_t op,
 			 uint8_t update);
 
-int bnx2x_test_bit(int nr, volatile unsigned long *addr)
-{
-	int res;
-
-	mb();
-	res = ((*addr) & (1UL << nr)) != 0;
-	mb();
-	return res;
-}
-
-void bnx2x_set_bit(unsigned int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_or(addr, (1UL << nr));
-}
-
-void bnx2x_clear_bit(int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_and(addr, ~(1UL << nr));
-}
-
-int bnx2x_test_and_clear_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = (1UL << nr);
-	return __sync_fetch_and_and(addr, ~mask) & mask;
-}
-
 int bnx2x_cmpxchg(volatile int *addr, int old, int new)
 {
 	return __sync_val_compare_and_swap(addr, old, new);
@@ -1427,11 +1401,11 @@ bnx2x_del_all_macs(struct bnx2x_softc *sc, struct ecore_vlan_mac_obj *mac_obj,
 
 	/* wait for completion of requested */
 	if (wait_for_comp) {
-		bnx2x_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
+		rte_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
 	}
 
 	/* Set the mac type of addresses we want to clear */
-	bnx2x_set_bit(mac_type, &vlan_mac_flags);
+	rte_set_bit(mac_type, &vlan_mac_flags);
 
 	rc = mac_obj->delete_all(sc, mac_obj, &vlan_mac_flags, &ramrod_flags);
 	if (rc < 0)
@@ -1458,26 +1432,26 @@ bnx2x_fill_accept_flags(struct bnx2x_softc *sc, uint32_t rx_mode,
 		break;
 
 	case BNX2X_RX_MODE_NORMAL:
-		bnx2x_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_MULTICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_MULTICAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
 
 		/* internal switching mode */
-		bnx2x_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_MULTICAST, tx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_MULTICAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
 
 		break;
 
 	case BNX2X_RX_MODE_ALLMULTI:
-		bnx2x_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_ALL_MULTICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ALL_MULTICAST, rx_accept_flags);
+		ret_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
 
 		/* internal switching mode */
-		bnx2x_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_ALL_MULTICAST, tx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ALL_MULTICAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
 
 		break;
 
@@ -1488,19 +1462,19 @@ bnx2x_fill_accept_flags(struct bnx2x_softc *sc, uint32_t rx_mode,
 		 * should receive matched and unmatched (in resolution of port)
 		 * unicast packets.
 		 */
-		bnx2x_set_bit(ECORE_ACCEPT_UNMATCHED, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_ALL_MULTICAST, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNMATCHED, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_UNICAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ALL_MULTICAST, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_BROADCAST, rx_accept_flags);
 
 		/* internal switching mode */
-		bnx2x_set_bit(ECORE_ACCEPT_ALL_MULTICAST, tx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ALL_MULTICAST, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_BROADCAST, tx_accept_flags);
 
 		if (IS_MF_SI(sc)) {
-			bnx2x_set_bit(ECORE_ACCEPT_ALL_UNICAST, tx_accept_flags);
+			rte_set_bit(ECORE_ACCEPT_ALL_UNICAST, tx_accept_flags);
 		} else {
-			bnx2x_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
+			rte_set_bit(ECORE_ACCEPT_UNICAST, tx_accept_flags);
 		}
 
 		break;
@@ -1512,8 +1486,8 @@ bnx2x_fill_accept_flags(struct bnx2x_softc *sc, uint32_t rx_mode,
 
 	/* Set ACCEPT_ANY_VLAN as we do not enable filtering by VLAN */
 	if (rx_mode != BNX2X_RX_MODE_NONE) {
-		bnx2x_set_bit(ECORE_ACCEPT_ANY_VLAN, rx_accept_flags);
-		bnx2x_set_bit(ECORE_ACCEPT_ANY_VLAN, tx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ANY_VLAN, rx_accept_flags);
+		rte_set_bit(ECORE_ACCEPT_ANY_VLAN, tx_accept_flags);
 	}
 
 	return 0;
@@ -1542,7 +1516,7 @@ bnx2x_set_q_rx_mode(struct bnx2x_softc *sc, uint8_t cl_id,
 	ramrod_param.rdata = BNX2X_SP(sc, rx_mode_rdata);
 	ramrod_param.rdata_mapping =
 	    (rte_iova_t)BNX2X_SP_MAPPING(sc, rx_mode_rdata),
-	    bnx2x_set_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state);
+	    rte_set_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state);
 
 	ramrod_param.ramrod_flags = ramrod_flags;
 	ramrod_param.rx_mode_flags = rx_mode_flags;
@@ -1571,9 +1545,9 @@ int bnx2x_set_storm_rx_mode(struct bnx2x_softc *sc)
 		return rc;
 	}
 
-	bnx2x_set_bit(RAMROD_RX, &ramrod_flags);
-	bnx2x_set_bit(RAMROD_TX, &ramrod_flags);
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
+	rte_set_bit(RAMROD_RX, &ramrod_flags);
+	rte_set_bit(RAMROD_TX, &ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
 
 	return bnx2x_set_q_rx_mode(sc, sc->fp[0].cl_id, rx_mode_flags,
 				 rx_accept_flags, tx_accept_flags,
@@ -1698,7 +1672,7 @@ static int bnx2x_func_wait_started(struct bnx2x_softc *sc)
 			    "Forcing STARTED-->TX_STOPPED-->STARTED");
 
 		func_params.f_obj = &sc->func_obj;
-		bnx2x_set_bit(RAMROD_DRV_CLR_ONLY, &func_params.ramrod_flags);
+		rte_set_bit(RAMROD_DRV_CLR_ONLY, &func_params.ramrod_flags);
 
 		/* STARTED-->TX_STOPPED */
 		func_params.cmd = ECORE_F_CMD_TX_STOP;
@@ -1722,7 +1696,7 @@ static int bnx2x_stop_queue(struct bnx2x_softc *sc, int index)
 
 	q_params.q_obj = &sc->sp_objs[fp->index].q_obj;
 	/* We want to wait for completion in this context */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &q_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &q_params.ramrod_flags);
 
 	/* Stop the primary connection: */
 
@@ -1783,7 +1757,7 @@ static int bnx2x_func_stop(struct bnx2x_softc *sc)
 	int rc;
 
 	/* prepare parameters for function state transitions */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
 	func_params.f_obj = &sc->func_obj;
 	func_params.cmd = ECORE_F_CMD_STOP;
 
@@ -1797,7 +1771,7 @@ static int bnx2x_func_stop(struct bnx2x_softc *sc)
 	if (rc) {
 		PMD_DRV_LOG(NOTICE, sc, "FUNC_STOP ramrod failed. "
 			    "Running a dry transaction");
-		bnx2x_set_bit(RAMROD_DRV_CLR_ONLY, &func_params.ramrod_flags);
+		rte_set_bit(RAMROD_DRV_CLR_ONLY, &func_params.ramrod_flags);
 		return ecore_func_state_change(sc, &func_params);
 	}
 
@@ -1809,7 +1783,7 @@ static int bnx2x_reset_hw(struct bnx2x_softc *sc, uint32_t load_code)
 	struct ecore_func_state_params func_params = { NULL };
 
 	/* Prepare parameters for function state transitions */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
 
 	func_params.f_obj = &sc->func_obj;
 	func_params.cmd = ECORE_F_CMD_HW_RESET;
@@ -1866,8 +1840,8 @@ bnx2x_chip_cleanup(struct bnx2x_softc *sc, uint32_t unload_mode, uint8_t keep_li
 	 * a race between the completion code and this code.
 	 */
 
-	if (bnx2x_test_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state)) {
-		bnx2x_set_bit(ECORE_FILTER_RX_MODE_SCHED, &sc->sp_state);
+	if (rte_test_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state)) {
+		rte_set_bit(ECORE_FILTER_RX_MODE_SCHED, &sc->sp_state);
 	} else {
 		bnx2x_set_storm_rx_mode(sc);
 	}
@@ -1960,12 +1934,12 @@ static void bnx2x_squeeze_objects(struct bnx2x_softc *sc)
 	/* Cleanup MACs' object first... */
 
 	/* Wait for completion of requested */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
 	/* Perform a dry cleanup */
-	bnx2x_set_bit(RAMROD_DRV_CLR_ONLY, &ramrod_flags);
+	rte_set_bit(RAMROD_DRV_CLR_ONLY, &ramrod_flags);
 
 	/* Clean ETH primary MAC */
-	bnx2x_set_bit(ECORE_ETH_MAC, &vlan_mac_flags);
+	rte_set_bit(ECORE_ETH_MAC, &vlan_mac_flags);
 	rc = mac_obj->delete_all(sc, &sc->sp_objs->mac_obj, &vlan_mac_flags,
 				 &ramrod_flags);
 	if (rc != 0) {
@@ -1974,7 +1948,7 @@ static void bnx2x_squeeze_objects(struct bnx2x_softc *sc)
 
 	/* Cleanup UC list */
 	vlan_mac_flags = 0;
-	bnx2x_set_bit(ECORE_UC_LIST_MAC, &vlan_mac_flags);
+	rte_set_bit(ECORE_UC_LIST_MAC, &vlan_mac_flags);
 	rc = mac_obj->delete_all(sc, mac_obj, &vlan_mac_flags, &ramrod_flags);
 	if (rc != 0) {
 		PMD_DRV_LOG(NOTICE, sc,
@@ -1984,7 +1958,7 @@ static void bnx2x_squeeze_objects(struct bnx2x_softc *sc)
 	/* Now clean mcast object... */
 
 	rparam.mcast_obj = &sc->mcast_obj;
-	bnx2x_set_bit(RAMROD_DRV_CLR_ONLY, &rparam.ramrod_flags);
+	rte_set_bit(RAMROD_DRV_CLR_ONLY, &rparam.ramrod_flags);
 
 	/* Add a DEL command... */
 	rc = ecore_config_mcast(sc, &rparam, ECORE_MCAST_CMD_DEL);
@@ -4288,7 +4262,7 @@ bnx2x_handle_classification_eqe(struct bnx2x_softc *sc, union event_ring_elem *e
 	struct ecore_vlan_mac_obj *vlan_mac_obj;
 
 	/* always push next commands out, don't wait here */
-	bnx2x_set_bit(RAMROD_CONT, &ramrod_flags);
+	rte_set_bit(RAMROD_CONT, &ramrod_flags);
 
 	switch (le32toh(elem->message.data.eth_event.echo) >> BNX2X_SWCID_SHIFT) {
 	case ECORE_FILTER_MAC_PENDING:
@@ -4319,10 +4293,10 @@ bnx2x_handle_classification_eqe(struct bnx2x_softc *sc, union event_ring_elem *e
 
 static void bnx2x_handle_rx_mode_eqe(struct bnx2x_softc *sc)
 {
-	bnx2x_clear_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state);
+	rte_clear_bit(ECORE_FILTER_RX_MODE_PENDING, &sc->sp_state);
 
 	/* send rx_mode command again if was requested */
-	if (bnx2x_test_and_clear_bit(ECORE_FILTER_RX_MODE_SCHED, &sc->sp_state)) {
+	if (rte_test_and_clear_bit(ECORE_FILTER_RX_MODE_SCHED, &sc->sp_state)) {
 		bnx2x_set_storm_rx_mode(sc);
 	}
 }
@@ -4693,7 +4667,7 @@ static int bnx2x_init_hw(struct bnx2x_softc *sc, uint32_t load_code)
 	PMD_INIT_FUNC_TRACE(sc);
 
 	/* prepare the parameters for function state transitions */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
 
 	func_params.f_obj = &sc->func_obj;
 	func_params.cmd = ECORE_F_CMD_HW_INIT;
@@ -4988,8 +4962,8 @@ static void bnx2x_init_eth_fp(struct bnx2x_softc *sc, int idx)
 	bnx2x_update_fp_sb_idx(fp);
 
 	/* Configure Queue State object */
-	bnx2x_set_bit(ECORE_Q_TYPE_HAS_RX, &q_type);
-	bnx2x_set_bit(ECORE_Q_TYPE_HAS_TX, &q_type);
+	rte_set_bit(ECORE_Q_TYPE_HAS_RX, &q_type);
+	rte_set_bit(ECORE_Q_TYPE_HAS_TX, &q_type);
 
 	ecore_init_queue_obj(sc,
 			     &sc->sp_objs[idx].q_obj,
@@ -5803,7 +5777,7 @@ static int bnx2x_func_start(struct bnx2x_softc *sc)
 	    &func_params.params.start;
 
 	/* Prepare parameters for function state transitions */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &func_params.ramrod_flags);
 
 	func_params.f_obj = &sc->func_obj;
 	func_params.cmd = ECORE_F_CMD_START;
@@ -6379,11 +6353,11 @@ bnx2x_pf_q_prep_init(struct bnx2x_softc *sc, struct bnx2x_fastpath *fp,
 	uint8_t cos;
 	int cxt_index, cxt_offset;
 
-	bnx2x_set_bit(ECORE_Q_FLG_HC, &init_params->rx.flags);
-	bnx2x_set_bit(ECORE_Q_FLG_HC, &init_params->tx.flags);
+	rte_set_bit(ECORE_Q_FLG_HC, &init_params->rx.flags);
+	rte_set_bit(ECORE_Q_FLG_HC, &init_params->tx.flags);
 
-	bnx2x_set_bit(ECORE_Q_FLG_HC_EN, &init_params->rx.flags);
-	bnx2x_set_bit(ECORE_Q_FLG_HC_EN, &init_params->tx.flags);
+	rte_set_bit(ECORE_Q_FLG_HC_EN, &init_params->rx.flags);
+	rte_set_bit(ECORE_Q_FLG_HC_EN, &init_params->tx.flags);
 
 	/* HC rate */
 	init_params->rx.hc_rate =
@@ -6417,7 +6391,7 @@ bnx2x_get_common_flags(struct bnx2x_softc *sc, uint8_t zero_stats)
 	unsigned long flags = 0;
 
 	/* PF driver will always initialize the Queue to an ACTIVE state */
-	bnx2x_set_bit(ECORE_Q_FLG_ACTIVE, &flags);
+	rte_set_bit(ECORE_Q_FLG_ACTIVE, &flags);
 
 	/*
 	 * tx only connections collect statistics (on the same index as the
@@ -6425,9 +6399,9 @@ bnx2x_get_common_flags(struct bnx2x_softc *sc, uint8_t zero_stats)
 	 * connection is initialized.
 	 */
 
-	bnx2x_set_bit(ECORE_Q_FLG_STATS, &flags);
+	rte_set_bit(ECORE_Q_FLG_STATS, &flags);
 	if (zero_stats) {
-		bnx2x_set_bit(ECORE_Q_FLG_ZERO_STATS, &flags);
+		rte_set_bit(ECORE_Q_FLG_ZERO_STATS, &flags);
 	}
 
 	/*
@@ -6435,10 +6409,10 @@ bnx2x_get_common_flags(struct bnx2x_softc *sc, uint8_t zero_stats)
 	 * CoS-ness doesn't survive the loopback
 	 */
 	if (sc->flags & BNX2X_TX_SWITCHING) {
-		bnx2x_set_bit(ECORE_Q_FLG_TX_SWITCH, &flags);
+		rte_set_bit(ECORE_Q_FLG_TX_SWITCH, &flags);
 	}
 
-	bnx2x_set_bit(ECORE_Q_FLG_PCSUM_ON_PKT, &flags);
+	rte_set_bit(ECORE_Q_FLG_PCSUM_ON_PKT, &flags);
 
 	return flags;
 }
@@ -6448,15 +6422,15 @@ static unsigned long bnx2x_get_q_flags(struct bnx2x_softc *sc, uint8_t leading)
 	unsigned long flags = 0;
 
 	if (IS_MF_SD(sc)) {
-		bnx2x_set_bit(ECORE_Q_FLG_OV, &flags);
+		rte_set_bit(ECORE_Q_FLG_OV, &flags);
 	}
 
 	if (leading) {
-		bnx2x_set_bit(ECORE_Q_FLG_LEADING_RSS, &flags);
-		bnx2x_set_bit(ECORE_Q_FLG_MCAST, &flags);
+		rte_set_bit(ECORE_Q_FLG_LEADING_RSS, &flags);
+		rte_set_bit(ECORE_Q_FLG_MCAST, &flags);
 	}
 
-	bnx2x_set_bit(ECORE_Q_FLG_VLAN, &flags);
+	rte_set_bit(ECORE_Q_FLG_VLAN, &flags);
 
 	/* merge with common flags */
 	return flags | bnx2x_get_common_flags(sc, TRUE);
@@ -6577,7 +6551,7 @@ bnx2x_setup_queue(struct bnx2x_softc *sc, struct bnx2x_fastpath *fp, uint8_t lea
 	q_params.q_obj = &BNX2X_SP_OBJ(sc, fp).q_obj;
 
 	/* we want to wait for completion in this context */
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &q_params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &q_params.ramrod_flags);
 
 	/* prepare the INIT parameters */
 	bnx2x_pf_q_prep_init(sc, fp, &q_params.params.init);
@@ -6645,20 +6619,20 @@ bnx2x_config_rss_pf(struct bnx2x_softc *sc, struct ecore_rss_config_obj *rss_obj
 
 	params.rss_obj = rss_obj;
 
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &params.ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &params.ramrod_flags);
 
-	bnx2x_set_bit(ECORE_RSS_MODE_REGULAR, &params.rss_flags);
+	rte_set_bit(ECORE_RSS_MODE_REGULAR, &params.rss_flags);
 
 	/* RSS configuration */
-	bnx2x_set_bit(ECORE_RSS_IPV4, &params.rss_flags);
-	bnx2x_set_bit(ECORE_RSS_IPV4_TCP, &params.rss_flags);
-	bnx2x_set_bit(ECORE_RSS_IPV6, &params.rss_flags);
-	bnx2x_set_bit(ECORE_RSS_IPV6_TCP, &params.rss_flags);
+	rte_set_bit(ECORE_RSS_IPV4, &params.rss_flags);
+	rte_set_bit(ECORE_RSS_IPV4_TCP, &params.rss_flags);
+	rte_set_bit(ECORE_RSS_IPV6, &params.rss_flags);
+	rte_set_bit(ECORE_RSS_IPV6_TCP, &params.rss_flags);
 	if (rss_obj->udp_rss_v4) {
-		bnx2x_set_bit(ECORE_RSS_IPV4_UDP, &params.rss_flags);
+		rte_set_bit(ECORE_RSS_IPV4_UDP, &params.rss_flags);
 	}
 	if (rss_obj->udp_rss_v6) {
-		bnx2x_set_bit(ECORE_RSS_IPV6_UDP, &params.rss_flags);
+		rte_set_bit(ECORE_RSS_IPV6_UDP, &params.rss_flags);
 	}
 
 	/* Hash bits */
@@ -6673,7 +6647,7 @@ bnx2x_config_rss_pf(struct bnx2x_softc *sc, struct ecore_rss_config_obj *rss_obj
 			params.rss_key[i] = (uint32_t) rte_rand();
 		}
 
-		bnx2x_set_bit(ECORE_RSS_SET_SRCH, &params.rss_flags);
+		rte_set_bit(ECORE_RSS_SET_SRCH, &params.rss_flags);
 	}
 
 	if (IS_PF(sc))
@@ -6730,11 +6704,11 @@ bnx2x_set_mac_one(struct bnx2x_softc *sc, uint8_t * mac,
 	ramrod_param.ramrod_flags = *ramrod_flags;
 
 	/* fill a user request section if needed */
-	if (!bnx2x_test_bit(RAMROD_CONT, ramrod_flags)) {
+	if (!rte_test_bit(RAMROD_CONT, ramrod_flags)) {
 		rte_memcpy(ramrod_param.user_req.u.mac.mac, mac,
 				 ETH_ALEN);
 
-		bnx2x_set_bit(mac_type, &ramrod_param.user_req.vlan_mac_flags);
+		rte_set_bit(mac_type, &ramrod_param.user_req.vlan_mac_flags);
 
 /* Set the command: ADD or DEL */
 		ramrod_param.user_req.cmd = (set) ? ECORE_VLAN_MAC_ADD :
@@ -6761,7 +6735,7 @@ static int bnx2x_set_eth_mac(struct bnx2x_softc *sc, uint8_t set)
 
 	PMD_DRV_LOG(DEBUG, sc, "Adding Ethernet MAC");
 
-	bnx2x_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
+	rte_set_bit(RAMROD_COMP_WAIT, &ramrod_flags);
 
 	/* Eth MAC is set on RSS leading client (fp[0]) */
 	return bnx2x_set_mac_one(sc, sc->link_params.mac_addr,
@@ -6893,24 +6867,26 @@ bnx2x_fill_report_data(struct bnx2x_softc *sc, struct bnx2x_link_report_data *da
 
 	/* Link is down */
 	if (!sc->link_vars.link_up || (sc->flags & BNX2X_MF_FUNC_DIS)) {
-		bnx2x_set_bit(BNX2X_LINK_REPORT_LINK_DOWN,
+		rte_set_bit(BNX2X_LINK_REPORT_LINK_DOWN,
 			    &data->link_report_flags);
 	}
 
 	/* Full DUPLEX */
 	if (sc->link_vars.duplex == DUPLEX_FULL) {
-		bnx2x_set_bit(BNX2X_LINK_REPORT_FULL_DUPLEX,
+		rte_set_bit(BNX2X_LINK_REPORT_FULL_DUPLEX,
 			    &data->link_report_flags);
 	}
 
 	/* Rx Flow Control is ON */
 	if (sc->link_vars.flow_ctrl & ELINK_FLOW_CTRL_RX) {
-		bnx2x_set_bit(BNX2X_LINK_REPORT_RX_FC_ON, &data->link_report_flags);
+		rte_set_bit(BNX2X_LINK_REPORT_RX_FC_ON,
+				&data->link_report_flags);
 	}
 
 	/* Tx Flow Control is ON */
 	if (sc->link_vars.flow_ctrl & ELINK_FLOW_CTRL_TX) {
-		bnx2x_set_bit(BNX2X_LINK_REPORT_TX_FC_ON, &data->link_report_flags);
+		rte_set_bit(BNX2X_LINK_REPORT_TX_FC_ON,
+				&data->link_report_flags);
 	}
 }
 
@@ -6929,9 +6905,9 @@ static void bnx2x_link_report_locked(struct bnx2x_softc *sc)
 
 	/* Don't report link down or exactly the same link status twice */
 	if (!memcmp(&cur_data, &sc->last_reported_link, sizeof(cur_data)) ||
-	    (bnx2x_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
+	    (rte_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
 			  &sc->last_reported_link.link_report_flags) &&
-	     bnx2x_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
+	     rte_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
 			  &cur_data.link_report_flags))) {
 		return;
 	}
@@ -6946,14 +6922,14 @@ static void bnx2x_link_report_locked(struct bnx2x_softc *sc)
 	/* report new link params and remember the state for the next time */
 	rte_memcpy(&sc->last_reported_link, &cur_data, sizeof(cur_data));
 
-	if (bnx2x_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
+	if (rte_test_bit(BNX2X_LINK_REPORT_LINK_DOWN,
 			 &cur_data.link_report_flags)) {
 		ELINK_DEBUG_P0(sc, "NIC Link is Down");
 	} else {
 		__rte_unused const char *duplex;
 		__rte_unused const char *flow;
 
-		if (bnx2x_test_and_clear_bit(BNX2X_LINK_REPORT_FULL_DUPLEX,
+		if (rte_test_and_clear_bit(BNX2X_LINK_REPORT_FULL_DUPLEX,
 					   &cur_data.link_report_flags)) {
 			duplex = "full";
 				ELINK_DEBUG_P0(sc, "link set to full duplex");
@@ -6968,19 +6944,19 @@ static void bnx2x_link_report_locked(struct bnx2x_softc *sc)
  * enabled.
  */
 		if (cur_data.link_report_flags) {
-			if (bnx2x_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
+			if (rte_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
 					 &cur_data.link_report_flags) &&
-			    bnx2x_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
+			    rte_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
 					 &cur_data.link_report_flags)) {
 				flow = "ON - receive & transmit";
-			} else if (bnx2x_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
+			} else if (rte_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
 						&cur_data.link_report_flags) &&
-				   !bnx2x_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
+				   !rte_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
 						 &cur_data.link_report_flags)) {
 				flow = "ON - receive";
-			} else if (!bnx2x_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
+			} else if (!rte_test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
 						 &cur_data.link_report_flags) &&
-				   bnx2x_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
+				   rte_test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
 						&cur_data.link_report_flags)) {
 				flow = "ON - transmit";
 			} else {
diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index 43c6040..aa2d251 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -15,6 +15,7 @@
 #define __BNX2X_H__
 
 #include <rte_byteorder.h>
+#include <rte_bitops.h>
 #include <rte_spinlock.h>
 #include <rte_bus_pci.h>
 #include <rte_io.h>
@@ -1809,10 +1810,6 @@ static const uint32_t dmae_reg_go_c[] = {
 #define PCI_PM_D0    1
 #define PCI_PM_D3hot 2
 
-int  bnx2x_test_bit(int nr, volatile unsigned long * addr);
-void bnx2x_set_bit(unsigned int nr, volatile unsigned long * addr);
-void bnx2x_clear_bit(int nr, volatile unsigned long * addr);
-int  bnx2x_test_and_clear_bit(int nr, volatile unsigned long * addr);
 int  bnx2x_cmpxchg(volatile int *addr, int old, int new);
 
 int bnx2x_dma_alloc(struct bnx2x_softc *sc, size_t size,
diff --git a/drivers/net/bnx2x/ecore_sp.h b/drivers/net/bnx2x/ecore_sp.h
index cc1db37..72697c2 100644
--- a/drivers/net/bnx2x/ecore_sp.h
+++ b/drivers/net/bnx2x/ecore_sp.h
@@ -73,10 +73,10 @@ typedef rte_spinlock_t ECORE_MUTEX_SPIN;
 #define ECORE_SET_BIT_NA(bit, var)         (*var |= (1 << bit))
 #define ECORE_CLEAR_BIT_NA(bit, var)       (*var &= ~(1 << bit))
 
-#define ECORE_TEST_BIT(bit, var)           bnx2x_test_bit(bit, var)
-#define ECORE_SET_BIT(bit, var)            bnx2x_set_bit(bit, var)
-#define ECORE_CLEAR_BIT(bit, var)          bnx2x_clear_bit(bit, var)
-#define ECORE_TEST_AND_CLEAR_BIT(bit, var) bnx2x_test_and_clear_bit(bit, var)
+#define ECORE_TEST_BIT(bit, var)           rte_test_bit(bit, var)
+#define ECORE_SET_BIT(bit, var)            rte_set_bit(bit, var)
+#define ECORE_CLEAR_BIT(bit, var)          rte_clear_bit(bit, var)
+#define ECORE_TEST_AND_CLEAR_BIT(bit, var) rte_test_and_clear_bit(bit, var)
 
 #define atomic_load_acq_int                (int)*
 #define atomic_store_rel_int(a, v)         (*a = v)
-- 
2.7.4

[dpdk-dev] [PATCH v1 4/5] net/hinic: use common rte bit operation APIs instead

[Joyce Kong]

Remove its own bit operation APIs and use the common one,
this can reduce the code duplication largely.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
 drivers/net/hinic/base/hinic_compat.h | 35 +----------------------------------
 drivers/net/hinic/hinic_pmd_ethdev.c  | 16 ++++++++--------
 2 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/drivers/net/hinic/base/hinic_compat.h b/drivers/net/hinic/base/hinic_compat.h
index f599947..ce1fdc6 100644
--- a/drivers/net/hinic/base/hinic_compat.h
+++ b/drivers/net/hinic/base/hinic_compat.h
@@ -11,6 +11,7 @@
 #include <pthread.h>
 #include <rte_common.h>
 #include <rte_byteorder.h>
+#include <rte_bitops.h>
 #include <rte_memzone.h>
 #include <rte_memcpy.h>
 #include <rte_malloc.h>
@@ -117,40 +118,6 @@ extern int hinic_logtype;
 
 #define HINIC_PAGE_SIZE_DPDK	6
 
-static inline int hinic_test_bit(int nr, volatile unsigned long *addr)
-{
-	int res;
-
-	rte_mb();
-	res = ((*addr) & (1UL << nr)) != 0;
-	rte_mb();
-	return res;
-}
-
-static inline void hinic_set_bit(unsigned int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_or(addr, (1UL << nr));
-}
-
-static inline void hinic_clear_bit(int nr, volatile unsigned long *addr)
-{
-	__sync_fetch_and_and(addr, ~(1UL << nr));
-}
-
-static inline int hinic_test_and_clear_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = (1UL << nr);
-
-	return __sync_fetch_and_and(addr, ~mask) & mask;
-}
-
-static inline int hinic_test_and_set_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = (1UL << nr);
-
-	return __sync_fetch_and_or(addr, mask) & mask;
-}
-
 void *dma_zalloc_coherent(void *dev, size_t size, dma_addr_t *dma_handle,
 			  gfp_t flag);
 void *dma_zalloc_coherent_aligned(void *dev, size_t size,
diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c b/drivers/net/hinic/hinic_pmd_ethdev.c
index c9a400e..dcdcfb9 100644
--- a/drivers/net/hinic/hinic_pmd_ethdev.c
+++ b/drivers/net/hinic/hinic_pmd_ethdev.c
@@ -227,7 +227,7 @@ static void hinic_dev_interrupt_handler(void *param)
 	struct rte_eth_dev *dev = param;
 	struct hinic_nic_dev *nic_dev = HINIC_ETH_DEV_TO_PRIVATE_NIC_DEV(dev);
 
-	if (!hinic_test_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status)) {
+	if (!rte_test_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status)) {
 		PMD_DRV_LOG(WARNING, "Device's interrupt is disabled, ignore interrupt event, dev_name: %s, port_id: %d",
 			    nic_dev->proc_dev_name, dev->data->port_id);
 		return;
@@ -907,7 +907,7 @@ static int hinic_dev_start(struct rte_eth_dev *dev)
 	if (dev->data->dev_conf.intr_conf.lsc != 0)
 		(void)hinic_link_update(dev, 0);
 
-	hinic_set_bit(HINIC_DEV_START, &nic_dev->dev_status);
+	rte_set_bit(HINIC_DEV_START, &nic_dev->dev_status);
 
 	return 0;
 
@@ -1030,7 +1030,7 @@ static void hinic_dev_stop(struct rte_eth_dev *dev)
 	name = dev->data->name;
 	port_id = dev->data->port_id;
 
-	if (!hinic_test_and_clear_bit(HINIC_DEV_START, &nic_dev->dev_status)) {
+	if (!rte_test_and_clear_bit(HINIC_DEV_START, &nic_dev->dev_status)) {
 		PMD_DRV_LOG(INFO, "Device %s already stopped", name);
 		return;
 	}
@@ -1073,7 +1073,7 @@ static void hinic_disable_interrupt(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	int ret, retries = 0;
 
-	hinic_clear_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status);
+	rte_clear_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status);
 
 	/* disable msix interrupt in hardware */
 	hinic_set_msix_state(nic_dev->hwdev, 0, HINIC_MSIX_DISABLE);
@@ -2197,9 +2197,9 @@ static int hinic_func_init(struct rte_eth_dev *eth_dev)
 			    eth_dev->data->name);
 		goto enable_intr_fail;
 	}
-	hinic_set_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status);
+	rte_set_bit(HINIC_DEV_INTR_EN, &nic_dev->dev_status);
 
-	hinic_set_bit(HINIC_DEV_INIT, &nic_dev->dev_status);
+	rte_set_bit(HINIC_DEV_INIT, &nic_dev->dev_status);
 	PMD_DRV_LOG(INFO, "Initialize %s in primary successfully",
 		    eth_dev->data->name);
 
@@ -2236,7 +2236,7 @@ static void hinic_dev_close(struct rte_eth_dev *dev)
 {
 	struct hinic_nic_dev *nic_dev = HINIC_ETH_DEV_TO_PRIVATE_NIC_DEV(dev);
 
-	if (hinic_test_and_set_bit(HINIC_DEV_CLOSE, &nic_dev->dev_status)) {
+	if (rte_test_and_set_bit(HINIC_DEV_CLOSE, &nic_dev->dev_status)) {
 		PMD_DRV_LOG(WARNING, "Device %s already closed",
 			    dev->data->name);
 		return;
@@ -2316,7 +2316,7 @@ static int hinic_dev_uninit(struct rte_eth_dev *dev)
 	struct hinic_nic_dev *nic_dev;
 
 	nic_dev = HINIC_ETH_DEV_TO_PRIVATE_NIC_DEV(dev);
-	hinic_clear_bit(HINIC_DEV_INIT, &nic_dev->dev_status);
+	rte_clear_bit(HINIC_DEV_INIT, &nic_dev->dev_status);
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
-- 
2.7.4

[dpdk-dev] [PATCH v1 5/5] net/qede: use common rte bit operation APIs instead

[Joyce Kong]

Remove its own bit operation APIs and use the common one,
this can reduce the code duplication largely.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
 drivers/net/qede/base/bcm_osal.c | 20 --------------------
 drivers/net/qede/base/bcm_osal.h | 10 ++++------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/net/qede/base/bcm_osal.c b/drivers/net/qede/base/bcm_osal.c
index 9915df4..665833c 100644
--- a/drivers/net/qede/base/bcm_osal.c
+++ b/drivers/net/qede/base/bcm_osal.c
@@ -45,26 +45,6 @@ u32 qede_osal_log2(u32 val)
 	return log;
 }
 
-inline void qede_set_bit(u32 nr, unsigned long *addr)
-{
-	__sync_fetch_and_or(addr, (1UL << nr));
-}
-
-inline void qede_clr_bit(u32 nr, unsigned long *addr)
-{
-	__sync_fetch_and_and(addr, ~(1UL << nr));
-}
-
-inline bool qede_test_bit(u32 nr, unsigned long *addr)
-{
-	bool res;
-
-	rte_mb();
-	res = ((*addr) & (1UL << nr)) != 0;
-	rte_mb();
-	return res;
-}
-
 static inline u32 qede_ffb(unsigned long word)
 {
 	unsigned long first_bit;
diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index 51edc41..9f2be0a 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -8,6 +8,7 @@
 #define __BCM_OSAL_H
 
 #include <rte_byteorder.h>
+#include <rte_bitops.h>
 #include <rte_spinlock.h>
 #include <rte_malloc.h>
 #include <rte_atomic.h>
@@ -311,17 +312,14 @@ typedef struct osal_list_t {
 #define OSAL_BITS_PER_UL_MASK		(OSAL_BITS_PER_UL - 1)
 
 /* Bitops */
-void qede_set_bit(u32, unsigned long *);
 #define OSAL_SET_BIT(bit, bitmap) \
-	qede_set_bit(bit, bitmap)
+	rte_set_bit(bit, bitmap)
 
-void qede_clr_bit(u32, unsigned long *);
 #define OSAL_CLEAR_BIT(bit, bitmap) \
-	qede_clr_bit(bit, bitmap)
+	rte_clear_bit(bit, bitmap)
 
-bool qede_test_bit(u32, unsigned long *);
 #define OSAL_TEST_BIT(bit, bitmap) \
-	qede_test_bit(bit, bitmap)
+	rte_test_bit(bit, bitmap)
 
 u32 qede_find_first_bit(unsigned long *, u32);
 #define OSAL_FIND_FIRST_BIT(bitmap, length) \
-- 
2.7.4

Re: [dpdk-dev] [PATCH v1 0/5] implement common rte bit operation APIs in PMDs

[Stephen Hemminger]

On Tue, 15 Oct 2019 15:49:56 +0800
Joyce Kong <joyce.kong@arm.com> wrote:

> There are a lot functions of bit operations scattered in
> PMDs, consolidate them into a common API family and applied
> in different PMDs to reduce code duplication.
> 
> Joyce Kong (5):
>   lib/eal: implement the family of rte bit operation APIs
>   net/axgbe: use common rte bit operation APIs instead
>   net/bnx2x: use common rte bit operation APIs instead
>   net/hinic: use common rte bit operation APIs instead
>   net/qede: use common rte bit operation APIs instead
> 
>  drivers/net/axgbe/axgbe_common.h           |  29 +----
>  drivers/net/axgbe/axgbe_ethdev.c           |  14 +-
>  drivers/net/axgbe/axgbe_mdio.c             |  14 +-
>  drivers/net/bnx2x/bnx2x.c                  | 202 +++++++++++++----------------
>  drivers/net/bnx2x/bnx2x.h                  |   5 +-
>  drivers/net/bnx2x/ecore_sp.h               |   8 +-
>  drivers/net/hinic/base/hinic_compat.h      |  35 +----
>  drivers/net/hinic/hinic_pmd_ethdev.c       |  16 +--
>  drivers/net/qede/base/bcm_osal.c           |  20 ---
>  drivers/net/qede/base/bcm_osal.h           |  10 +-
>  lib/librte_eal/common/Makefile             |   1 +
>  lib/librte_eal/common/include/rte_bitops.h |  56 ++++++++
>  lib/librte_eal/common/meson.build          |   1 +
>  13 files changed, 180 insertions(+), 231 deletions(-)
>  create mode 100644 lib/librte_eal/common/include/rte_bitops.h
> 


This is a really good idea, and should have been done long ago.
Could you add tests for these as well?

Re: [dpdk-dev] [PATCH v1 0/5] implement common rte bit operation APIs in PMDs

[Joyce Kong (Arm Technology China)]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, October 16, 2019 12:51 AM
> To: Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>
> Cc: dev@dpdk.org; 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; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v1 0/5] implement common rte bit operation
> APIs in PMDs
> 
> On Tue, 15 Oct 2019 15:49:56 +0800
> Joyce Kong <joyce.kong@arm.com> wrote:
> 
> > There are a lot functions of bit operations scattered in PMDs,
> > consolidate them into a common API family and applied in different
> > PMDs to reduce code duplication.
> >
> > Joyce Kong (5):
> >   lib/eal: implement the family of rte bit operation APIs
> >   net/axgbe: use common rte bit operation APIs instead
> >   net/bnx2x: use common rte bit operation APIs instead
> >   net/hinic: use common rte bit operation APIs instead
> >   net/qede: use common rte bit operation APIs instead
> >
> >  drivers/net/axgbe/axgbe_common.h           |  29 +----
> >  drivers/net/axgbe/axgbe_ethdev.c           |  14 +-
> >  drivers/net/axgbe/axgbe_mdio.c             |  14 +-
> >  drivers/net/bnx2x/bnx2x.c                  | 202 +++++++++++++----------------
> >  drivers/net/bnx2x/bnx2x.h                  |   5 +-
> >  drivers/net/bnx2x/ecore_sp.h               |   8 +-
> >  drivers/net/hinic/base/hinic_compat.h      |  35 +----
> >  drivers/net/hinic/hinic_pmd_ethdev.c       |  16 +--
> >  drivers/net/qede/base/bcm_osal.c           |  20 ---
> >  drivers/net/qede/base/bcm_osal.h           |  10 +-
> >  lib/librte_eal/common/Makefile             |   1 +
> >  lib/librte_eal/common/include/rte_bitops.h |  56 ++++++++
> >  lib/librte_eal/common/meson.build          |   1 +
> >  13 files changed, 180 insertions(+), 231 deletions(-)  create mode
> > 100644 lib/librte_eal/common/include/rte_bitops.h
> >
> 
> 
> This is a really good idea, and should have been done long ago.
> Could you add tests for these as well?

Yes. Will add some tests for these APIs in next version.

[dpdk-dev] [PATCH v2 0/6] implement common rte bit operation APIs in PMDs

[Joyce Kong]

There are a lot functions of bit operations scattered in PMDs,
consolidate them into a common API family and applied in different
PMDs to reduce code duplication.

v2:
  1. Add doxygen comments for the rte bit operation API(suggested by Stephen Hemminger).
  2. Add test cases for common rte bit operation API(suggested by Stephen Hemminger).
  3. Change the header file to rte_io_bitops.h and the operation to rte_io_set_bit()etc.,
     as the API uses barriers inside and the barriers are only needed for IO operations
     (suggested by Jerin Jacob).
  4. Use an well defined uint_NN_t type(suggested by Morten Brørup).

Joyce Kong (6):
  lib/eal: implement the family of rte bit operation APIs
  test/iobitops: add io bit operation test case
  net/axgbe: use common rte bit operation APIs instead
  net/bnx2x: use common rte bit operation APIs instead
  net/hinic: use common rte bit operation APIs instead
  net/qede: use common rte bit operation APIs instead

 app/test/Makefile                             |   1 +
 app/test/test_io_bitops.c                     |  86 +++++++++++
 drivers/net/axgbe/axgbe_common.h              |  29 +---
 drivers/net/axgbe/axgbe_ethdev.c              |  14 +-
 drivers/net/axgbe/axgbe_mdio.c                |  14 +-
 drivers/net/bnx2x/bnx2x.c                     | 209 ++++++++++++--------------
 drivers/net/bnx2x/bnx2x.h                     |   4 -
 drivers/net/bnx2x/ecore_sp.h                  |   9 +-
 drivers/net/hinic/base/hinic_compat.h         |  35 +----
 drivers/net/hinic/hinic_pmd_ethdev.c          |  16 +-
 drivers/net/qede/base/bcm_osal.c              |  20 ---
 drivers/net/qede/base/bcm_osal.h              |  10 +-
 lib/librte_eal/common/Makefile                |   1 +
 lib/librte_eal/common/include/rte_io_bitops.h | 112 ++++++++++++++
 lib/librte_eal/common/meson.build             |   1 +
 15 files changed, 327 insertions(+), 234 deletions(-)
 create mode 100644 app/test/test_io_bitops.c
 create mode 100644 lib/librte_eal/common/include/rte_io_bitops.h

-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/6] implement common rte bit operation APIs in PMDs

[David Marchand]

On Wed, Oct 23, 2019 at 4:55 AM Joyce Kong <joyce.kong@arm.com> wrote:
>
> There are a lot functions of bit operations scattered in PMDs,
> consolidate them into a common API family and applied in different
> PMDs to reduce code duplication.
>
> v2:
>   1. Add doxygen comments for the rte bit operation API(suggested by Stephen Hemminger).
>   2. Add test cases for common rte bit operation API(suggested by Stephen Hemminger).
>   3. Change the header file to rte_io_bitops.h and the operation to rte_io_set_bit()etc.,
>      as the API uses barriers inside and the barriers are only needed for IO operations
>      (suggested by Jerin Jacob).
>   4. Use an well defined uint_NN_t type(suggested by Morten Brørup).

Thanks for working on this.
This series is a cleanup and worth looking at, yet it came rather late.

Discussion and en