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

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

[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).

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/rcu/rte_rcu_qsbr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index 09a14a15f1..1d19d1dc95 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -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);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[Honnappa Nagarahalli]

Thank you Andre for the patch. It looks good overall.

> On Nov 12, 2024, at 4:02 PM, Andre Muezerie <andremue@linux.microsoft.com> wrote:
> 
> ../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 <andremue@linux.microsoft.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
> lib/rcu/rte_rcu_qsbr.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
> index 09a14a15f1..1d19d1dc95 100644
> --- a/lib/rcu/rte_rcu_qsbr.c
> +++ b/lib/rcu/rte_rcu_qsbr.c
> @@ -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);
> }
> }
> 
> -- 
> 2.34.1
> 

RE: [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[Morten Brørup]

> From: Andre Muezerie [mailto:andremue@linux.microsoft.com]
> 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 <andremue@linux.microsoft.com>
> ---

As far as I can see, this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was incorrect when thread_id > 0x1f.

Please add a Fixes tag, so backporting can be considered.

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[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.

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

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/rcu/rte_rcu_qsbr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index 40d7c566c8..8805903c94 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -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);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[David Marchand]

On Wed, Nov 13, 2024 at 5:24 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> ../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 <andremue@linux.microsoft.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

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.


-- 
David Marchand

Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[Andre Muezerie]

On Fri, Nov 15, 2024 at 03:21:42PM +0100, David Marchand wrote:
> On Wed, Nov 13, 2024 at 5:24 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > ../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 <andremue@linux.microsoft.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 
> Just a nit, EAL provides a macro:
> lib/eal/include/rte_bitops.h:#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> 

Ah, nice! Thanks for letting me know. I'll update the patch.

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

You're correct. The rcu is currently not being built for MSVC. I'm fixing the issues and want to include rcu in the MSVC built. I'm doing the same for other parts of the DPDK code and I appreciate the help in reviewing the changes.

Thanks!

Andre Muezerie

> 
> 
> -- 
> David Marchand

[PATCH v3] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[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.

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

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/rcu/rte_rcu_qsbr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index 40d7c566c8..dbf31501a6 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -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);
+						RTE_BIT64(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 & RTE_BIT64(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);
+						 ~RTE_BIT64(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 & RTE_BIT64(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 &= ~RTE_BIT64(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 &= ~RTE_BIT64(t);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v3] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion

[Stephen Hemminger]

On Fri, 15 Nov 2024 07:25:30 -0800
Andre Muezerie <andremue@linux.microsoft.com> wrote:

> 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?