* Re: [Bug fix]mips 64bits checksum error -- csum_tcpudp_nofold
2017-01-25 18:13 ` Alexander Duyck
@ 2017-01-26 2:33 ` Mark Zhang
2017-01-26 15:57 ` Alexander Duyck
2017-01-26 8:09 ` Ralf Baechle
1 sibling, 1 reply; 6+ messages in thread
From: Mark Zhang @ 2017-01-26 2:33 UTC (permalink / raw)
To: Alexander Duyck
Cc: Ralf Baechle, David Miller, Alexander Duyck, linux-mips, linux-kernel
Hi Alex,
Thanks for your reply.
I tested your correction. The result is correct.
The C language will cause this function(csum_tcpudp_nofold) become
176 MIPS instructions. The assemble code is 150 MIPS instruction.
If the MIPS CPU is running as 1GHz, C language cause more 60 nano
seconds during send/receive each tcp/udp packet. I'm not sure whether
it will cause any negative result if the frequency of CPU was lower.
MIPS arch is usually used in networking equipments.
I think Ralf's correction is better.
- Mark
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
arch/mips/include/asm/checksum.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 5c585c5..08b6ba3 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -186,7 +186,9 @@ static inline __wsum csum_tcpudp_nofold(__be32 saddr,
" daddu %0, %4 \n"
" dsll32 $1, %0, 0 \n"
" daddu %0, $1 \n"
+ " sltu $1, %0, $1 \n"
" dsra32 %0, %0, 0 \n"
+ " daddu %0, $1 \n"
#endif
" .set pop"
: "=r" (sum)
2017-01-26 2:13 GMT+08:00 Alexander Duyck <alexander.duyck@gmail.com>:
> On Tue, Jan 24, 2017 at 8:35 PM, Mark Zhang <bomb.zhang@gmail.com> wrote:
>> If the input parameters as saddr = 0xc0a8fd60,daddr = 0xc0a8fda1,len =
>> 80, proto = 17, sum =0x7eae049d.
>> The correct result should be 1, but original function return 0.
>>
>> Attached the correction patch.
>
> I've copied your patch here:
>
> From 52e265f7fe0acf9a6e9c4346e1fe6fa994aa00b6 Mon Sep 17 00:00:00 2001
> From: qzhang <qin.2.zhang@nsn.com>
> Date: Wed, 25 Jan 2017 12:25:25 +0800
> Subject: [PATCH] Fixed the mips 64bits checksum error -- csum_tcpudp_nofold
>
> ---
> arch/mips/include/asm/checksum.h | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
> index 7749daf..0e351c5 100644
> --- a/arch/mips/include/asm/checksum.h
> +++ b/arch/mips/include/asm/checksum.h
> @@ -184,6 +184,10 @@ static inline __wsum csum_tcpudp_nofold(__be32
> saddr, __be32 daddr,
> " daddu %0, %2 \n"
> " daddu %0, %3 \n"
> " daddu %0, %4 \n"
> + " dsrl32 $1, %0, 0 \n"
> + " dsll32 %0, %0, 0 \n"
> + " dsrl32 %0, %0, 0 \n"
> + " daddu %0, $1 \n"
> " dsll32 $1, %0, 0 \n"
> " daddu %0, $1 \n"
> " dsra32 %0, %0, 0 \n"
>
> I agree there does appear to be a bug in the code, and my
> understanding of MIPS assembly is limited, but I don't think your
> patch truly fixes it. From what I can understand it seems like you
> would just be shifting the register called out at %0 past 64 bits
> which would just zero it out.
>
> Below is the snippet you are updating:
>
> #ifdef CONFIG_64BIT
> " daddu %0, %2 \n"
> " daddu %0, %3 \n"
> " daddu %0, %4 \n"
> " dsll32 $1, %0, 0 \n"
> " daddu %0, $1 \n"
> " dsra32 %0, %0, 0 \n"
> #endif
>
> From what I can tell the issue is that the dsll32 really needs to be
> replaced with some sort of rotation type call instead of a shift, but
> it looks like MIPS doesn't have a rotation instruction. We need to
> add the combination of a shift left by 32, to a shift right 32, and
> then add that value back to the original register. Then we will get
> the correct result in the upper 32 bits.
>
> I'm not even sure it is necessary to use inline assembler. You could
> probably just use the inline assembler for the 32b and change the 64b
> approach over to using C. The code for it would be pretty simple:
> unsigned long res = (__force unsigned long)sum;
>
> res += (__force unsigned long)daddr;
> res += (__force unsigned long)saddr;
> #ifdef __MIPSEL__
> res += (proto + len) << 8;
> #else
> res += proto + len;
> #endif
> res += (res << 32) | (res >> 32);
>
> return (__force __wsum)(res >> 32);
>
> That would probably be enough to fix the issue and odds are it should
> generate assembly code very similar to what was already there.
>
> - Alex
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Bug fix]mips 64bits checksum error -- csum_tcpudp_nofold
2017-01-25 18:13 ` Alexander Duyck
2017-01-26 2:33 ` Mark Zhang
@ 2017-01-26 8:09 ` Ralf Baechle
1 sibling, 0 replies; 6+ messages in thread
From: Ralf Baechle @ 2017-01-26 8:09 UTC (permalink / raw)
To: Alexander Duyck
Cc: Mark Zhang, David Miller, Alexander Duyck, linux-mips, linux-kernel
On Wed, Jan 25, 2017 at 10:13:32AM -0800, Alexander Duyck wrote:
> On Tue, Jan 24, 2017 at 8:35 PM, Mark Zhang <bomb.zhang@gmail.com> wrote:
> > If the input parameters as saddr = 0xc0a8fd60,daddr = 0xc0a8fda1,len =
> > 80, proto = 17, sum =0x7eae049d.
> > The correct result should be 1, but original function return 0.
> >
> > Attached the correction patch.
>
> I've copied your patch here:
>
> >From 52e265f7fe0acf9a6e9c4346e1fe6fa994aa00b6 Mon Sep 17 00:00:00 2001
> From: qzhang <qin.2.zhang@nsn.com>
> Date: Wed, 25 Jan 2017 12:25:25 +0800
> Subject: [PATCH] Fixed the mips 64bits checksum error -- csum_tcpudp_nofold
>
> ---
> arch/mips/include/asm/checksum.h | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
> index 7749daf..0e351c5 100644
> --- a/arch/mips/include/asm/checksum.h
> +++ b/arch/mips/include/asm/checksum.h
> @@ -184,6 +184,10 @@ static inline __wsum csum_tcpudp_nofold(__be32
> saddr, __be32 daddr,
> " daddu %0, %2 \n"
> " daddu %0, %3 \n"
> " daddu %0, %4 \n"
> + " dsrl32 $1, %0, 0 \n" # Put upper 32 bits of %0 into $1
> + " dsll32 %0, %0, 0 \n" # zero upper 32 bits of %0
> + " dsrl32 %0, %0, 0 \n"
> + " daddu %0, $1 \n" # now add
> " dsll32 $1, %0, 0 \n"
> " daddu %0, $1 \n"
> " dsra32 %0, %0, 0 \n"
See the comments I added into above code.
> I agree there does appear to be a bug in the code, and my
> understanding of MIPS assembly is limited, but I don't think your
> patch truly fixes it. From what I can understand it seems like you
> would just be shifting the register called out at %0 past 64 bits
> which would just zero it out.
Not quite, DSLL32 is a logic shift left by 32 + x bits where x is the 0..31
constant of the 3rd argument. Similarly DSRA32 is an arithmetic shift
right by 32 + x bits where x is the 0..31 constant of the 3rd argument.
> Below is the snippet you are updating:
>
> #ifdef CONFIG_64BIT
> " daddu %0, %2 \n"
> " daddu %0, %3 \n"
> " daddu %0, %4 \n"
> " dsll32 $1, %0, 0 \n"
> " daddu %0, $1 \n"
> " dsra32 %0, %0, 0 \n"
> #endif
>
> >From what I can tell the issue is that the dsll32 really needs to be
> replaced with some sort of rotation type call instead of a shift, but
> it looks like MIPS doesn't have a rotation instruction. We need to
> add the combination of a shift left by 32, to a shift right 32, and
> then add that value back to the original register. Then we will get
> the correct result in the upper 32 bits.
The DSLL32 is fine. What the DSLL32 does is moving the low 32 bits of %0
to the upper 32 bits of the target register $1 before the DADDU is adding
both 32 bit halfs of the 64 bit intermediate checksum together.
The real bug is that above inline code was written assuming that when
folding the intermediate 64-bit sum to 32 bit there can not ever be a
carry.
> I'm not even sure it is necessary to use inline assembler. You could
> probably just use the inline assembler for the 32b and change the 64b
> approach over to using C. The code for it would be pretty simple:
> unsigned long res = (__force unsigned long)sum;
>
> res += (__force unsigned long)daddr;
> res += (__force unsigned long)saddr;
> #ifdef __MIPSEL__
> res += (proto + len) << 8;
> #else
> res += proto + len;
> #endif
> res += (res << 32) | (res >> 32);
>
> return (__force __wsum)(res >> 32);
>
> That would probably be enough to fix the issue and odds are it should
> generate assembly code very similar to what was already there.
GCC used to do silly things at times which is the sole reason why it
ever made sense to use inline assembler. No more and compiled C code
can be scheduled better than inline assembler these days.
I have a fix but unlike Mark's original fix it only adds two instructions.
Ralf
^ permalink raw reply [flat|nested] 6+ messages in thread