[RFC,7/7] eal: deprecate relaxed family of bit operations

Message ID 20240302135328.531940-8-mattias.ronnblom@ericsson.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series Improve EAL bit operations API |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build fail github build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-amd64-testing fail Testing issues
ci/iol-compile-amd64-testing fail Testing issues
ci/iol-unit-arm64-testing fail Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-compile-arm64-testing fail Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Mattias Rönnblom March 2, 2024, 1:53 p.m. UTC
  Informally (by means of documentation) deprecate the
rte_bit_relaxed_*() family of bit-level operations.

rte_bit_relaxed_*() has been replaced by three new families of
bit-level query and manipulation functions.

rte_bit_relaxed_*() failed to deliver the atomicity guarantees their
name suggested. If deprecated, it will encourage the user to consider
whether the actual, implemented behavior (e.g., non-atomic
test-and-set with read/write-once semantics) or the semantics implied
by their names (i.e., atomic), or something else, is what's actually
needed.

Bugzilla ID: 1385

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/include/rte_bitops.h | 48 ++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
  

Comments

Stephen Hemminger March 2, 2024, 5:07 p.m. UTC | #1
On Sat, 2 Mar 2024 14:53:28 +0100
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> index b5a9df5930..783dd0e1ee 100644
> --- a/lib/eal/include/rte_bitops.h
> +++ b/lib/eal/include/rte_bitops.h
> @@ -1179,6 +1179,10 @@ __RTE_GEN_BIT_ATOMIC_OPS(64)
>   *   The address holding the bit.
>   * @return
>   *   The target bit.
> + * @note
> + *   This function is deprecated. Use rte_bit_test32(),
> + *   rte_bit_once_test32(), or rte_bit_atomic_test32() instead,
> + *   depending on exactly what guarantees are required.
>   */
>  static inline uint32_t
>  rte_bit_relaxed_get32(unsigned int nr, volatile uint32_t *addr)

The DPDK process is:
	- mark these as deprecated in release notes of release N.
	- mark these as deprecated using __rte_deprecated in next LTS
	- drop these in LTS release after that.

Don't use notes for this.
  
Mattias Rönnblom March 3, 2024, 6:30 a.m. UTC | #2
On 2024-03-02 18:07, Stephen Hemminger wrote:
> On Sat, 2 Mar 2024 14:53:28 +0100
> Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> 
>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>> index b5a9df5930..783dd0e1ee 100644
>> --- a/lib/eal/include/rte_bitops.h
>> +++ b/lib/eal/include/rte_bitops.h
>> @@ -1179,6 +1179,10 @@ __RTE_GEN_BIT_ATOMIC_OPS(64)
>>    *   The address holding the bit.
>>    * @return
>>    *   The target bit.
>> + * @note
>> + *   This function is deprecated. Use rte_bit_test32(),
>> + *   rte_bit_once_test32(), or rte_bit_atomic_test32() instead,
>> + *   depending on exactly what guarantees are required.
>>    */
>>   static inline uint32_t
>>   rte_bit_relaxed_get32(unsigned int nr, volatile uint32_t *addr)
> 
> The DPDK process is:
> 	- mark these as deprecated in release notes of release N.
> 	- mark these as deprecated using __rte_deprecated in next LTS
> 	- drop these in LTS release after that.
> 
> Don't use notes for this.

Don't use notes to replace the above process, or don't use notes at all?

A note seems useful to me, especially considering there is a choice to 
be made (not just mindlessly replacing one call with another).

Anyway, release notes updates have to wait so I'll just drop this patch 
for now.
  

Patch

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index b5a9df5930..783dd0e1ee 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -1179,6 +1179,10 @@  __RTE_GEN_BIT_ATOMIC_OPS(64)
  *   The address holding the bit.
  * @return
  *   The target bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_test32(),
+ *   rte_bit_once_test32(), or rte_bit_atomic_test32() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline uint32_t
 rte_bit_relaxed_get32(unsigned int nr, volatile uint32_t *addr)
@@ -1196,6 +1200,10 @@  rte_bit_relaxed_get32(unsigned int nr, volatile uint32_t *addr)
  *   The target bit to set.
  * @param addr
  *   The address holding the bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_set32(),
+ *   rte_bit_once_set32(), or rte_bit_atomic_set32() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline void
 rte_bit_relaxed_set32(unsigned int nr, volatile uint32_t *addr)
@@ -1213,6 +1221,10 @@  rte_bit_relaxed_set32(unsigned int nr, volatile uint32_t *addr)
  *   The target bit to clear.
  * @param addr
  *   The address holding the bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_clear32(),
+ *   rte_bit_once_clear32(), or rte_bit_atomic_clear32() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline void
 rte_bit_relaxed_clear32(unsigned int nr, volatile uint32_t *addr)
@@ -1233,6 +1245,12 @@  rte_bit_relaxed_clear32(unsigned int nr, volatile uint32_t *addr)
  *   The address holding the bit.
  * @return
  *   The original bit.
+ * @note
+ *   This function is deprecated and replaced by
+ *   rte_bit_atomic_test_and_set32(), for use cases where the
+ *   operation needs to be atomic. For non-atomic/non-ordered use
+ *   cases, use rte_bit_test32() + rte_bit_set32() or
+ *   rte_bit_once_test32() + rte_bit_once_set32().
  */
 static inline uint32_t
 rte_bit_relaxed_test_and_set32(unsigned int nr, volatile uint32_t *addr)
@@ -1255,6 +1273,12 @@  rte_bit_relaxed_test_and_set32(unsigned int nr, volatile uint32_t *addr)
  *   The address holding the bit.
  * @return
  *   The original bit.
+ * @note
+ *   This function is deprecated and replaced by
+ *   rte_bit_atomic_test_and_clear32(), for use cases where the
+ *   operation needs to be atomic. For non-atomic/non-ordered use
+ *   cases, use rte_bit_test32() + rte_bit_clear32() or
+ *   rte_bit_once_test32() + rte_bit_once_clear32().
  */
 static inline uint32_t
 rte_bit_relaxed_test_and_clear32(unsigned int nr, volatile uint32_t *addr)
@@ -1278,6 +1302,10 @@  rte_bit_relaxed_test_and_clear32(unsigned int nr, volatile uint32_t *addr)
  *   The address holding the bit.
  * @return
  *   The target bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_test64(),
+ *   rte_bit_once_test64(), or rte_bit_atomic_test64() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline uint64_t
 rte_bit_relaxed_get64(unsigned int nr, volatile uint64_t *addr)
@@ -1295,6 +1323,10 @@  rte_bit_relaxed_get64(unsigned int nr, volatile uint64_t *addr)
  *   The target bit to set.
  * @param addr
  *   The address holding the bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_set64(),
+ *   rte_bit_once_set64(), or rte_bit_atomic_set64() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline void
 rte_bit_relaxed_set64(unsigned int nr, volatile uint64_t *addr)
@@ -1312,6 +1344,10 @@  rte_bit_relaxed_set64(unsigned int nr, volatile uint64_t *addr)
  *   The target bit to clear.
  * @param addr
  *   The address holding the bit.
+ * @note
+ *   This function is deprecated. Use rte_bit_clear64(),
+ *   rte_bit_once_clear64(), or rte_bit_atomic_clear64() instead,
+ *   depending on exactly what guarantees are required.
  */
 static inline void
 rte_bit_relaxed_clear64(unsigned int nr, volatile uint64_t *addr)
@@ -1332,6 +1368,12 @@  rte_bit_relaxed_clear64(unsigned int nr, volatile uint64_t *addr)
  *   The address holding the bit.
  * @return
  *   The original bit.
+ * @note
+ *   This function is deprecated and replaced by
+ *   rte_bit_atomic_test_and_set64(), for use cases where the
+ *   operation needs to be atomic. For non-atomic/non-ordered use
+ *   cases, use rte_bit_test64() + rte_bit_set64() or
+ *   rte_bit_once_test64() + rte_bit_once_set64().
  */
 static inline uint64_t
 rte_bit_relaxed_test_and_set64(unsigned int nr, volatile uint64_t *addr)
@@ -1354,6 +1396,12 @@  rte_bit_relaxed_test_and_set64(unsigned int nr, volatile uint64_t *addr)
  *   The address holding the bit.
  * @return
  *   The original bit.
+ * @note
+ *   This function is deprecated and replaced by
+ *   rte_bit_atomic_test_and_clear64(), for use cases where the
+ *   operation needs to be atomic. For non-atomic/non-ordered use
+ *   cases, use rte_bit_test64() + rte_bit_clear64() or
+ *   rte_bit_once_test64() + rte_bit_once_clear64().
  */
 static inline uint64_t
 rte_bit_relaxed_test_and_clear64(unsigned int nr, volatile uint64_t *addr)