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

["Morten Brørup" <mb@smartsharesystems.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@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