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

[Andre Muezerie <andremue@linux.microsoft.com>]

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