[PATCH] MIPS: Fix a longstanding error in div64.h

[PATCH] MIPS: Fix a longstanding error in div64.h

[Huacai Chen]

Only 32bit kernel need __div64_32(), but commit c21004cd5b4cb7d479514d4
("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.") makes it depend
on 64bit kernel by mistake. This patch fix this longstanding error.

Fixes: c21004cd5b4cb7d479514d4 ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.")
Cc: stable@vger.kernel.org
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 arch/mips/include/asm/div64.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/div64.h b/arch/mips/include/asm/div64.h
index dc5ea5736440..d199fe36eb46 100644
--- a/arch/mips/include/asm/div64.h
+++ b/arch/mips/include/asm/div64.h
@@ -11,7 +11,7 @@
 
 #include <asm-generic/div64.h>
 
-#if BITS_PER_LONG == 64
+#if BITS_PER_LONG == 32
 
 #include <linux/types.h>
 
-- 
2.27.0

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[H. Nikolaus Schaller]

> Am 06.04.2021 um 13:24 schrieb Huacai Chen <chenhuacai@kernel.org>:
> 
> Only 32bit kernel need __div64_32(), but commit c21004cd5b4cb7d479514d4
> ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.") makes it depend
> on 64bit kernel by mistake. This patch fix this longstanding error.
> 
> Fixes: c21004cd5b4cb7d479514d4 ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
> arch/mips/include/asm/div64.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/div64.h b/arch/mips/include/asm/div64.h
> index dc5ea5736440..d199fe36eb46 100644
> --- a/arch/mips/include/asm/div64.h
> +++ b/arch/mips/include/asm/div64.h
> @@ -11,7 +11,7 @@
> 
> #include <asm-generic/div64.h>
> 
> -#if BITS_PER_LONG == 64
> +#if BITS_PER_LONG == 32
> 
> #include <linux/types.h>

Hm.

Ingenic jz4780/CI20 build attempt on top ov v5.12-rc6:

In file included from ./include/linux/math64.h:7:0,
                 from ./include/linux/time.h:6,
                 from ./include/linux/compat.h:10,
                 from arch/mips/kernel/asm-offsets.c:12:
./include/linux/math64.h: In function 'div_u64_rem':
./arch/mips/include/asm/div64.h:29:11: error: invalid type argument of unary '*' (have 'long long unsigned int')
  __high = *__n >> 32;      \
           ^
./include/asm-generic/div64.h:243:11: note: in expansion of macro '__div64_32'
   __rem = __div64_32(&(n), __base); \
           ^
./include/linux/math64.h:91:15: note: in expansion of macro 'do_div'
  *remainder = do_div(dividend, divisor);
               ^

Or does it just reveal an unknown bug in my compiler?

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Thomas Bogendoerfer]

On Tue, Apr 06, 2021 at 07:24:03PM +0800, Huacai Chen wrote:
> Only 32bit kernel need __div64_32(), but commit c21004cd5b4cb7d479514d4
> ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.") makes it depend
> on 64bit kernel by mistake. This patch fix this longstanding error.
> 
> Fixes: c21004cd5b4cb7d479514d4 ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  arch/mips/include/asm/div64.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/div64.h b/arch/mips/include/asm/div64.h
> index dc5ea5736440..d199fe36eb46 100644
> --- a/arch/mips/include/asm/div64.h
> +++ b/arch/mips/include/asm/div64.h
> @@ -11,7 +11,7 @@
>  
>  #include <asm-generic/div64.h>
>  
> -#if BITS_PER_LONG == 64
> +#if BITS_PER_LONG == 32

are you sure this will make a difference ? asm-generic/div64.h checks
for __div64_32, which is not defined before including it here.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Zhou Yanjie]

Hi Nikolaus,

On 2021/4/6 下午8:42, H. Nikolaus Schaller wrote:
>> Am 06.04.2021 um 13:24 schrieb Huacai Chen <chenhuacai@kernel.org>:
>>
>> Only 32bit kernel need __div64_32(), but commit c21004cd5b4cb7d479514d4
>> ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.") makes it depend
>> on 64bit kernel by mistake. This patch fix this longstanding error.
>>
>> Fixes: c21004cd5b4cb7d479514d4 ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>> ---
>> arch/mips/include/asm/div64.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/include/asm/div64.h b/arch/mips/include/asm/div64.h
>> index dc5ea5736440..d199fe36eb46 100644
>> --- a/arch/mips/include/asm/div64.h
>> +++ b/arch/mips/include/asm/div64.h
>> @@ -11,7 +11,7 @@
>>
>> #include <asm-generic/div64.h>
>>
>> -#if BITS_PER_LONG == 64
>> +#if BITS_PER_LONG == 32
>>
>> #include <linux/types.h>
> Hm.
>
> Ingenic jz4780/CI20 build attempt on top ov v5.12-rc6:
>
> In file included from ./include/linux/math64.h:7:0,
>                   from ./include/linux/time.h:6,
>                   from ./include/linux/compat.h:10,
>                   from arch/mips/kernel/asm-offsets.c:12:
> ./include/linux/math64.h: In function 'div_u64_rem':
> ./arch/mips/include/asm/div64.h:29:11: error: invalid type argument of unary '*' (have 'long long unsigned int')
>    __high = *__n >> 32;      \
>             ^
> ./include/asm-generic/div64.h:243:11: note: in expansion of macro '__div64_32'
>     __rem = __div64_32(&(n), __base); \
>             ^
> ./include/linux/math64.h:91:15: note: in expansion of macro 'do_div'
>    *remainder = do_div(dividend, divisor);
>                 ^
>
> Or does it just reveal an unknown bug in my compiler?


I also get these:


In file included from ./include/linux/math64.h:7:0,
                  from ./include/linux/time.h:6,
                  from ./include/linux/compat.h:10,
                  from arch/mips/kernel/asm-offsets.c:12:
./include/linux/math64.h: In function 'div_u64_rem':
./arch/mips/include/asm/div64.h:29:11: error: invalid type argument of 
unary '*' (have 'long long unsigned int')
   __high = *__n >> 32;      \
            ^
./include/asm-generic/div64.h:243:11: note: in expansion of macro 
'__div64_32'
    __rem = __div64_32(&(n), __base); \
            ^
./include/linux/math64.h:91:15: note: in expansion of macro 'do_div'
   *remainder = do_div(dividend, divisor);
                ^
./include/linux/math64.h: In function 'mul_u64_u32_div':
./arch/mips/include/asm/div64.h:29:11: error: invalid type argument of 
unary '*' (have 'long long unsigned int')
   __high = *__n >> 32;      \
            ^
./include/asm-generic/div64.h:243:11: note: in expansion of macro 
'__div64_32'
    __rem = __div64_32(&(n), __base); \
            ^
./include/linux/math64.h:256:14: note: in expansion of macro 'do_div'
   rl.l.high = do_div(rh.ll, divisor);
               ^
./arch/mips/include/asm/div64.h:29:11: error: invalid type argument of 
unary '*' (have 'long long unsigned int')
   __high = *__n >> 32;      \

...


So it seems not a compiler problem.

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Maciej W. Rozycki]

On Tue, 6 Apr 2021, Zhou Yanjie wrote:

> So it seems not a compiler problem.

 This code is rather broken in an obvious way, starting from:

	unsigned long long __n;						\
									\
	__high = *__n >> 32;						\
	__low = __n;							\

where `__n' is used uninitialised.  Since this is my code originally I'll 
look into it; we may want to reinstate `do_div' too, which didn't have to 
be removed in the first place.

 Also commit e8e4eb0fbeda ("asm-generic/div64: Fix documentation of 
do_div() parameter") was an incomplete documentation fix.

 Huacai, thanks for your investigation!  Please be more careful in 
verifying your future submissions however.

  Maciej

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Huacai Chen]

Hi, Thomas,


On Tue, Apr 6, 2021 at 9:18 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Tue, Apr 06, 2021 at 07:24:03PM +0800, Huacai Chen wrote:
> > Only 32bit kernel need __div64_32(), but commit c21004cd5b4cb7d479514d4
> > ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.") makes it depend
> > on 64bit kernel by mistake. This patch fix this longstanding error.
> >
> > Fixes: c21004cd5b4cb7d479514d4 ("MIPS: Rewrite <asm/div64.h> to work with gcc 4.4.0.")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  arch/mips/include/asm/div64.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/mips/include/asm/div64.h b/arch/mips/include/asm/div64.h
> > index dc5ea5736440..d199fe36eb46 100644
> > --- a/arch/mips/include/asm/div64.h
> > +++ b/arch/mips/include/asm/div64.h
> > @@ -11,7 +11,7 @@
> >
> >  #include <asm-generic/div64.h>
> >
> > -#if BITS_PER_LONG == 64
> > +#if BITS_PER_LONG == 32
>
> are you sure this will make a difference ? asm-generic/div64.h checks
> for __div64_32, which is not defined before including it here.
Sorry for my carelessness, the #include should be after __div64_32,
and there are other bugs in this file, I will send a new version.

Huacai
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Huacai Chen]

Hi, Maciej,

On Wed, Apr 7, 2021 at 6:22 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Tue, 6 Apr 2021, Zhou Yanjie wrote:
>
> > So it seems not a compiler problem.
>
>  This code is rather broken in an obvious way, starting from:
>
>         unsigned long long __n;                                         \
>                                                                         \
>         __high = *__n >> 32;                                            \
>         __low = __n;                                                    \
>
> where `__n' is used uninitialised.  Since this is my code originally I'll
> look into it; we may want to reinstate `do_div' too, which didn't have to
> be removed in the first place.
I think we can reuse the generic do_div().

>
>  Also commit e8e4eb0fbeda ("asm-generic/div64: Fix documentation of
> do_div() parameter") was an incomplete documentation fix.
>
>  Huacai, thanks for your investigation!  Please be more careful in
> verifying your future submissions however.
Sorry, I thought there is only one bug in div64.h, but in fact there
are three...

Huacai
>
>   Maciej

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Maciej W. Rozycki]

On Wed, 7 Apr 2021, Huacai Chen wrote:

> >  This code is rather broken in an obvious way, starting from:
> >
> >         unsigned long long __n;                                         \
> >                                                                         \
> >         __high = *__n >> 32;                                            \
> >         __low = __n;                                                    \
> >
> > where `__n' is used uninitialised.  Since this is my code originally I'll
> > look into it; we may want to reinstate `do_div' too, which didn't have to
> > be removed in the first place.
> I think we can reuse the generic do_div().

 We can, but it's not clear to me if this is optimal.  We have a DIVMOD 
instruction which original code took advantage of (although I can see 
potential in reusing bits from include/asm-generic/div64.h).  The two 
implementations would have to be benchmarked against each other across a 
couple of different CPUs.

> >  Huacai, thanks for your investigation!  Please be more careful in
> > verifying your future submissions however.
> Sorry, I thought there is only one bug in div64.h, but in fact there
> are three...

 This just shows the verification you made was not good enough, hence my 
observation.

  Maciej

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[Huacai Chen]

Hi, Maciej,

On Wed, Apr 7, 2021 at 9:38 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Wed, 7 Apr 2021, Huacai Chen wrote:
>
> > >  This code is rather broken in an obvious way, starting from:
> > >
> > >         unsigned long long __n;                                         \
> > >                                                                         \
> > >         __high = *__n >> 32;                                            \
> > >         __low = __n;                                                    \
> > >
> > > where `__n' is used uninitialised.  Since this is my code originally I'll
> > > look into it; we may want to reinstate `do_div' too, which didn't have to
> > > be removed in the first place.
> > I think we can reuse the generic do_div().
>
>  We can, but it's not clear to me if this is optimal.  We have a DIVMOD
> instruction which original code took advantage of (although I can see
> potential in reusing bits from include/asm-generic/div64.h).  The two
> implementations would have to be benchmarked against each other across a
> couple of different CPUs.
The original MIPS do_div() has "h" constraint, and this is also the
reason why Ralf rewrote this file. How can we reintroduce do_div()
without "h" constraint?

Huacai
>
> > >  Huacai, thanks for your investigation!  Please be more careful in
> > > verifying your future submissions however.
> > Sorry, I thought there is only one bug in div64.h, but in fact there
> > are three...
>
>  This just shows the verification you made was not good enough, hence my
> observation.
>
>   Maciej

Re: [PATCH] MIPS: Fix a longstanding error in div64.h

[YunQiang Su]

Huacai Chen <chenhuacai@kernel.org> 于2021年4月8日周四 下午12:56写道：
>
> Hi, Maciej,
>
> On Wed, Apr 7, 2021 at 9:38 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> > On Wed, 7 Apr 2021, Huacai Chen wrote:
> >
> > > >  This code is rather broken in an obvious way, starting from:
> > > >
> > > >         unsigned long long __n;                                         \
> > > >                                                                         \
> > > >         __high = *__n >> 32;                                            \
> > > >         __low = __n;                                                    \
> > > >
> > > > where `__n' is used uninitialised.  Since this is my code originally I'll
> > > > look into it; we may want to reinstate `do_div' too, which didn't have to
> > > > be removed in the first place.
> > > I think we can reuse the generic do_div().
> >
> >  We can, but it's not clear to me if this is optimal.  We have a DIVMOD
> > instruction which original code took advantage of (although I can see
> > potential in reusing bits from include/asm-generic/div64.h).  The two
> > implementations would have to be benchmarked against each other across a
> > couple of different CPUs.
> The original MIPS do_div() has "h" constraint, and this is also the
> reason why Ralf rewrote this file. How can we reintroduce do_div()
> without "h" constraint?
>

I try to figure out a new version:

uint32_t __attribute__ ((noinline)) div64_32n(uint64_t *x, uint32_t b) {
        uint64_t a = *x;

        uint64_t t1 = ((a>>32)/b)<<32;
        uint32_t t2 = (a>>32)%b;

        uint32_t res = (uint32_t)a;
        uint32_t t1lo = 0;

        uint32_t t3 = 0xffffffffu/b;
        uint32_t t4 = t3*b;
        uint32_t hi, lo;

        while(t2>0) {
                __asm__ (
                        "multu %2, %3\n"
                        "mfhi %0\n"
                        "mflo %1\n"
                        : "=r" (hi), "=r"(lo)
                        : "r" (t4), "r"(t2)
                );

                // yes, we are sure that t2*t3 will not overflow
                t1lo += (t3*t2);
                t2 -= hi;
                if (lo > 0) {
                        t2 --; // we are sure that t2 > 0
                        lo = 0xffffffff - lo + 1;
                        unsigned tmp = lo + res;
                        // overflow
                        if (tmp < lo || tmp < res) {
                                t2 ++;
                        }
                        res = tmp;
                }
        }
        if (res >= b) {
                t1lo += (res/b);
                res = (res%b);
        }

        t1 += t1lo;
        *x = t1;
        return res;
}

With some test the performace: ((uint64_t)(-1))/3 with 0xfffff times

GCC: 5555555555555555, 0, seconds: 5
SYQ: 5555555555555555, 0, seconds: 4
KER: 5555555555555555, 0, seconds: 8
RAL: ffffffff, 2, seconds: 4

1. the MIPS current asm version cost 4s (and wrong result)
2. the simplest C code : a/b && a % b, cost 5s
3. the asm-generic version cost 8s.
4. my version cost 4s.

And the question is why asm-generic version exists
since it has bad performance than the code generated by GCC?

> Huacai
> >
> > > >  Huacai, thanks for your investigation!  Please be more careful in
> > > > verifying your future submissions however.
> > > Sorry, I thought there is only one bug in div64.h, but in fact there
> > > are three...
> >
> >  This just shows the verification you made was not good enough, hence my
> > observation.
> >
> >   Maciej



-- 
YunQiang Su