math.h: Account for 64-bit division on i386

[RFC PATCH] math.h: Account for 64-bit division on i386

Posted by cem@kernel.org 7 months, 4 weeks ago

From: Carlos Maiolino <cem@kernel.org>

Building linux on i386 might fail if a 64bit type is passed to
mult_fract(). To prevent the failure, use do_div() for the division
calculation instead of hardcoding a / b.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
---

I'm sending it as a RFC because I didn't to extensive testing on this
patch, also I'm not sure if mult_frac() was intended to work on 32-bit
only types. If that's the case, perhaps, a new mult_frac64() might be a
better idea?!

 include/linux/math.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/math.h b/include/linux/math.h
index 0198c92cbe3e..05ea853b75b4 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
 #undef __STRUCT_FRACT
 
 /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
-#define mult_frac(x, n, d)	\
-({				\
-	typeof(x) x_ = (x);	\
-	typeof(n) n_ = (n);	\
-	typeof(d) d_ = (d);	\
-				\
-	typeof(x_) q = x_ / d_;	\
-	typeof(x_) r = x_ % d_;	\
-	q * n_ + r * n_ / d_;	\
+#define mult_frac(x, n, d)		\
+({					\
+	typeof(x) x_ = (x);		\
+	typeof(n) n_ = (n);		\
+	typeof(d) d_ = (d);		\
+					\
+	typeof(x_) r = do_div(x_, d_);	\
+	r *= n_;			\
+	do_div(r, d_);			\
+	x_ * n_ + r;			\
 })
 
 #define sector_div(a, b) do_div(a, b)
-- 
2.49.0

Re: [RFC PATCH] math.h: Account for 64-bit division on i386

Posted by Guenter Roeck 7 months, 3 weeks ago

On Sat, Apr 19, 2025 at 01:51:46PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Building linux on i386 might fail if a 64bit type is passed to

i386 actually builds. Its compiler is probably able to convert
the offending mult_frac() without helpers since the divisor is
a constant. I see the problem with openrisc and parisc, with
gcc 13.3.0.

> mult_fract(). To prevent the failure, use do_div() for the division
> calculation instead of hardcoding a / b.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
> ---
> 
> I'm sending it as a RFC because I didn't to extensive testing on this
> patch, also I'm not sure if mult_frac() was intended to work on 32-bit
> only types. If that's the case, perhaps, a new mult_frac64() might be a
> better idea?!
> 
>  include/linux/math.h | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/math.h b/include/linux/math.h
> index 0198c92cbe3e..05ea853b75b4 100644
> --- a/include/linux/math.h
> +++ b/include/linux/math.h
> @@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
>  #undef __STRUCT_FRACT
>  
>  /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> -#define mult_frac(x, n, d)	\
> -({				\
> -	typeof(x) x_ = (x);	\
> -	typeof(n) n_ = (n);	\
> -	typeof(d) d_ = (d);	\
> -				\
> -	typeof(x_) q = x_ / d_;	\
> -	typeof(x_) r = x_ % d_;	\
> -	q * n_ + r * n_ / d_;	\
> +#define mult_frac(x, n, d)		\
> +({					\
> +	typeof(x) x_ = (x);		\
> +	typeof(n) n_ = (n);		\
> +	typeof(d) d_ = (d);		\
> +					\
> +	typeof(x_) r = do_div(x_, d_);	\
> +	r *= n_;			\
> +	do_div(r, d_);			\
> +	x_ * n_ + r;			\
>  })
>  

Unfortunately that doesn't work. I get build errors on parisc.

In file included from ./arch/parisc/include/generated/asm/div64.h:1,
                 from ./include/linux/math.h:6,
                 from ./include/linux/kernel.h:27,
                 from ./arch/parisc/include/asm/bug.h:5,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/mmdebug.h:5,
                 from ./include/linux/mm.h:6,
                 from mm/page_alloc.c:19:
mm/page_alloc.c: In function 'boost_watermark':
./include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror]
  183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
      |                                   ^~
./include/linux/math.h:142:24: note: in expansion of macro 'do_div'
  142 |         typeof(x_) r = do_div(x_, d_);  \
      |                        ^~~~~~
mm/page_alloc.c:2010:21: note: in expansion of macro 'mult_frac'
 2010 |         max_boost = mult_frac(zone->_watermark[WMARK_HIGH],

That is just one example. It seems to affect all uses of
mult_frac().

Guenter

Re: [RFC PATCH] math.h: Account for 64-bit division on i386

Posted by Carlos Maiolino 7 months, 3 weeks ago

On Sun, Apr 20, 2025 at 08:40:25AM -0700, Guenter Roeck wrote:
> On Sat, Apr 19, 2025 at 01:51:46PM +0200, cem@kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> >
> > Building linux on i386 might fail if a 64bit type is passed to
> 
> i386 actually builds. Its compiler is probably able to convert
> the offending mult_frac() without helpers since the divisor is
> a constant. I see the problem with openrisc and parisc, with
> gcc 13.3.0.

It does fail to build, that's how it got identified in the first place. I
managed to reproduce here according to the reproducer sent by the test robot.
I think it works if you don't enable build checks (in this case W=1) though.

I didn't try other architectures other than x86 though, thanks for the heads up.


> 
> > mult_fract(). To prevent the failure, use do_div() for the division
> > calculation instead of hardcoding a / b.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
> > ---
> >
> > I'm sending it as a RFC because I didn't to extensive testing on this
> > patch, also I'm not sure if mult_frac() was intended to work on 32-bit
> > only types. If that's the case, perhaps, a new mult_frac64() might be a
> > better idea?!
> >
> >  include/linux/math.h | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/math.h b/include/linux/math.h
> > index 0198c92cbe3e..05ea853b75b4 100644
> > --- a/include/linux/math.h
> > +++ b/include/linux/math.h
> > @@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
> >  #undef __STRUCT_FRACT
> >
> >  /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> > -#define mult_frac(x, n, d)	\
> > -({				\
> > -	typeof(x) x_ = (x);	\
> > -	typeof(n) n_ = (n);	\
> > -	typeof(d) d_ = (d);	\
> > -				\
> > -	typeof(x_) q = x_ / d_;	\
> > -	typeof(x_) r = x_ % d_;	\
> > -	q * n_ + r * n_ / d_;	\
> > +#define mult_frac(x, n, d)		\
> > +({					\
> > +	typeof(x) x_ = (x);		\
> > +	typeof(n) n_ = (n);		\
> > +	typeof(d) d_ = (d);		\
> > +					\
> > +	typeof(x_) r = do_div(x_, d_);	\
> > +	r *= n_;			\
> > +	do_div(r, d_);			\
> > +	x_ * n_ + r;			\
> >  })
> >
> 
> Unfortunately that doesn't work. I get build errors on parisc.
> 
> In file included from ./arch/parisc/include/generated/asm/div64.h:1,
>                  from ./include/linux/math.h:6,
>                  from ./include/linux/kernel.h:27,
>                  from ./arch/parisc/include/asm/bug.h:5,
>                  from ./include/linux/bug.h:5,
>                  from ./include/linux/mmdebug.h:5,
>                  from ./include/linux/mm.h:6,
>                  from mm/page_alloc.c:19:
> mm/page_alloc.c: In function 'boost_watermark':
> ./include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror]
>   183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
>       |                                   ^~
> ./include/linux/math.h:142:24: note: in expansion of macro 'do_div'
>   142 |         typeof(x_) r = do_div(x_, d_);  \
>       |                        ^~~~~~
> mm/page_alloc.c:2010:21: note: in expansion of macro 'mult_frac'
>  2010 |         max_boost = mult_frac(zone->_watermark[WMARK_HIGH],
> 
> That is just one example. It seems to affect all uses of
> mult_frac().
> 
> Guenter

Re: [RFC PATCH] math.h: Account for 64-bit division on i386

Posted by Guenter Roeck 7 months, 3 weeks ago

On Sun, Apr 20, 2025 at 08:40:27AM -0700, Guenter Roeck wrote:
> On Sat, Apr 19, 2025 at 01:51:46PM +0200, cem@kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> > 
> > Building linux on i386 might fail if a 64bit type is passed to
> 
> i386 actually builds. Its compiler is probably able to convert
> the offending mult_frac() without helpers since the divisor is
> a constant. I see the problem with openrisc and parisc, with
> gcc 13.3.0.
> 
> > mult_fract(). To prevent the failure, use do_div() for the division
> > calculation instead of hardcoding a / b.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
> > ---
> > 
> > I'm sending it as a RFC because I didn't to extensive testing on this
> > patch, also I'm not sure if mult_frac() was intended to work on 32-bit
> > only types. If that's the case, perhaps, a new mult_frac64() might be a
> > better idea?!
> > 
> >  include/linux/math.h | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/linux/math.h b/include/linux/math.h
> > index 0198c92cbe3e..05ea853b75b4 100644
> > --- a/include/linux/math.h
> > +++ b/include/linux/math.h
> > @@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
> >  #undef __STRUCT_FRACT
> >  
> >  /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> > -#define mult_frac(x, n, d)	\
> > -({				\
> > -	typeof(x) x_ = (x);	\
> > -	typeof(n) n_ = (n);	\
> > -	typeof(d) d_ = (d);	\
> > -				\
> > -	typeof(x_) q = x_ / d_;	\
> > -	typeof(x_) r = x_ % d_;	\
> > -	q * n_ + r * n_ / d_;	\
> > +#define mult_frac(x, n, d)		\
> > +({					\
> > +	typeof(x) x_ = (x);		\
> > +	typeof(n) n_ = (n);		\
> > +	typeof(d) d_ = (d);		\
> > +					\
> > +	typeof(x_) r = do_div(x_, d_);	\
> > +	r *= n_;			\
> > +	do_div(r, d_);			\
> > +	x_ * n_ + r;			\
> >  })
> >  
> 
> Unfortunately that doesn't work. I get build errors on parisc.
> 

Turns out the first parameter of do_div needs to be u64, not s64,
at least on parisc.

Guenter

> In file included from ./arch/parisc/include/generated/asm/div64.h:1,
>                  from ./include/linux/math.h:6,
>                  from ./include/linux/kernel.h:27,
>                  from ./arch/parisc/include/asm/bug.h:5,
>                  from ./include/linux/bug.h:5,
>                  from ./include/linux/mmdebug.h:5,
>                  from ./include/linux/mm.h:6,
>                  from mm/page_alloc.c:19:
> mm/page_alloc.c: In function 'boost_watermark':
> ./include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror]
>   183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
>       |                                   ^~
> ./include/linux/math.h:142:24: note: in expansion of macro 'do_div'
>   142 |         typeof(x_) r = do_div(x_, d_);  \
>       |                        ^~~~~~
> mm/page_alloc.c:2010:21: note: in expansion of macro 'mult_frac'
>  2010 |         max_boost = mult_frac(zone->_watermark[WMARK_HIGH],
> 
> That is just one example. It seems to affect all uses of
> mult_frac().
> 
> Guenter

Re: [RFC PATCH] math.h: Account for 64-bit division on i386

Posted by Geert Uytterhoeven 7 months, 3 weeks ago

On Sun, 20 Apr 2025 at 19:42, Guenter Roeck <linux@roeck-us.net> wrote:
> On Sun, Apr 20, 2025 at 08:40:27AM -0700, Guenter Roeck wrote:
> > On Sat, Apr 19, 2025 at 01:51:46PM +0200, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > >
> > > Building linux on i386 might fail if a 64bit type is passed to
> >
> > i386 actually builds. Its compiler is probably able to convert
> > the offending mult_frac() without helpers since the divisor is
> > a constant. I see the problem with openrisc and parisc, with
> > gcc 13.3.0.
> >
> > > mult_fract(). To prevent the failure, use do_div() for the division
> > > calculation instead of hardcoding a / b.
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
> > > ---
> > >
> > > I'm sending it as a RFC because I didn't to extensive testing on this
> > > patch, also I'm not sure if mult_frac() was intended to work on 32-bit
> > > only types. If that's the case, perhaps, a new mult_frac64() might be a
> > > better idea?!
> > >
> > >  include/linux/math.h | 19 ++++++++++---------
> > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/math.h b/include/linux/math.h
> > > index 0198c92cbe3e..05ea853b75b4 100644
> > > --- a/include/linux/math.h
> > > +++ b/include/linux/math.h
> > > @@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
> > >  #undef __STRUCT_FRACT
> > >
> > >  /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> > > -#define mult_frac(x, n, d) \
> > > -({                         \
> > > -   typeof(x) x_ = (x);     \
> > > -   typeof(n) n_ = (n);     \
> > > -   typeof(d) d_ = (d);     \
> > > -                           \
> > > -   typeof(x_) q = x_ / d_; \
> > > -   typeof(x_) r = x_ % d_; \
> > > -   q * n_ + r * n_ / d_;   \
> > > +#define mult_frac(x, n, d)         \
> > > +({                                 \
> > > +   typeof(x) x_ = (x);             \
> > > +   typeof(n) n_ = (n);             \
> > > +   typeof(d) d_ = (d);             \
> > > +                                   \
> > > +   typeof(x_) r = do_div(x_, d_);  \
> > > +   r *= n_;                        \
> > > +   do_div(r, d_);                  \
> > > +   x_ * n_ + r;                    \
> > >  })
> > >
> >
> > Unfortunately that doesn't work. I get build errors on parisc.
> >
>
> Turns out the first parameter of do_div needs to be u64, not s64,
> at least on parisc.

That is correct: the first parameter must be u64, the second must be u32:
https://elixir.bootlin.com/linux/v6.14.3/source/include/asm-generic/div64.h

As you must never use open-coded 64-bit divisions in the kernel,
simple helpers like mult_frac() can only be used for 32-bit values.
For anything involving 64-bit divisions, you must use the helpers from
<linux/math64.h>, e.g. mul_u64_u32_div().
https://elixir.bootlin.com/linux/v6.14.3/source/include/linux/math64.h#L257

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] math.h: Account for 64-bit division on i386

Posted by Carlos Maiolino 7 months, 3 weeks ago

On Sun, Apr 20, 2025 at 10:42:18AM -0700, Guenter Roeck wrote:
> On Sun, Apr 20, 2025 at 08:40:27AM -0700, Guenter Roeck wrote:
> > On Sat, Apr 19, 2025 at 01:51:46PM +0200, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > >
> > > Building linux on i386 might fail if a 64bit type is passed to
> >
> > i386 actually builds. Its compiler is probably able to convert
> > the offending mult_frac() without helpers since the divisor is
> > a constant. I see the problem with openrisc and parisc, with
> > gcc 13.3.0.
> >
> > > mult_fract(). To prevent the failure, use do_div() for the division
> > > calculation instead of hardcoding a / b.
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202504181233.F7D9Atra-lkp@intel.com/
> > > ---
> > >
> > > I'm sending it as a RFC because I didn't to extensive testing on this
> > > patch, also I'm not sure if mult_frac() was intended to work on 32-bit
> > > only types. If that's the case, perhaps, a new mult_frac64() might be a
> > > better idea?!
> > >
> > >  include/linux/math.h | 19 ++++++++++---------
> > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/math.h b/include/linux/math.h
> > > index 0198c92cbe3e..05ea853b75b4 100644
> > > --- a/include/linux/math.h
> > > +++ b/include/linux/math.h
> > > @@ -133,15 +133,16 @@ __STRUCT_FRACT(u32)
> > >  #undef __STRUCT_FRACT
> > >
> > >  /* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> > > -#define mult_frac(x, n, d)	\
> > > -({				\
> > > -	typeof(x) x_ = (x);	\
> > > -	typeof(n) n_ = (n);	\
> > > -	typeof(d) d_ = (d);	\
> > > -				\
> > > -	typeof(x_) q = x_ / d_;	\
> > > -	typeof(x_) r = x_ % d_;	\
> > > -	q * n_ + r * n_ / d_;	\
> > > +#define mult_frac(x, n, d)		\
> > > +({					\
> > > +	typeof(x) x_ = (x);		\
> > > +	typeof(n) n_ = (n);		\
> > > +	typeof(d) d_ = (d);		\
> > > +					\
> > > +	typeof(x_) r = do_div(x_, d_);	\
> > > +	r *= n_;			\
> > > +	do_div(r, d_);			\
> > > +	x_ * n_ + r;			\
> > >  })
> > >
> >
> > Unfortunately that doesn't work. I get build errors on parisc.
> >
> 
> Turns out the first parameter of do_div needs to be u64, not s64,
> at least on parisc.

Yup, that's true for x86 too, needs to be fixed once we decide which
path to take to fix it.

> 
> Guenter
> 
> > In file included from ./arch/parisc/include/generated/asm/div64.h:1,
> >                  from ./include/linux/math.h:6,
> >                  from ./include/linux/kernel.h:27,
> >                  from ./arch/parisc/include/asm/bug.h:5,
> >                  from ./include/linux/bug.h:5,
> >                  from ./include/linux/mmdebug.h:5,
> >                  from ./include/linux/mm.h:6,
> >                  from mm/page_alloc.c:19:
> > mm/page_alloc.c: In function 'boost_watermark':
> > ./include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror]
> >   183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> >       |                                   ^~
> > ./include/linux/math.h:142:24: note: in expansion of macro 'do_div'
> >   142 |         typeof(x_) r = do_div(x_, d_);  \
> >       |                        ^~~~~~
> > mm/page_alloc.c:2010:21: note: in expansion of macro 'mult_frac'
> >  2010 |         max_boost = mult_frac(zone->_watermark[WMARK_HIGH],
> >
> > That is just one example. It seems to affect all uses of
> > mult_frac().
> >
> > Guenter