[v8,1/6] lib/eal: implement the family of common bit operation APIs
Checks
Commit Message
Bitwise operation APIs are defined and used in a lot of PMDs,
which caused a huge code duplication. To reduce duplication,
this patch consolidates them into a common API family.
Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
MAINTAINERS | 4 +
doc/api/doxy-api-index.md | 5 +-
lib/librte_eal/include/meson.build | 1 +
lib/librte_eal/include/rte_bitops.h | 258 ++++++++++++++++++++++++++++
4 files changed, 266 insertions(+), 2 deletions(-)
create mode 100644 lib/librte_eal/include/rte_bitops.h
Comments
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Joyce Kong
> Sent: Thursday, April 16, 2020 7:39 AM
>
> Bitwise operation APIs are defined and used in a lot of PMDs,
> which caused a huge code duplication. To reduce duplication,
> this patch consolidates them into a common API family.
>
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Return the original bit from a 32-bit value, then set it to 1 without
> + * memory ordering.
> + *
> + * @param nr
> + * The target bit to get and set.
> + * @param addr
> + * The address holding the bit.
> + * @return
> + * The original bit.
> + */
> +__rte_experimental
> +static inline uint32_t
> +rte_test_and_set_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
> +{
> + RTE_ASSERT(nr < 32);
> +
> + uint32_t mask = UINT32_C(1) << nr;
> + uint32_t val = *addr;
> + *addr = (*addr) | mask;
Suggestion:
- *addr = (*addr) | mask;
+ *addr = val | mask;
> + return val & mask;
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Return the original bit from a 32-bit value, then clear it to 0 without
> + * memory ordering.
> + *
> + * @param nr
> + * The target bit to get and clear.
> + * @param addr
> + * The address holding the bit.
> + * @return
> + * The original bit.
> + */
> +__rte_experimental
> +static inline uint32_t
> +rte_test_and_clear_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
> +{
> + RTE_ASSERT(nr < 32);
> +
> + uint32_t mask = UINT32_C(1) << nr;
> + uint32_t val = *addr;
> + *addr = (*addr) & (~mask);
Suggestion:
- *addr = (*addr) & (~mask);
+ *addr = val & (~mask);
> + return val & mask;
> +}
> +
> +/*---------------------------- 64 bit operations -------------------------
> ---*/
> +
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Return the original bit from a 64-bit value, then set it to 1 without
> + * memory ordering.
> + *
> + * @param nr
> + * The target bit to get and set.
> + * @param addr
> + * The address holding the bit.
> + * @return
> + * The original bit.
> + */
> +__rte_experimental
> +static inline uint64_t
> +rte_test_and_set_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
> +{
> + RTE_ASSERT(nr < 64);
> +
> + uint64_t mask = UINT64_C(1) << nr;
> + uint64_t val = *addr;
> + *addr = (*addr) | mask;
Suggestion:
- *addr = (*addr) | mask;
+ *addr = val | mask;
> + return val;
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> + *
> + * Return the original bit from a 64-bit value, then clear it to 0 without
> + * memory ordering.
> + *
> + * @param nr
> + * The target bit to get and clear.
> + * @param addr
> + * The address holding the bit.
> + * @return
> + * The original bit.
> + */
> +__rte_experimental
> +static inline uint64_t
> +rte_test_and_clear_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
> +{
> + RTE_ASSERT(nr < 64);
> +
> + uint64_t mask = UINT64_C(1) << nr;
> + uint64_t val = *addr;
> + *addr = (*addr) & (~mask);
Suggestion:
- *addr = (*addr) & (~mask);
+ *addr = val & (~mask);
> + return val & mask;
> +}
> +
> +#endif /* _RTE_BITOPS_H_ */
> --
> 2.17.1
>
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Friday, April 17, 2020 2:55 AM
> To: Joyce Kong <Joyce.Kong@arm.com>; thomas@monjalon.net;
> stephen@networkplumber.org; david.marchand@redhat.com;
> jerinj@marvell.com; bruce.richardson@intel.com; ravi1.kumar@amd.com;
> rmody@marvell.com; shshaikh@marvell.com; xuanziyang2@huawei.com;
> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; Phil Yang <Phil.Yang@arm.com>
> Cc: nd <nd@arm.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v8 1/6] lib/eal: implement the family of
> commonbit operation APIs
>
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Joyce Kong
> > Sent: Thursday, April 16, 2020 7:39 AM
> >
> > Bitwise operation APIs are defined and used in a lot of PMDs, which
> > caused a huge code duplication. To reduce duplication, this patch
> > consolidates them into a common API family.
> >
>
> [...]
>
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 32-bit value, then set it to 1
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and set.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_test_and_set_bit32_relaxed(unsigned int nr, volatile uint32_t
> > +*addr) {
> > + RTE_ASSERT(nr < 32);
> > +
> > + uint32_t mask = UINT32_C(1) << nr;
> > + uint32_t val = *addr;
> > + *addr = (*addr) | mask;
>
> Suggestion:
> - *addr = (*addr) | mask;
> + *addr = val | mask;
>
Shall take the advice in v9.
Thanks,
Joyce
> > + return val & mask;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 32-bit value, then clear it to 0
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and clear.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_test_and_clear_bit32_relaxed(unsigned int nr, volatile uint32_t
> > +*addr) {
> > + RTE_ASSERT(nr < 32);
> > +
> > + uint32_t mask = UINT32_C(1) << nr;
> > + uint32_t val = *addr;
> > + *addr = (*addr) & (~mask);
>
> Suggestion:
> - *addr = (*addr) & (~mask);
> + *addr = val & (~mask);
>
Shall take the advice in v9.
> > + return val & mask;
> > +}
> > +
> > +/*---------------------------- 64 bit operations
> > +-------------------------
> > ---*/
> > +
>
> [...]
>
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 64-bit value, then set it to 1
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and set.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint64_t
> > +rte_test_and_set_bit64_relaxed(unsigned int nr, volatile uint64_t
> > +*addr) {
> > + RTE_ASSERT(nr < 64);
> > +
> > + uint64_t mask = UINT64_C(1) << nr;
> > + uint64_t val = *addr;
> > + *addr = (*addr) | mask;
>
> Suggestion:
> - *addr = (*addr) | mask;
> + *addr = val | mask;
>
Shall take the advice in v9.
> > + return val;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 64-bit value, then clear it to 0
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and clear.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint64_t
> > +rte_test_and_clear_bit64_relaxed(unsigned int nr, volatile uint64_t
> > +*addr) {
> > + RTE_ASSERT(nr < 64);
> > +
> > + uint64_t mask = UINT64_C(1) << nr;
> > + uint64_t val = *addr;
> > + *addr = (*addr) & (~mask);
>
> Suggestion:
> - *addr = (*addr) & (~mask);
> + *addr = val & (~mask);
>
Shall take the advice in v9.
> > + return val & mask;
> > +}
> > +
> > +#endif /* _RTE_BITOPS_H_ */
> > --
> > 2.17.1
> >
On Thu, Apr 16, 2020 at 11:09 AM Joyce Kong <joyce.kong@arm.com> wrote:
>
> Bitwise operation APIs are defined and used in a lot of PMDs,
> which caused a huge code duplication. To reduce duplication,
> this patch consolidates them into a common API family.
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> MAINTAINERS | 4 +
> doc/api/doxy-api-index.md | 5 +-
> lib/librte_eal/include/meson.build | 1 +
> lib/librte_eal/include/rte_bitops.h | 258 ++++++++++++++++++++++++++++
> 4 files changed, 266 insertions(+), 2 deletions(-)
> create mode 100644 lib/librte_eal/include/rte_bitops.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4800f6884..1d5fad28b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -241,6 +241,10 @@ M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> F: lib/librte_eal/include/rte_bitmap.h
> F: app/test/test_bitmap.c
>
> +Bitops
> +M: Joyce Kong <joyce.kong@arm.com>
> +F: lib/librte_eal/include/rte_bitops.h
> +
> MCSlock - EXPERIMENTAL
> M: Phil Yang <phil.yang@arm.com>
> F: lib/librte_eal/include/generic/rte_mcslock.h
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index dff496be0..b63e782ec 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -9,6 +9,7 @@ API {#index}
> The public API headers are grouped by topics:
>
> - **device**:
> + [bitops] (@ref rte_bitops.h),
IMO, It may not come in "device" section, instead existing "**basic**
section is more appropriate.
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Friday, April 17, 2020 5:39 PM
> To: Joyce Kong <Joyce.Kong@arm.com>
> Cc: thomas@monjalon.net; Stephen Hemminger
> <stephen@networkplumber.org>; David Marchand
> <david.marchand@redhat.com>; Morten Brørup
> <mb@smartsharesystems.com>; jerinj@marvell.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>;
> Rasesh Mody <rmody@marvell.com>; Shahed Shaikh
> <shshaikh@marvell.com>; Ziyang Xuan <xuanziyang2@huawei.com>;
> Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
> <zhouguoyang@huawei.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; Phil
> Yang <Phil.Yang@arm.com>; nd <nd@arm.com>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH v8 1/6] lib/eal: implement the family of
> common bit operation APIs
>
> On Thu, Apr 16, 2020 at 11:09 AM Joyce Kong <joyce.kong@arm.com> wrote:
> >
> > Bitwise operation APIs are defined and used in a lot of PMDs, which
> > caused a huge code duplication. To reduce duplication, this patch
> > consolidates them into a common API family.
> >
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > MAINTAINERS | 4 +
> > doc/api/doxy-api-index.md | 5 +-
> > lib/librte_eal/include/meson.build | 1 +
> > lib/librte_eal/include/rte_bitops.h | 258
> > ++++++++++++++++++++++++++++
> > 4 files changed, 266 insertions(+), 2 deletions(-) create mode
> > 100644 lib/librte_eal/include/rte_bitops.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 4800f6884..1d5fad28b
> > 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -241,6 +241,10 @@ M: Cristian Dumitrescu
> > <cristian.dumitrescu@intel.com>
> > F: lib/librte_eal/include/rte_bitmap.h
> > F: app/test/test_bitmap.c
> >
> > +Bitops
> > +M: Joyce Kong <joyce.kong@arm.com>
> > +F: lib/librte_eal/include/rte_bitops.h
> > +
> > MCSlock - EXPERIMENTAL
> > M: Phil Yang <phil.yang@arm.com>
> > F: lib/librte_eal/include/generic/rte_mcslock.h
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index dff496be0..b63e782ec 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -9,6 +9,7 @@ API {#index}
> > The public API headers are grouped by topics:
> >
> > - **device**:
> > + [bitops] (@ref rte_bitops.h),
>
> IMO, It may not come in "device" section, instead existing "**basic** section
> is more appropriate.
Will change to '**basic** section' in v9.
@@ -241,6 +241,10 @@ M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
F: lib/librte_eal/include/rte_bitmap.h
F: app/test/test_bitmap.c
+Bitops
+M: Joyce Kong <joyce.kong@arm.com>
+F: lib/librte_eal/include/rte_bitops.h
+
MCSlock - EXPERIMENTAL
M: Phil Yang <phil.yang@arm.com>
F: lib/librte_eal/include/generic/rte_mcslock.h
@@ -9,6 +9,7 @@ API {#index}
The public API headers are grouped by topics:
- **device**:
+ [bitops] (@ref rte_bitops.h),
[dev] (@ref rte_dev.h),
[ethdev] (@ref rte_ethdev.h),
[ethctrl] (@ref rte_eth_ctrl.h),
@@ -133,12 +134,12 @@ The public API headers are grouped by topics:
[BPF] (@ref rte_bpf.h)
- **containers**:
+ [bitmap] (@ref rte_bitmap.h),
[mbuf] (@ref rte_mbuf.h),
[mbuf pool ops] (@ref rte_mbuf_pool_ops.h),
[ring] (@ref rte_ring.h),
[stack] (@ref rte_stack.h),
- [tailq] (@ref rte_tailq.h),
- [bitmap] (@ref rte_bitmap.h)
+ [tailq] (@ref rte_tailq.h)
- **packet framework**:
* [port] (@ref rte_port.h):
@@ -6,6 +6,7 @@ includes += include_directories('.')
headers += files(
'rte_alarm.h',
'rte_bitmap.h',
+ 'rte_bitops.h',
'rte_branch_prediction.h',
'rte_bus.h',
'rte_class.h',
new file mode 100644
@@ -0,0 +1,258 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Arm Limited
+ */
+
+#ifndef _RTE_BITOPS_H_
+#define _RTE_BITOPS_H_
+
+/**
+ * @file
+ * Bit Operations
+ *
+ * This file defines a family of APIs for bit operations
+ * without enforcing memory ordering.
+ */
+
+#include <stdint.h>
+#include <rte_debug.h>
+#include <rte_compat.h>
+
+/*---------------------------- 32 bit operations ----------------------------*/
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the target bit from a 32-bit value without memory ordering.
+ *
+ * @param nr
+ * The target bit to get.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The target bit.
+ */
+__rte_experimental
+static inline uint32_t
+rte_get_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
+{
+ RTE_ASSERT(nr < 32);
+
+ uint32_t mask = UINT32_C(1) << nr;
+ return (*addr) & mask;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Set the target bit in a 32-bit value to 1 without memory ordering.
+ *
+ * @param nr
+ * The target bit to set.
+ * @param addr
+ * The address holding the bit.
+ */
+__rte_experimental
+static inline void
+rte_set_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
+{
+ RTE_ASSERT(nr < 32);
+
+ uint32_t mask = UINT32_C(1) << nr;
+ *addr = (*addr) | mask;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Clear the target bit in a 32-bit value to 0 without memory ordering.
+ *
+ * @param nr
+ * The target bit to clear.
+ * @param addr
+ * The address holding the bit.
+ */
+__rte_experimental
+static inline void
+rte_clear_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
+{
+ RTE_ASSERT(nr < 32);
+
+ uint32_t mask = UINT32_C(1) << nr;
+ *addr = (*addr) & (~mask);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Return the original bit from a 32-bit value, then set it to 1 without
+ * memory ordering.
+ *
+ * @param nr
+ * The target bit to get and set.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The original bit.
+ */
+__rte_experimental
+static inline uint32_t
+rte_test_and_set_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
+{
+ RTE_ASSERT(nr < 32);
+
+ uint32_t mask = UINT32_C(1) << nr;
+ uint32_t val = *addr;
+ *addr = (*addr) | mask;
+ return val & mask;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Return the original bit from a 32-bit value, then clear it to 0 without
+ * memory ordering.
+ *
+ * @param nr
+ * The target bit to get and clear.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The original bit.
+ */
+__rte_experimental
+static inline uint32_t
+rte_test_and_clear_bit32_relaxed(unsigned int nr, volatile uint32_t *addr)
+{
+ RTE_ASSERT(nr < 32);
+
+ uint32_t mask = UINT32_C(1) << nr;
+ uint32_t val = *addr;
+ *addr = (*addr) & (~mask);
+ return val & mask;
+}
+
+/*---------------------------- 64 bit operations ----------------------------*/
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the target bit from a 64-bit value without memory ordering.
+ *
+ * @param nr
+ * The target bit to get.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The target bit.
+ */
+__rte_experimental
+static inline uint64_t
+rte_get_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
+{
+ RTE_ASSERT(nr < 64);
+
+ uint64_t mask = UINT64_C(1) << nr;
+ return (*addr) & mask;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Set the target bit in a 64-bit value to 1 without memory ordering.
+ *
+ * @param nr
+ * The target bit to set.
+ * @param addr
+ * The address holding the bit.
+ */
+__rte_experimental
+static inline void
+rte_set_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
+{
+ RTE_ASSERT(nr < 64);
+
+ uint64_t mask = UINT64_C(1) << nr;
+ (*addr) = (*addr) | mask;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Clear the target bit in a 64-bit value to 0 without memory ordering.
+ *
+ * @param nr
+ * The target bit to clear.
+ * @param addr
+ * The address holding the bit.
+ */
+__rte_experimental
+static inline void
+rte_clear_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
+{
+ RTE_ASSERT(nr < 64);
+
+ uint64_t mask = UINT64_C(1) << nr;
+ *addr = (*addr) & (~mask);
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Return the original bit from a 64-bit value, then set it to 1 without
+ * memory ordering.
+ *
+ * @param nr
+ * The target bit to get and set.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The original bit.
+ */
+__rte_experimental
+static inline uint64_t
+rte_test_and_set_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
+{
+ RTE_ASSERT(nr < 64);
+
+ uint64_t mask = UINT64_C(1) << nr;
+ uint64_t val = *addr;
+ *addr = (*addr) | mask;
+ return val;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Return the original bit from a 64-bit value, then clear it to 0 without
+ * memory ordering.
+ *
+ * @param nr
+ * The target bit to get and clear.
+ * @param addr
+ * The address holding the bit.
+ * @return
+ * The original bit.
+ */
+__rte_experimental
+static inline uint64_t
+rte_test_and_clear_bit64_relaxed(unsigned int nr, volatile uint64_t *addr)
+{
+ RTE_ASSERT(nr < 64);
+
+ uint64_t mask = UINT64_C(1) << nr;
+ uint64_t val = *addr;
+ *addr = (*addr) & (~mask);
+ return val & mask;
+}
+
+#endif /* _RTE_BITOPS_H_ */