New struct sock_common breaks parisc 64 bit compiles with a misalignment

New struct sock_common breaks parisc 64 bit compiles with a misalignment

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

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



[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 1131 bytes --]

===== include/net/sock.h 1.43 vs edited =====
--- 1.43/include/net/sock.h	Wed Jun  4 19:57:07 2003
+++ edited/include/net/sock.h	Mon Jun  9 23:41:15 2003
@@ -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 =====
--- 1.44/include/net/tcp.h	Fri Jun  6 05:24:44 2003
+++ edited/include/net/tcp.h	Mon Jun  9 23:39:44 2003
@@ -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;

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[David S. Miller]

   From: James Bottomley <James.Bottomley@SteelEye.com>
   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.

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[David S. Miller]

On Mon, 2003-06-09 at 21:57, James Bottomley wrote:
> 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.

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 <davem@redhat.com>

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[David S. Miller]

   From: James Bottomley <James.Bottomley@SteelEye.com>
   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.

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[James Bottomley]

On Sun, 2003-06-15 at 01:19, David S. Miller wrote:
> On Mon, 2003-06-09 at 21:57, James Bottomley wrote:
> > 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.
> 
> I'm fixing this, but why does it "fail"?  You should get unaligned
> traps which get fixed up by the trap handler.

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 that isn't happening, lots of things in the networking should
> break on you.

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

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[James Bottomley]

On Sun, 2003-06-15 at 09:35, David S. Miller wrote:
> Welcome to the real world, unaligned accesses are perfectly
> legal in the networking stack.

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).

> They are in fact guarenteed to occur when certain protocols
> are encapsulated in others.
> 
> Please add an unaligned trap handler for parisc64, thanks.

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

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[David S. Miller]

   From: James Bottomley <James.Bottomley@SteelEye.com>
   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.

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[Andi Kleen]

"David S. Miller" <davem@redhat.com> writes:

>    From: James Bottomley <James.Bottomley@SteelEye.com>
>    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.

It can be remotely triggered in ordinary TCP. Just add an odd number of nops 
before a TCP timestamp to misalign it.

In short any linux parisc64 box on the net is very likely remotely 
panicable.

-Andi

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[David S. Miller]

   From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
   Date: Mon, 16 Jun 2003 02:09:20 +0900 (JST)

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

Re: New struct sock_common breaks parisc 64 bit compiles with a misalignment

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030610.091217.112601441.davem@redhat.com> (at Tue, 10 Jun 2003 09:12:17 -0700 (PDT)), "David S. Miller" <davem@redhat.com> says:

>    From: James Bottomley <James.Bottomley@SteelEye.com>
>    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.

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
--- linux-2.5/include/net/tcp.h	7 Jun 2003 00:22:34 -0000	1.38
+++ linux-2.5/include/net/tcp.h	15 Jun 2003 15:50:22 -0000
@@ -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 <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA