Jerry Van Baren wrote:

Ben Warren wrote:

...

Luigi 'Comio' Mantellini wrote:

...

Hi ML,

I'm working on a mips target and I used qemu_mips target to simulate my target (that I hope to have in the next week...)

Following my activities I noticed that IP_t structure is no defined with attribute "packed". I noticed this issue because using a self-made toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 32bit boundary. This is not ok, because the packets IP_t can be non aligned (see the /net/net.c PingSend function, for an example).

Why is your compiler aligning all bytes to 32-bit boundary? Seems like an awful waste of space. This struct should pack itself nicely, and does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and gcc 4.0.0 ppc_4xx).

The compiler is optimizing for speed and/or execution size at the expense of larger data structures either by command (e.g. a -O option) or as part of the compiler writer's choice. CPUs almost always execute code significantly faster when the data is properly aligned. Many CPUs require software to deal with the misalignment which costs code space and execution time.

Since the compiler wasn't instructed that the IP headers needed to be packed, it is within the compiler's right to not pack them.

Sure. In this case, though, the optimization's not necessarily going to gain anything since we never use a raw struct IP_t, only pointers that overlay other char arrays through casting. Of course there's no point discussing this much further here, but I suspect that this packing problem will exist in many more places than the protocol headers.

[snip]

Best regards, gvb

regards, Ben

Luigi Mantellini wrote:

Dear All

From my point of view, when packing is formally required (ie packets headers), the structs should be declared explicitly as __packed__. The correctness of the object code should be independent from the compiler optimizations and we should always remember that the offset of a struct field is not necessarily the sum of the sizes of the fields that precede it.

struct { type1 a; type2 b; type3 c; } mystruct

offset(mystruct.c) != sizeof(type1) + sizeof(type2).

Regarding the CFLAGS used by me... I haven't set any CFLAGS and I just do a make qemu_mips_config CROSS_COMPILE=mips-linux- && make CROSS_COMPILE=mips-linux... and a boundary alignment is not an alien choice for a good compiler (a standard gcc4.2.4 in my case). Furthermore I expect a correct object always... with any -Ox flag (a apart bugs... of course).

My idea should be to declare a define like this

#define PKT_HEADER __attribute__((__packed__))

my 2EuroCents.

best regards,

luigi

OK, sounds good. Send a patch please.

regards, Ben <snip>

Wolfgang Denk wrote:

Dear Ben Warren,

In message 4980CC59.1070302@gmail.com you wrote:

...

...

My idea should be to declare a define like this

#define PKT_HEADER __attribute__((__packed__))

my 2EuroCents.

best regards,

luigi

OK, sounds good. Send a patch please.

Hm... and what does this give us?

How long until sombebody detects that all the data structures describing device register layout or so many processors and chips don't use any __packed__ either? Except for some (IMHO broken, but I know that I'm more attached to PowerPC anyway) ARM systems it is sufficient to use a "sane" selection of data types.

Well, yeah, I alluded to that in my other e-mail. I'm not crazy about this either, but don't see any significant downside. Obviously if this was a common problem, it would have surfaced a long time ago...

I vote against changing this code. Just for a quick cross-check: do the corresponding data structs in the Linux kernel use any __packed__?

Here is for example a copy of /usr/include/netinet/ip.h :

... struct iphdr { #if __BYTE_ORDER == __LITTLE_ENDIAN unsigned int ihl:4; unsigned int version:4; #elif __BYTE_ORDER == __BIG_ENDIAN unsigned int version:4; unsigned int ihl:4; #else # error "Please fix <bits/endian.h>" #endif u_int8_t tos; u_int16_t tot_len; u_int16_t id; u_int16_t frag_off; u_int8_t ttl; u_int8_t protocol; u_int16_t check; u_int32_t saddr; u_int32_t daddr; /*The options start here. */ }; ...

Do you see any __packed__?

Yeah, I made the same observation, but am not fluent enough in the black magic of Linux header files to know if packing was being enforced somewhere else or in some other way that my little brain can't comprehend.

Best regards,

Wolfgang Denk

regards, Ben

Dear All,

2009/1/28 Wolfgang Denk wd@denx.de:

Dear Ben Warren,

In message 4980CC59.1070302@gmail.com you wrote:

...

...

My idea should be to declare a define like this

#define PKT_HEADER __attribute__((__packed__))

my 2EuroCents.

best regards,

luigi

OK, sounds good. Send a patch please.

I think that an audit of the code is important to understand if we have a problem (or not) and how large is the problem.

Hm... and what does this give us?

How long until sombebody detects that all the data structures describing device register layout or so many processors and chips don't use any __packed__ either? Except for some (IMHO broken, but I know that I'm more attached to PowerPC anyway) ARM systems it is sufficient to use a "sane" selection of data types.

My compiler is not broken...

I vote against changing this code. Just for a quick cross-check: do the corresponding data structs in the Linux kernel use any __packed__?

cassini linux # head Makefile -n5 VERSION = 2 PATCHLEVEL = 6 SUBLEVEL = 28 EXTRAVERSION = -tuxonice-r1 NAME = Erotic Pickled Herring cassini linux # find -name *.c -o -name *.h |xargs grep attribute | grep packed | wc -l 3153

I see a lot of packed structs...

Here is for example a copy of /usr/include/netinet/ip.h :

... struct iphdr { #if __BYTE_ORDER == __LITTLE_ENDIAN unsigned int ihl:4; unsigned int version:4; #elif __BYTE_ORDER == __BIG_ENDIAN unsigned int version:4; unsigned int ihl:4; #else # error "Please fix <bits/endian.h>" #endif u_int8_t tos; u_int16_t tot_len; u_int16_t id; u_int16_t frag_off; u_int8_t ttl; u_int8_t protocol; u_int16_t check; u_int32_t saddr; u_int32_t daddr; /*The options start here. */ }; ...

Do you see any __packed__?

This doesn't say anything regarding how kernel guys have resolved the problem (if they are solved...). Checking the kernel headers a lot (all?) of structs that refers to drivers and internal structures are defined using the attribute packed. For me this shows that somebody asked himself if packed is needed or not. I think that "struct packing" needs to be understood and a global mechanism should be used (like -fpack-struct option always defined or a style guideline that requires a tag for each structs). From my point of view, we cannot define as wrong a compiler that follows different choices that are anyway conform to the language standard.

My usual 2Eurocents.

best regards,

luigi

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de The C-shell doesn't parse. It adhoculates.

• Casper.Dik@Holland.Sun.COM in 3ol96k$b2j@engnews2.Eng.Sun.COM

Dear Wolfgang,

2009/1/28 Wolfgang Denk wd@denx.de:

Dear Luigi Mantellini,

In message b73e93990901281416k766b2bf3qc6429f77545f9191@mail.gmail.com you wrote:

...

I think that an audit of the code is important to understand if we have a problem (or not) and how large is the problem.

We (i. e. all of us except you) do not have a problem.

my question is: how can you be sure on this? I haven't used strange compilers or strange CFLAGS.. I just do "make" on a supported target (qemu_mips). I haven't any problem to correct my local source tree. I ask myself why gcc offers a packed atribute, ms-vc offers pragma packed, ...

...

My compiler is not broken...

Well, YMMV...

the behavior is clear: in my environment, the default choice is to align fields on 32bit for speed reasons... and I like this by default for my applications. I can ignore the problem using options like -Os (I will try tomorrow) or -fpack-struct or other global mechanisms or, pay attention on structures definitions to be sure that the structure size is compiler and cflags independent.

...

cassini linux # find -name *.c -o -name *.h |xargs grep attribute | grep packed | wc -l 3153

I see a lot of packed structs...

...

...

Here is for example a copy of /usr/include/netinet/ip.h :

...

...

This doesn't say anything regarding how kernel guys have resolved the problem (if they are solved...). Checking the kernel headers a lot

This is not a kernel header. This is one of the standard user space network headers, and a pretty central one. Feel free to check any others.

I see. I would like to understand why this structure is optimization-prof. I will study tomorrow...

...

I think that "struct packing" needs to be understood and a global

I agree on this. This definitely needs to be understood.

Hm... I think I understand it, and it works for me :-)

iso C doensn't require packing by default of the structures. To assume that a structure is packed by default is not a good assumption. All compilers offer directives to control the behavior of packing... I believe that there is a reason...

...

mechanism should be used (like -fpack-struct option always defined or a style guideline that requires a tag for each structs). From my point

I think this is not needed. Please read the docuemntation about alignment and padding. It is pretty clear.

which documentation should I read? iso c documents? gcc manuals? I haven't found anything that opposes my affirmations. I want underline that the structures (IP_t, ... ) don't have field across the word-machine boundary... but this doesn't exclude an "memory-access" optimization. Kindly, can you give me any good links?

Anyway, I don't want to talk about philosophy. I just noticed an anomaly and I wanted to share with the ML.

my 2LireItaliane

best regards,

luigi

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "And it should be the law: If you use the word `paradigm' without knowing what the dictionary says it means, you go to jail. No exceptions." - David Jones @ Megatest Corporation

Now study the data structures used for netwrking - they are all carefully crafted that the natural aligment "just fits", i. e. there are no gaps between variables if they are aligned naturally.

Actually, a packed structure not only doesn't have gaps, but can live at any address. So the compiler must access it byte-by-byte regardless. This means that a naturally-aligned structure performs much worse if declared as packed. And single-word loads and stores are not atomic any more.

I tried with eldk-4.2 for arm and an older gcc for mips, I have no ppc or other archs handy right now:

#include <stdint.h> #include <stdio.h>

#ifdef PACKEDTEST # define P __attribute__ ((packed)) #else # define P #endif

struct {uint32_t i; uint16_t s; uint8_t c, d;} P datum;

uint32_t i; uint16_t s; uint8_t c;

int main(int argc, char **argv) { printf("ih\n"); i = datum.i; printf("oh\n"); s = datum.s; printf("ah\n"); c = datum.c; return 0; }

Compilation: ARM non-packed (only interesting lines):

8378: e5942000 ldr r2, [r4] 8384: e5832000 str r2, [r3]

838c: e1d420b4 ldrh r2, [r4, #4] 8398: e1c320b0 strh r2, [r3]

83a0: e5d42006 ldrb r2, [r4, #6] 83ac: e5c32000 strb r2, [r3]

Compilation: ARM packed (only interesting lines):

8378: e5d42001 ldrb r2, [r4, #1] 837c: e5d43000 ldrb r3, [r4] 8380: e5d40002 ldrb r0, [r4, #2] 8384: e5d41003 ldrb r1, [r4, #3] 8388: e1833402 orr r3, r3, r2, lsl #8 838c: e1833800 orr r3, r3, r0, lsl #16 8394: e1833c01 orr r3, r3, r1, lsl #24 8398: e5823000 str r3, [r2]

83a4: e5d43005 ldrb r3, [r4, #5] 83a8: e5d42004 ldrb r2, [r4, #4] 83b0: e1822403 orr r2, r2, r3, lsl #8 83b8: e1c320b0 strh r2, [r3]

83c0: e5d42006 ldrb r2, [r4, #6] 83cc: e5c32000 strb r2, [r3]

Compilation: MIPS non-packed (only interesting lines):

44: 8e030000 lw v1,0(s0) 5c: ac230000 sw v1,0(at)

74: 96030004 lhu v1,4(s0) 8c: a4230000 sh v1,0(at)

a4: 92030006 lbu v1,6(s0) bc: a0230000 sb v1,0(at)

Compilation: MIPS packed (only interesting lines):

44: 8a030003 lwl v1,3(s0) 48: 9a030000 lwr v1,0(s0) 60: ac230000 sw v1,0(at)

78: 92030005 lbu v1,5(s0) 7c: 92020004 lbu v0,4(s0) 80: 00031a00 sll v1,v1,0x8 84: 00621825 or v1,v1,v0 9c: a4230000 sh v1,0(at)

b4: 92030006 lbu v1,6(s0) cc: a0230000 sb v1,0(at)

/alessandro

Dear Luigi Mantellini,

In message b73e93990901281258s37f7e4d6hc6c2ff4c5d5fa47b@mail.gmail.com you wrote:

...

I'd focus on fixing your toolchain. Your problem will not be confined to protocol headers.

my toolchain works fine ;)

Except that it adds padding where it doesn't make sense. I think you want to fix it.

Best regards,

Wolfgang Denk