[PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Daniel Jacobowitz]

The type of sum in csum_tcpudp_nofold is "unsigned int", so when we assign
to it in an asm() block, and we're running on a system with 64-bit
registers, it is vitally important that we sign extend it correctly before
returning to C.  Otherwise the stray high bits will be preserved into
csum_fold, and on the SB-1 processor, 32-bit arithmetic on a non
sign-extended register will yield surprising results.

This caused incorrect checksums in some UDP packets for NFS root.  The
problem was mild when using a 10.0.1.x IP address, but severe when
using 192.168.1.x.

Signed-off-by: Daniel Jacobowitz <dan@codesourcery.com>

Index: linux/include/asm-mips/checksum.h
===================================================================
--- linux.orig/include/asm-mips/checksum.h	2005-09-19 21:00:48.000000000 -0400
+++ linux/include/asm-mips/checksum.h	2005-09-19 21:52:11.000000000 -0400
@@ -149,7 +149,7 @@ static inline unsigned int csum_tcpudp_n
 	"	daddu	%0, %4		\n"
 	"	dsll32	$1, %0, 0	\n"
 	"	daddu	%0, $1		\n"
-	"	dsrl32	%0, %0, 0	\n"
+	"	dsra32	%0, %0, 0	\n"
 #endif
 	"	.set	pop"
 	: "=r" (sum)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Maciej W. Rozycki]

On Mon, 19 Sep 2005, Daniel Jacobowitz wrote:

> This caused incorrect checksums in some UDP packets for NFS root.  The
> problem was mild when using a 10.0.1.x IP address, but severe when
> using 192.168.1.x.

 Ah!  So *that* is the reason for the absolutely abysmal NFS performance 
of the SWARM with 2.6!  I have had no time to track it down -- thanks a 
lot!

  Maciej

Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Matej Kupljen]

Hi

> > This caused incorrect checksums in some UDP packets for NFS root.  The
> > problem was mild when using a 10.0.1.x IP address, but severe when
> > using 192.168.1.x.
> 
>  Ah!  So *that* is the reason for the absolutely abysmal NFS performance 
> of the SWARM with 2.6!  I have had no time to track it down -- thanks a 
> lot!

Is this for MIPS64 only?
Because, on dbau1200 we also have poor NFS performance :-(

BR,
Matej

Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Ralf Baechle]

On Mon, Sep 19, 2005 at 11:28:18PM -0400, Daniel Jacobowitz wrote:

> The type of sum in csum_tcpudp_nofold is "unsigned int", so when we assign
> to it in an asm() block, and we're running on a system with 64-bit
> registers, it is vitally important that we sign extend it correctly before
> returning to C.  Otherwise the stray high bits will be preserved into
> csum_fold, and on the SB-1 processor, 32-bit arithmetic on a non
> sign-extended register will yield surprising results.
> 
> This caused incorrect checksums in some UDP packets for NFS root.  The
> problem was mild when using a 10.0.1.x IP address, but severe when
> using 192.168.1.x.

Good catch.  And just to increase the func factor this bug did also apply
to 2.4.  Applied,

  Ralf

Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Ralf Baechle]

On Tue, Sep 20, 2005 at 01:00:04PM +0200, Matej Kupljen wrote:

> > > This caused incorrect checksums in some UDP packets for NFS root.  The
> > > problem was mild when using a 10.0.1.x IP address, but severe when
> > > using 192.168.1.x.
> > 
> >  Ah!  So *that* is the reason for the absolutely abysmal NFS performance 
> > of the SWARM with 2.6!  I have had no time to track it down -- thanks a 
> > lot!
> 
> Is this for MIPS64 only?
> Because, on dbau1200 we also have poor NFS performance :-(

It's for 64-bit kernels only.  Note the difference, I didn't say MIPS64.

Also, since this bug did result in an operation that has undefined
behaviour it likely may will only have impacted some 64-bit processors -
such as the SB1 - but others may have been unaffected.

  Ralf

RE: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Michael Uhler]

For what it's worth, the 64-bit architecture, both prior to and with MIPS64,
has always required that 64-bit GPRs be sign-extended when used with 32-bit
operations.  I'm surprised that this wasn't seen on more 64-bit CPUs than
just the SB1.


/gmu
---
Michael Uhler, Chief Technology Officer
MIPS Technologies, Inc.   Email: uhler at mips.com
1225 Charleston Road      Voice:  (650)567-5025
Mountain View, CA 94043

> -----Original Message-----
> From: linux-mips-bounce@linux-mips.org 
> [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Ralf Baechle
> Sent: Tuesday, September 20, 2005 4:06 AM
> To: Matej Kupljen
> Cc: Maciej W. Rozycki; Daniel Jacobowitz; linux-mips@linux-mips.org
> Subject: Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1
> 
> 
> On Tue, Sep 20, 2005 at 01:00:04PM +0200, Matej Kupljen wrote:
> 
> > > > This caused incorrect checksums in some UDP packets for 
> NFS root.  
> > > > The problem was mild when using a 10.0.1.x IP address, 
> but severe 
> > > > when using 192.168.1.x.
> > > 
> > >  Ah!  So *that* is the reason for the absolutely abysmal NFS 
> > > performance
> > > of the SWARM with 2.6!  I have had no time to track it 
> down -- thanks a 
> > > lot!
> > 
> > Is this for MIPS64 only?
> > Because, on dbau1200 we also have poor NFS performance :-(
> 
> It's for 64-bit kernels only.  Note the difference, I didn't 
> say MIPS64.
> 
> Also, since this bug did result in an operation that has 
> undefined behaviour it likely may will only have impacted 
> some 64-bit processors - such as the SB1 - but others may 
> have been unaffected.
> 
>   Ralf
> 
> 

RE: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Michael Uhler]

For what it's worth, the 64-bit architecture, both prior to and with MIPS64,
has always required that 64-bit GPRs be sign-extended when used with 32-bit
operations.  I'm surprised that this wasn't seen on more 64-bit CPUs than
just the SB1.


/gmu
---
Michael Uhler, Chief Technology Officer
MIPS Technologies, Inc.   Email: uhler at mips.com
1225 Charleston Road      Voice:  (650)567-5025
Mountain View, CA 94043

> -----Original Message-----
> From: linux-mips-bounce@linux-mips.org 
> [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Ralf Baechle
> Sent: Tuesday, September 20, 2005 4:06 AM
> To: Matej Kupljen
> Cc: Maciej W. Rozycki; Daniel Jacobowitz; linux-mips@linux-mips.org
> Subject: Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1
> 
> 
> On Tue, Sep 20, 2005 at 01:00:04PM +0200, Matej Kupljen wrote:
> 
> > > > This caused incorrect checksums in some UDP packets for 
> NFS root.  
> > > > The problem was mild when using a 10.0.1.x IP address, 
> but severe 
> > > > when using 192.168.1.x.
> > > 
> > >  Ah!  So *that* is the reason for the absolutely abysmal NFS 
> > > performance
> > > of the SWARM with 2.6!  I have had no time to track it 
> down -- thanks a 
> > > lot!
> > 
> > Is this for MIPS64 only?
> > Because, on dbau1200 we also have poor NFS performance :-(
> 
> It's for 64-bit kernels only.  Note the difference, I didn't 
> say MIPS64.
> 
> Also, since this bug did result in an operation that has 
> undefined behaviour it likely may will only have impacted 
> some 64-bit processors - such as the SB1 - but others may 
> have been unaffected.
> 
>   Ralf
> 
> 

Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

[Ralf Baechle]

On Tue, Sep 20, 2005 at 07:26:54AM -0700, Michael Uhler wrote:

> For what it's worth, the 64-bit architecture, both prior to and with MIPS64,
> has always required that 64-bit GPRs be sign-extended when used with 32-bit
> operations.  I'm surprised that this wasn't seen on more 64-bit CPUs than
> just the SB1.

Usually resends will paper over this kind of problem.  It's only a question
of time until they succeed for any protocol that changed the packet
content sufficiently to make the checksum work eventually.  But of course
performance will suffer and as a matter of statistics certain IP address
and port ranges are going to suffer more than others.

  Ralf