[dpdk-dev] [PATCH v8 1/6] lib/eal: implement the family of commonbit operation APIs

[Joyce Kong]

> -----Original Message-----
> From: Morten Brørup <mb at smartsharesystems.com>
> Sent: Friday, April 17, 2020 2:55 AM
> To: Joyce Kong <Joyce.Kong at arm.com>; thomas at monjalon.net;
> stephen at networkplumber.org; david.marchand at redhat.com;
> jerinj at marvell.com; bruce.richardson at intel.com; ravi1.kumar at amd.com;
> rmody at marvell.com; shshaikh at marvell.com; xuanziyang2 at huawei.com;
> cloud.wangxiaoyun at huawei.com; zhouguoyang at huawei.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli at arm.com>; Gavin Hu
> <Gavin.Hu at arm.com>; Phil Yang <Phil.Yang at arm.com>
> Cc: nd <nd at arm.com>; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v8 1/6] lib/eal: implement the family of
> commonbit operation APIs
> 
> > From: dev [mailto:dev-bounces at 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
> >