New struct sock_common breaks parisc 64 bit compiles with a misalignment

The problem seems to be that the new struct sock_common ends with a pointer and an atomic_t (which is an int on parisc), so the compiler adds an extra four bytes of padding where none previously existed in struct tcp_tw_bucket, so the __u64 ptr tricks with tw_daddr fail.

A fix that seems to work for me on parisc64 is to move the atomic_t out of the end of struct sock_common and back into the two other structures (so struct sock_common now ends on 0 mod 8).

James

===== include/net/sock.h 1.43 vs edited =====

@@ -111,7 +111,6 @@
 	struct sock		**skc_pprev;
 	struct sock		*skc_bind_next;
 	struct sock		**skc_bind_pprev;
-	atomic_t		skc_refcnt;
 };
 
 /**

@@ -191,7 +190,7 @@
 #define sk_pprev		__sk_common.skc_pprev
 #define sk_bind_next		__sk_common.skc_bind_next
 #define sk_bind_pprev		__sk_common.skc_bind_pprev
-#define sk_refcnt		__sk_common.skc_refcnt
+	atomic_t		sk_refcnt;
 	volatile unsigned char	sk_zapped;
 	unsigned char		sk_shutdown;
 	unsigned char		sk_use_write_queue;

===== include/net/tcp.h 1.44 vs edited =====

@@ -178,7 +178,7 @@
 #define tw_pprev		__tw_common.skc_pprev
 #define tw_bind_next		__tw_common.skc_bind_next
 #define tw_bind_pprev		__tw_common.skc_bind_pprev
-#define tw_refcnt		__tw_common.skc_refcnt
+	atomic_t		tw_refcnt;
 	volatile unsigned char	tw_substate;
 	unsigned char		tw_rcv_wscale;
 	__u16			tw_sport;

From: James Bottomley <[email protected]>
Date: 09 Jun 2003 23:57:46 -0500

A fix that seems to work for me on parisc64 is to move the atomic_t out of the end of struct sock_common and back into the two other structures (so struct sock_common now ends on 0 mod 8).

I would suggest instead to add the proper alignment attribute to the appropriate members of the tw bucket.

In article <[email protected]> (at Tue, 10 Jun 2003 09:12:17 -0700 (PDT)), "David S. Miller" <[email protected]> says:

Like this?

Index: linux-2.5/include/net/tcp.h
===================================================================
RCS file: /home/cvs/linux-2.5/include/net/tcp.h,v
retrieving revision 1.38

diff -u -r1.38 tcp.h
@@ -164,6 +164,14 @@
  * problems of sockets in such a state on heavily loaded servers, but
  * without violating the protocol specification.
  */
+
+/* Address pair should be aligned to 64-bits boundary to avoid penalties. */
+#if (BITS_PER_LONG == 64)
+# define	__tw_aligned	__attribute__ ((aligned(8)))
+#else
+# define	__tw_aligned
+#endif
+
 struct tcp_tw_bucket {
 	/*
 	 * Now struct sock also uses sock_common, so please just

@@ -184,7 +192,7 @@
 	__u16			tw_sport;
 	/* Socket demultiplex comparisons on incoming packets. */
 	/* these five are in inet_opt */
-	__u32			tw_daddr;
+	__u32	__tw_aligned	tw_daddr;
 	__u32			tw_rcv_saddr;
 	__u16			tw_dport;
 	__u16			tw_num;

Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
Date: Mon, 16 Jun 2003 02:09:20 +0900 (JST)

Like this?

I did this in my tree yesterday.
But thank you.

I'm fixing this, but why does it "fail"? You should get unaligned traps which get fixed up by the trap handler.

If that isn't happening, lots of things in the networking should break on you.

David S. Miller <[email protected]>

Well, it's an architecture thing, I suppose. Unaligned access traps are pretty expensive on the parisc, so we don't actually handle them when they're from the kernel, we panic instead (and expect the problem code to be fixed).

In this case, our rather crappy kernel tool chain gcc generated the instruction

ldd 52(%r1),%r4

Which is actually illegal assembly (displacements greater than 16 must be multiples of 8 for the load double word instruction). So I couldn't even compile the code.

If the gcc is told that the structure won't be aligned, it generates non-alignment faulting instructions to access it, so no, we don't see any misalignment faults in the networking layer.

James

From: James Bottomley <[email protected]>
Date: 15 Jun 2003 09:35:52 -0500

Unaligned access traps are pretty expensive on the parisc, so we don't actually handle them when they're from the kernel, we panic instead (and expect the problem code to be fixed).

Welcome to the real world, unaligned accesses are perfectly legal in the networking stack.

They are in fact guarenteed to occur when certain protocols are encapsulated in others.

Please add an unaligned trap handler for parisc64, thanks.

I didn't say we couldn't do unaligned accesses, I said we have to do them by marking the structure as potentially misaligned so the compiler generates instructions that won't trap.

It's slightly more expensive to access the structure this way (the compiler usually does eight byte loads instead of a double word load, but it's still far cheaper than having an unaligned access trap).

It's not necessary and would, indeed, be detrimental to operation since we'd generate alignment traps on almost every encapsulated protocol (at several hundred instructions per trap). If we do this, our network performance will tank.

As of 2.5.70, the networking layer seems to comply perfectly with the parisc requirements (at least we don't have any misaligned access trap panics from it), so most of the structures are correctly marked, anyway.

James

From: James Bottomley <[email protected]>
Date: 15 Jun 2003 10:17:10 -0500

It's not necessary and would, indeed, be detrimental to operation since we'd generate alignment traps on almost every encapsulated protocol (at several hundred instructions per trap). If we do this, our network performance will tank.

It doesn't happen for all the normal cases, but it does for things like IP in appletalk and stuff like that.

Please, implement the trap handlers. Thanks.