Excess use of packed attribute

Excess use of packed attribute

[Stephen Hemminger]

After reading:
	http://bugzilla.kernel.org/show_bug.cgi?id=6693

I noticed there were stupid uses of packed attribute in several network headers.

Silly offenders:	include/net/ipx.h
			include/net/ieee80211.h
			include/net/ip6_tunnel.h
			include/net/ndisc.h
			include/linux/if_ether.h
			include/linux/if_fddi.h
		
			include/linux/sctp.h -- really bad
		
-- 

Re: Excess use of packed attribute

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 7 Aug 2006 13:34:23 -0700

> Silly offenders:	include/net/ipx.h
> 			include/net/ieee80211.h
> 			include/net/ip6_tunnel.h
> 			include/net/ndisc.h
> 			include/linux/if_ether.h
> 			include/linux/if_fddi.h
> 		
> 			include/linux/sctp.h -- really bad

The ndisc.h one, for example, is needed for cases like ARM.

The if_ether.h one is also needed, or else for:

	struct ethhdr *eth;

"eth + 1" would do the wrong thing as the compiler would
align the structure to the native pointer size or similar.
This is an issue because ethhdr is 14 bytes in size.

Re: Excess use of packed attribute

[Sridhar Samudrala]

On Mon, 2006-08-07 at 13:34 -0700, Stephen Hemminger wrote:
> After reading:
> 	http://bugzilla.kernel.org/show_bug.cgi?id=6693
> 
> I noticed there were stupid uses of packed attribute in several network headers.
> 
> Silly offenders:	include/net/ipx.h
> 			include/net/ieee80211.h
> 			include/net/ip6_tunnel.h
> 			include/net/ndisc.h
> 			include/linux/if_ether.h
> 			include/linux/if_fddi.h
> 		
> 			include/linux/sctp.h -- really bad

All the structures in sctp.h that use packed atrribute define
standard on-wire SCTP chunk/parameter formats. They need to be at the
exact offsets as they go on wire.
I think we saw some issues without the packed attribute on 64-bit archs
and just to be safe we added packed to all the on-wire structures.

Thanks
Sridhar

Re: Excess use of packed attribute

[David Miller]

From: Sridhar Samudrala <sri@us.ibm.com>
Date: Mon, 07 Aug 2006 17:02:08 -0700

> All the structures in sctp.h that use packed atrribute define
> standard on-wire SCTP chunk/parameter formats. They need to be at the
> exact offsets as they go on wire.
> I think we saw some issues without the packed attribute on 64-bit archs
> and just to be safe we added packed to all the on-wire structures.

For the cases where it is no actually necessary, the code generation
cost on RISC cpus is very high.  Byte loads and stores will be used to
access _every_ member of such structures on RISC machines because
the compiler cannot guarentee the alignment of any data object
when packed is specified.

It's OK to do this when it's really needed, but when it's not it
should be heavily avoided.

Re: Excess use of packed attribute

[Roland Dreier]

    David> For the cases where it is no actually necessary, the code
    David> generation cost on RISC cpus is very high.  Byte loads and
    David> stores will be used to access _every_ member of such
    David> structures on RISC machines because the compiler cannot
    David> guarentee the alignment of any data object when packed is
    David> specified.

Agreed (although not really RISC -- sparc64 and ia64 have this
problem, while ppc is fine with unaligned access).  However
__attribute__((packed,aligned)) has just been brought to my attention.
For example, on sparc64,

	struct foo { char x; int a; } __attribute__((packed,aligned));
	struct bar { char x; int b; } __attribute__((packed));
	
	int c(struct foo *x) { return x->a; }
	int d(struct bar *x) { return x->b; }

compiles to:

	0000000000000000 <c>:
	   0:   d0 5a 00 00     ldx  [ %o0 ], %o0
	   4:   91 2a 30 08     sllx  %o0, 8, %o0
	   8:   81 c3 e0 08     retl
	   c:   91 3a 30 20     srax  %o0, 0x20, %o0
	  10:   30 68 00 04     b,a   %xcc, 20 <d>
	  14:   01 00 00 00     nop
	  18:   01 00 00 00     nop
	  1c:   01 00 00 00     nop
	
	0000000000000020 <d>:
	  20:   c6 0a 20 01     ldub  [ %o0 + 1 ], %g3
	  24:   c2 0a 20 02     ldub  [ %o0 + 2 ], %g1
	  28:   c4 0a 20 03     ldub  [ %o0 + 3 ], %g2
	  2c:   87 28 f0 18     sllx  %g3, 0x18, %g3
	  30:   d0 0a 20 04     ldub  [ %o0 + 4 ], %o0
	  34:   83 28 70 10     sllx  %g1, 0x10, %g1
	  38:   82 10 40 03     or  %g1, %g3, %g1
	  3c:   85 28 b0 08     sllx  %g2, 8, %g2
	  40:   84 10 80 01     or  %g2, %g1, %g2
	  44:   90 12 00 02     or  %o0, %g2, %o0
	  48:   81 c3 e0 08     retl
	  4c:   91 3a 20 00     sra  %o0, 0, %o0
	  50:   30 68 00 04     b,a   %xcc, 60 <d+0x40>
	  54:   01 00 00 00     nop
	  58:   01 00 00 00     nop
	  5c:   01 00 00 00     nop

which suggests that adding "aligned" is a good idea for many of the
uses of "packed".  However I don't know how all gcc version do with
this.  Anyone have any comments on "__attribute__((packed,aligned))"?

Thanks,
  Roland

Re: Excess use of packed attribute

[Ville Nuorvala]

David Miller wrote:
> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Mon, 7 Aug 2006 13:34:23 -0700
> 
>> Silly offenders:	include/net/ipx.h
>> 			include/net/ieee80211.h
>> 			include/net/ip6_tunnel.h
>> 			include/net/ndisc.h
>> 			include/linux/if_ether.h
>> 			include/linux/if_fddi.h
>> 		
>> 			include/linux/sctp.h -- really bad
> 
> The ndisc.h one, for example, is needed for cases like ARM.
> 
> The if_ether.h one is also needed, or else for:
> 
> 	struct ethhdr *eth;
> 
> "eth + 1" would do the wrong thing as the compiler would
> align the structure to the native pointer size or similar.
> This is an issue because ethhdr is 14 bytes in size.

Similarly the ipv6_tlv_tnl_enc_lim in ip6_tunnel.h is a 3-octet IPv6
destination sub-option. The sub-option can be located anywhere inside
the destination option header and can only be found by parsing
the whole header.

Regards,
Ville

Re: Excess use of packed attribute

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Wed, 09 Aug 2006 11:37:31 -0700

> Agreed (although not really RISC -- sparc64 and ia64 have this
> problem, while ppc is fine with unaligned access).  However
> __attribute__((packed,aligned)) has just been brought to my attention.
> For example, on sparc64,
> 
> 	struct foo { char x; int a; } __attribute__((packed,aligned));
> 	struct bar { char x; int b; } __attribute__((packed));
> 	
> 	int c(struct foo *x) { return x->a; }
> 	int d(struct bar *x) { return x->b; }
> 
> compiles to:
 ...
> which suggests that adding "aligned" is a good idea for many of the
> uses of "packed".  However I don't know how all gcc version do with
> this.  Anyone have any comments on "__attribute__((packed,aligned))"?

Good find.

It might usable in some of the questionable cases, however it changes
the size of the struct which eliminates most of the gains.

For example:

davem@sunset:~/src/GIT/net-2.6$ cat foo.c
#include <stdio.h>

struct foo_packed {
	char x; int a;
} __attribute__((packed));

struct foo_packed_aligned {
	char x; int a;
} __attribute__((packed,aligned));

int main(void)
{
	printf("foo_packed: sizeof(%Zd)\n",
	       sizeof(struct foo_packed));
	printf("foo_packed_aligned: sizeof(%Zd)\n",
	       sizeof(struct foo_packed_aligned));

	return 0;
}
davem@sunset:~/src/GIT/net-2.6$ gcc -O -o foo foo.c
davem@sunset:~/src/GIT/net-2.6$ ./foo
foo_packed: sizeof(5)
foo_packed_aligned: sizeof(8)
davem@sunset:~/src/GIT/net-2.6$ 

This doesn't work for things like packet headers, for example.

Oh well.