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

[Joyce Kong (Arm Technology China)]

Hi Morten,

> -----Original Message-----
> From: Morten Brørup <m...@smartsharesystems.com>
> Sent: Monday, November 18, 2019 6:52 PM
> To: Joyce Kong (Arm Technology China) <joyce.k...@arm.com>;
> dev@dpdk.org
> Cc: nd <n...@arm.com>; tho...@monjalon.net; jer...@marvell.com;
> step...@networkplumber.org; david.march...@redhat.com; Honnappa
> Nagarahalli <honnappa.nagaraha...@arm.com>; Gavin Hu (Arm Technology
> China) <gavin...@arm.com>; ravi1.ku...@amd.com; rm...@marvell.com;
> shsha...@marvell.com; xuanziya...@huawei.com;
> cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com
> Subject: RE: [dpdk-dev] [PATCH v3 1/6] lib/eal: implement the family of rte
> bitoperation APIs
> 
> > -----Original Message-----
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Joyce Kong
> > Sent: Monday, November 18, 2019 11:07 AM
> >
> 
> [snip]
> 
> > +++ b/lib/librte_eal/common/include/rte_bitops.h
> > @@ -0,0 +1,474 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 Arm Limited
> > + */
> > +
> > +#ifndef _RTE_BITOPS_H_
> > +#define _RTE_BITOPS_H_
> > +
> > +/**
> > + * @file
> > + * Bit Operations
> > + *
> > + * This file defines a API for bit operations without/with memory
> > ordering.
> > + */
> > +
> > +#include <stdint.h>
> > +#include <assert.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, unsigned long *addr) {
> > +   assert(nr < 32);
> > +
> > +   uint32_t mask = 1UL << nr;
> > +   return __atomic_load_n(addr, __ATOMIC_RELAXED) & mask; }
> 
> Address pointer should be: uint32_t *addr.
> Likewise in the other 32 bit functions.
> 
> Use RTE_ASSERT() instead of assert().
> Likewise in all other functions.
> 
> When setting the mask, consider using UINT32_C(1) from <stdint.h> instead
> of 1UL.
> 
> [snip]
> 
> > +
> > +/*---------------------------- 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, unsigned long *addr) {
> > +   assert(nr < 64);
> > +
> > +   uint64_t mask = 1UL << nr;
> > +   return __atomic_load_n(addr, __ATOMIC_RELAXED) & mask; }
> 
> Address pointer should be: uint64_t *addr.
> Likewise in the other 64 bit functions.
> 
> Mask should be 1ULL, not 1UL. Or use UINT64_C(1) from <stdint.h> instead.
> Likewise in the other 64 bit functions.
> 
> [snip]
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup
> 
> 
Thanks! I shall address above comments in Patch v4 for both 32-bit and 64-bit 
functions.