* Re: [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-12 22:02 [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion Andre Muezerie
@ 2024-11-13 3:12 ` Honnappa Nagarahalli
2024-11-13 3:31 ` Morten Brørup
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Honnappa Nagarahalli @ 2024-11-13 3:12 UTC (permalink / raw)
To: Andre Muezerie; +Cc: dev, nd
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-12 22:02 [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion Andre Muezerie
2024-11-13 3:12 ` Honnappa Nagarahalli
@ 2024-11-13 3:31 ` Morten Brørup
2024-11-13 16:23 ` [PATCH v2] " Andre Muezerie
2024-11-15 15:25 ` [PATCH v3] " Andre Muezerie
3 siblings, 0 replies; 8+ messages in thread
From: Morten Brørup @ 2024-11-13 3:31 UTC (permalink / raw)
To: Andre Muezerie, Honnappa Nagarahalli; +Cc: dev
> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-12 22:02 [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion Andre Muezerie
2024-11-13 3:12 ` Honnappa Nagarahalli
2024-11-13 3:31 ` Morten Brørup
@ 2024-11-13 16:23 ` Andre Muezerie
2024-11-15 14:21 ` David Marchand
2024-11-15 15:25 ` [PATCH v3] " Andre Muezerie
3 siblings, 1 reply; 8+ messages in thread
From: Andre Muezerie @ 2024-11-13 16:23 UTC (permalink / raw)
To: dev; +Cc: honnappa.nagarahalli, doug.foster, 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-13 16:23 ` [PATCH v2] " Andre Muezerie
@ 2024-11-15 14:21 ` David Marchand
2024-11-15 14:45 ` Andre Muezerie
0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2024-11-15 14:21 UTC (permalink / raw)
To: Andre Muezerie; +Cc: dev, honnappa.nagarahalli, doug.foster
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-15 14:21 ` David Marchand
@ 2024-11-15 14:45 ` Andre Muezerie
0 siblings, 0 replies; 8+ messages in thread
From: Andre Muezerie @ 2024-11-15 14:45 UTC (permalink / raw)
To: David Marchand; +Cc: dev, honnappa.nagarahalli, doug.foster
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-12 22:02 [PATCH] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion Andre Muezerie
` (2 preceding siblings ...)
2024-11-13 16:23 ` [PATCH v2] " Andre Muezerie
@ 2024-11-15 15:25 ` Andre Muezerie
2024-11-15 16:18 ` Stephen Hemminger
3 siblings, 1 reply; 8+ messages in thread
From: Andre Muezerie @ 2024-11-15 15:25 UTC (permalink / raw)
To: dev; +Cc: honnappa.nagarahalli, doug.foster, david.marchand, 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
2024-11-15 15:25 ` [PATCH v3] " Andre Muezerie
@ 2024-11-15 16:18 ` Stephen Hemminger
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2024-11-15 16:18 UTC (permalink / raw)
To: Andre Muezerie; +Cc: dev, honnappa.nagarahalli, doug.foster, david.marchand
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?
^ permalink raw reply [flat|nested] 8+ messages in thread