rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) ../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) ../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

These warnings are being issued by the MSVC compiler. Since the result is being stored in a variable of type uint64_t, it makes sense to shift a 64-bit number instead of shifting a 32-bit number and then having the compiler to convert the result implicitly to 64 bits. UINT64_C was used in the fix as it is the portable way to define a 64-bit constant (ULL suffix is architecture dependent).

Signed-off-by Andre Muezerie <[email protected]>

---
lib/rcu/rte_rcu_qsbr.c | 12 ++++++------

1 file changed, 6 insertions(+), 6 deletions(-)

lib/rcu/rte_rcu_qsbr.c +5 -5

index 09a14a15f1..1d19d1dc95 100644
@@ -99,12 +99,12 @@ rte_rcu_qsbr_thread_register(struct rte_rcu_qsbr *v, unsigned int thread_id)
 
 /* Add the thread to the bitmap of registered threads */
 old_bmap = rte_atomic_fetch_or_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
- (1UL << id), rte_memory_order_release);
+ (UINT64_C(1) << id), rte_memory_order_release);
 
 /* Increment the number of threads registered only if the thread was not already
 * registered
 */
- if (!(old_bmap & (1UL << id)))
+ if (!(old_bmap & (UINT64_C(1) << id)))
 rte_atomic_fetch_add_explicit(&v->num_threads, 1, rte_memory_order_relaxed);
 
 return 0;

@@ -137,12 +137,12 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id)
 * reporting threads.
 */
 old_bmap = rte_atomic_fetch_and_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
- ~(1UL << id), rte_memory_order_release);
+ ~(UINT64_C(1) << id), rte_memory_order_release);
 
 /* Decrement the number of threads unregistered only if the thread was not already
 * unregistered
 */
- if (old_bmap & (1UL << id))
+ if (old_bmap & (UINT64_C(1) << id))
 rte_atomic_fetch_sub_explicit(&v->num_threads, 1, rte_memory_order_relaxed);
 
 return 0;

@@ -198,7 +198,7 @@ rte_rcu_qsbr_dump(FILE *f, struct rte_rcu_qsbr *v)
 t = rte_ctz64(bmap);
 fprintf(f, "%u ", id + t);
 
- bmap &= ~(1UL << t);
+ bmap &= ~(UINT64_C(1) << t);
 }
 }
 

@@ -225,7 +225,7 @@ rte_rcu_qsbr_dump(FILE *f, struct rte_rcu_qsbr *v)

rte_atomic_load_explicit(
&v->qsbr_cnt[id + t].lock_cnt,
rte_memory_order_relaxed));
- bmap &= ~(1UL << t);
+ bmap &= ~(UINT64_C(1) << t);
}
}

Signature · 1 line

2.34.1

From: Andre Muezerie [mailto:[email protected]]
Sent: Tuesday, 12 November 2024 23.03

../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)

These warnings are being issued by the MSVC compiler. Since the result is
being stored in a variable of type uint64_t, it makes sense to shift a 64-bit number instead of shifting a 32-bit number and then having the compiler to convert the result implicitly to 64 bits. UINT64_C was used in the fix as it is the portable way to define a 64- bit
constant (ULL suffix is architecture dependent).

Signed-off-by Andre Muezerie <[email protected]>

---

../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)

These warnings are being issued by the MSVC compiler. Since the result is being stored in a variable of type uint64_t, it makes sense to shift a 64-bit number instead of shifting a 32-bit number and then having the compiler to convert the result implicitly to 64 bits. UINT64_C was used in the fix as it is the portable way to define a 64-bit constant (ULL suffix is architecture dependent).

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.

Fixes 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")

Signed-off-by Andre Muezerie <[email protected]>

Reviewed-by Honnappa Nagarahalli <[email protected]>

Reviewed-by Morten Brørup <[email protected]>

Quoted 18 lines · Andre Muezerie

../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)

These warnings are being issued by the MSVC compiler. Since the result is being stored in a variable of type uint64_t, it makes sense to shift a 64-bit number instead of shifting a 32-bit number and then having the compiler to convert the result implicitly to 64 bits. UINT64_C was used in the fix as it is the portable way to define a 64-bit constant (ULL suffix is architecture dependent).

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.

Quoted 5 lines

Fixes 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")

Signed-off-by Andre Muezerie <[email protected]>

Reviewed-by Honnappa Nagarahalli <[email protected]>

Reviewed-by Morten Brørup <[email protected]>

Just a nit, EAL provides a macro:
lib/eal/include/rte_bitops.h:#define RTE_BIT64(nr) (UINT64_C(1) << (nr))

Does this change allow to build the rcu library with MSVC? I see it is disabled in meson.

Signature · 2 lines

-- 
David Marchand

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.

Quoted 2 lines · On Fri, 15 Nov 2024 07:25:30 -0800 Andre Muezerie…

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.

Since this seems to be a common anti-pattern in DPDK,
I wonder if coccinelle is smart enough to be able to make a script for these?

From: Thomas Monjalon [mailto:[email protected]]
Sent: Tuesday, 19 November 2024 10.26

15/11/2024 17:18, Stephen Hemminger:

Quoted 5 lines

Quoted 2 lines · On Fri, 15 Nov 2024 07:25:30 -0800 Andre Muezerie…

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id >

0x1f.

Quoted 3 lines

Since this seems to be a common anti-pattern in DPDK,
I wonder if coccinelle is smart enough to be able to make a script

for these?

A simple grep '1UL << ' would do it as well.

We could add a checkpatch warning for this pattern.

../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)

These warnings are being issued by the MSVC compiler. Since the result is being stored in a variable of type uint64_t, it makes sense to shift a 64-bit number instead of shifting a 32-bit number and then having the compiler to convert the result implicitly to 64 bits. UINT64_C was used in the fix as it is the portable way to define a 64-bit constant (ULL suffix is architecture dependent).

From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.

Fixes 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")

Signed-off-by Andre Muezerie <[email protected]>

Reviewed-by Honnappa Nagarahalli <[email protected]>

Reviewed-by Morten Brørup <[email protected]>