Re: [PATCH] lib: count_zeros: fix 32/64-bit inconsistency in count_trailing_zeros()

[Enzo Matsumiya]

Hi Yury,

On 03/12, Yury Norov wrote:
> Based on 'sizeof(x) == 4' condition, in 32-bit case the function is wired
> to ffs(), while in 64-bit case to __ffs(). The difference is substantial:
> ffs(x) == __ffs(x) + 1. Also, ffs(0) == 0, while __ffs(0) is undefined.
>
> The 32-bit behaviour is inconsistent with the function description, so it
> needs to get fixed.
>
> There are 9 individual users for the function in 6 different subsystems.
> Some arches and drivers are 64-bit only:
> - arch/loongarch/kvm/intc/eiointc.c;
> - drivers/hv/mshv_vtl_main.c;
> - kernel/liveupdate/kexec_handover.c;
>
> The others are:
> - ib_umem_find_best_pgsz(): as per comment, __ffs() should be correct;
> - rzv2m_csi_reg_write_bit(): ARCH_RENESAS only, unclear;
> - lz77_match_len(): CIFS_COMPRESSION only, unclear, experimental;
>
> None of them explicitly tweak their code for a word length, or x == 0.

Context for lz77_match_len() case:

	const u64 diff = lz77_read64(cur) ^ lz77_read64(wnd);

	if (!diff) {
	...
	}

	cur += count_trailing_zeros(diff) >> 3;

So x == 0 is checked, however it does assume that
sizeof(unsigned long) == sizeof(u64).  I'll have to fix it for when
that's not the case (even with your patch in, as __ffs() casts x to
unsigned long down the line).  Thanks for the heads up.


Cheers,

Enzo

> Requesting comments from the corresponding maintainers on how to proceed
> with this.
>
> The attached patch gets rid of 32-bit explicit support, so that both
> 32- and 64-bit versions rely on __ffs().
>
> CC: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx> (hyperv)
> CC: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> (hyperv)
> CC: Jason Gunthorpe <jgg@xxxxxxxx> (infiniband)
> CC: Leon Romanovsky <leon@xxxxxxxxxx> (infiniband)
> CC: Mark Brown <broonie@xxxxxxxxxx> (spi)
> CC: Steve French <sfrench@xxxxxxxxx> (smb)
> CC: Alexander Graf <graf@xxxxxxxxxx> (kexec)
> CC: Mike Rapoport <rppt@xxxxxxxxxx> (kexec)
> CC: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> (kexec)
> Signed-off-by: Yury Norov <ynorov@xxxxxxxxxx>
> ---
> include/linux/count_zeros.h | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/count_zeros.h b/include/linux/count_zeros.h
> index 4e5680327ece..5034a30b5c7c 100644
> --- a/include/linux/count_zeros.h
> +++ b/include/linux/count_zeros.h
> @@ -10,6 +10,8 @@
>
> #include <asm/bitops.h>
>
> +#define COUNT_TRAILING_ZEROS_0 (-1)
> +
> /**
>  * count_leading_zeros - Count the number of zeros from the MSB back
>  * @x: The value
> @@ -40,12 +42,7 @@ static inline int count_leading_zeros(unsigned long x)
>  */
> static inline int count_trailing_zeros(unsigned long x)
> {
> -#define COUNT_TRAILING_ZEROS_0 (-1)
> -
> -	if (sizeof(x) == 4)
> -		return ffs(x);
> -	else
> -		return (x != 0) ? __ffs(x) : COUNT_TRAILING_ZEROS_0;
> +	return (x != 0) ? __ffs(x) : COUNT_TRAILING_ZEROS_0;
> }
>
> #endif /* _LINUX_BITOPS_COUNT_ZEROS_H_ */
> --
> 2.43.0