* [PATCH] MIPS: Fix a longstanding error in div64.h @ 2021-04-06 11:24 Huacai Chen 2021-04-06 12:42 ` H. Nikolaus Schaller 2021-04-06 13:06 ` Thomas Bogendoerfer 0 siblings, 2 replies; 10+ messages in thread From: Huacai Chen @ 2021-04-06 11:24 UTC (permalink / raw) To: Thomas Bogendoerfer; +Cc: linux-mips, Jiaxun Yang, Huacai Chen, stable 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 11:24 [PATCH] MIPS: Fix a longstanding error in div64.h Huacai Chen @ 2021-04-06 12:42 ` H. Nikolaus Schaller 2021-04-06 14:49 ` Zhou Yanjie 2021-04-06 13:06 ` Thomas Bogendoerfer 1 sibling, 1 reply; 10+ messages in thread From: H. Nikolaus Schaller @ 2021-04-06 12:42 UTC (permalink / raw) To: Huacai Chen Cc: Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, stable, Discussions about the Letux Kernel > 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 12:42 ` H. Nikolaus Schaller @ 2021-04-06 14:49 ` Zhou Yanjie 2021-04-06 22:22 ` Maciej W. Rozycki 0 siblings, 1 reply; 10+ messages in thread From: Zhou Yanjie @ 2021-04-06 14:49 UTC (permalink / raw) To: H. Nikolaus Schaller, Huacai Chen Cc: Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, stable, Discussions about the Letux Kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 14:49 ` Zhou Yanjie @ 2021-04-06 22:22 ` Maciej W. Rozycki 2021-04-07 1:27 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Maciej W. Rozycki @ 2021-04-06 22:22 UTC (permalink / raw) To: Zhou Yanjie, Huacai Chen Cc: H. Nikolaus Schaller, Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, Ralf Baechle, stable, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 22:22 ` Maciej W. Rozycki @ 2021-04-07 1:27 ` Huacai Chen 2021-04-07 13:38 ` Maciej W. Rozycki 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2021-04-07 1:27 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Zhou Yanjie, H. Nikolaus Schaller, Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, Ralf Baechle, stable, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-07 1:27 ` Huacai Chen @ 2021-04-07 13:38 ` Maciej W. Rozycki 2021-04-08 4:56 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Maciej W. Rozycki @ 2021-04-07 13:38 UTC (permalink / raw) To: Huacai Chen Cc: Zhou Yanjie, H. Nikolaus Schaller, Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, Ralf Baechle, stable, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-07 13:38 ` Maciej W. Rozycki @ 2021-04-08 4:56 ` Huacai Chen 2021-04-09 7:17 ` YunQiang Su 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2021-04-08 4:56 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Zhou Yanjie, H. Nikolaus Schaller, Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, Ralf Baechle, stable, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-08 4:56 ` Huacai Chen @ 2021-04-09 7:17 ` YunQiang Su 0 siblings, 0 replies; 10+ messages in thread From: YunQiang Su @ 2021-04-09 7:17 UTC (permalink / raw) To: Huacai Chen Cc: Maciej W. Rozycki, Zhou Yanjie, H. Nikolaus Schaller, Thomas Bogendoerfer, linux-mips, Jiaxun Yang, Huacai Chen, Ralf Baechle, stable, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 11:24 [PATCH] MIPS: Fix a longstanding error in div64.h Huacai Chen 2021-04-06 12:42 ` H. Nikolaus Schaller @ 2021-04-06 13:06 ` Thomas Bogendoerfer 2021-04-07 0:40 ` Huacai Chen 1 sibling, 1 reply; 10+ messages in thread From: Thomas Bogendoerfer @ 2021-04-06 13:06 UTC (permalink / raw) To: Huacai Chen; +Cc: linux-mips, Jiaxun Yang, Huacai Chen, stable 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 ] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MIPS: Fix a longstanding error in div64.h 2021-04-06 13:06 ` Thomas Bogendoerfer @ 2021-04-07 0:40 ` Huacai Chen 0 siblings, 0 replies; 10+ messages in thread From: Huacai Chen @ 2021-04-07 0:40 UTC (permalink / raw) To: Thomas Bogendoerfer; +Cc: open list:MIPS, Jiaxun Yang, Huacai Chen, stable 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 ] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-04-09 7:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-06 11:24 [PATCH] MIPS: Fix a longstanding error in div64.h Huacai Chen 2021-04-06 12:42 ` H. Nikolaus Schaller 2021-04-06 14:49 ` Zhou Yanjie 2021-04-06 22:22 ` Maciej W. Rozycki 2021-04-07 1:27 ` Huacai Chen 2021-04-07 13:38 ` Maciej W. Rozycki 2021-04-08 4:56 ` Huacai Chen 2021-04-09 7:17 ` YunQiang Su 2021-04-06 13:06 ` Thomas Bogendoerfer 2021-04-07 0:40 ` Huacai Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.