| Message ID | 20210406112404.2671507-1-chenhuacai@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Fix a longstanding error in div64.h | expand |
> 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?
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.
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.
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
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 ]
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
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
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
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
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>
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(-)