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: <20250614223006.20c16642@pumpkin>
Date: Sat, 14 Jun 2025 22:30:06 +0100
From: David Laight <david.laight.linux@...il.com>
To: Nicolas Pitre <npitre@...libre.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 11:37:54 -0400 (EDT)
Nicolas Pitre <npitre@...libre.com> wrote:

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

Because I think it is easier to read with two definitions,
especially when the bodies are entirely different.

	David

> > +{
> >  	/* 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
> > 
> >