Skip to contentSkip to conversation

Fix TCP/UDP checksums on the Broadcom SB-1

PATCH In review Daniel Jacobowitz 5 participants 7 messages Sep 20, 2005 MIPS

lore
Share & tools
lore

Daniel Jacobowitz

author
#0 lore raw

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

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 <[email protected]>
include/asm-mips/checksum.h +1 -1
Index: linux/include/asm-mips/checksum.h
===================================================================
@@ -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)
Signature · 2 lines
Daniel Jacobowitz
CodeSourcery, LLC
#1 lore raw
Quoted 3 lines · Daniel Jacobowitz

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

#2 lore raw

Hi

Quoted 7 lines
Quoted 3 lines

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

#3 lore raw
Quoted 10 lines · Matej Kupljen
Quoted 7 lines
Quoted 3 lines

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

#4 lore raw

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

Quoted 37 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Ralf Baechle
Sent: Tuesday, September 20, 2005 4:06 AM
To: Matej Kupljen
Cc: Maciej W. Rozycki; Daniel Jacobowitz; [email protected]
Subject: Re: [PATCH] Fix TCP/UDP checksums on the Broadcom SB-1

Quoted 1 line · Matej Kupljen
Quoted 1 line
Quoted 1 line

This caused incorrect checksums in some UDP packets for

NFS root.

Quoted 1 line
Quoted 1 line
Quoted 1 line

The problem was mild when using a 10.0.1.x IP address,

but severe

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

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

Quoted 4 lines
Quoted 1 line

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

#5 lore raw
Quoted 4 lines · 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.

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

#6 lore raw
In reply to Daniel Jacobowitz
Quoted 10 lines · 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.

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

Ralf