linux-kernel - Re: [PATCH v3 next 08/10] lib: mul_u64_u64_div_u64() Separate multiply to a helper for clarity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <58porr76-92os-7019-nr00-n68r74202pps@onlyvoer.pbz>
Date: Sat, 14 Jun 2025 11:37:54 -0400 (EDT)
From: Nicolas Pitre <npitre@...libre.com>
To: David Laight <david.laight.linux@...il.com>
cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
    u.kleine-koenig@...libre.com, Oleg Nesterov <oleg@...hat.com>, 
    Peter Zijlstra <peterz@...radead.org>, 
    Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH v3 next 08/10] lib: mul_u64_u64_div_u64() Separate multiply
 to a helper for clarity

On Sat, 14 Jun 2025, David Laight wrote:

> Move the 64x64 => 128 multiply into a static inline helper function
> for code clarity.
> No need for the a/b_hi/lo variables, the implicit casts on the function
> calls do the work for us.
> Should have minimal effect on the generated code.
> 
> Signed-off-by: David Laight <david.laight.linux@...il.com>
> ---
> 
> new patch for v3.
> 
>  lib/math/div64.c | 54 +++++++++++++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/math/div64.c b/lib/math/div64.c
> index 2ac7e25039a1..fb77fd9d999d 100644
> --- a/lib/math/div64.c
> +++ b/lib/math/div64.c
> @@ -193,42 +193,48 @@ static u64 mul_add(u32 a, u32 b, u32 c)
>  	return add_u64_u32(mul_u32_u32(a, b), c);
>  }
>  
> -u64 mul_u64_add_u64_div_u64(u64 a, u64 b, u64 c, u64 d)
> -{
> -	if (WARN_ONCE(!d, "%s: division of (%#llx * %#llx + %#llx) by zero, returning 0",
> -		      __func__, a, b, c)) {
> -		/*
> -		 * Return 0 (rather than ~(u64)0) because it is less likely to
> -		 * have unexpected side effects.
> -		 */
> -		return 0;
> -	}
> -
>  #if defined(__SIZEOF_INT128__) && !defined(test_mul_u64_add_u64_div_u64)
> -
> +static inline u64 mul_u64_u64_add_u64(u64 *p_lo, u64 a, u64 b, u64 c)

Why not move the #if inside the function body and have only one function 
definition?



> +{
>  	/* native 64x64=128 bits multiplication */
>  	u128 prod = (u128)a * b + c;
> -	u64 n_lo = prod, n_hi = prod >> 64;
>  
> -#else
> +	*p_lo = prod;
> +	return prod >> 64;
> +}
>  
> -	/* perform a 64x64=128 bits multiplication manually */
> -	u32 a_lo = a, a_hi = a >> 32, b_lo = b, b_hi = b >> 32;
> +#else
> +static inline u64 mul_u64_u64_add_u64(u64 *p_lo, u64 a, u64 b, u64 c)
> +{
> +	/* perform a 64x64=128 bits multiplication in 32bit chunks */
>  	u64 x, y, z;
>  
>  	/* Since (x-1)(x-1) + 2(x-1) == x.x - 1 two u32 can be added to a u64 */
> -	x = mul_add(a_lo, b_lo, c);
> -	y = mul_add(a_lo, b_hi, c >> 32);
> +	x = mul_add(a, b, c);
> +	y = mul_add(a, b >> 32, c >> 32);
>  	y = add_u64_u32(y, x >> 32);
> -	z = mul_add(a_hi, b_hi, y >> 32);
> -	y = mul_add(a_hi, b_lo, y);
> -	z = add_u64_u32(z, y >> 32);
> -	x = (y << 32) + (u32)x;
> -
> -	u64 n_lo = x, n_hi = z;
> +	z = mul_add(a >> 32, b >> 32, y >> 32);
> +	y = mul_add(a >> 32, b, y);
> +	*p_lo = (y << 32) + (u32)x;
> +	return add_u64_u32(z, y >> 32);
> +}
>  
>  #endif
>  
> +u64 mul_u64_add_u64_div_u64(u64 a, u64 b, u64 c, u64 d)
> +{
> +	u64 n_lo, n_hi;
> +
> +	if (WARN_ONCE(!d, "%s: division of (%llx * %llx + %llx) by zero, returning 0",
> +		      __func__, a, b, c )) {
> +		/*
> +		 * Return 0 (rather than ~(u64)0) because it is less likely to
> +		 * have unexpected side effects.
> +		 */
> +		return 0;
> +	}
> +
> +	n_hi = mul_u64_u64_add_u64(&n_lo, a, b, c);
>  	if (!n_hi)
>  		return div64_u64(n_lo, d);
>  
> -- 
> 2.39.5
> 
>