On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
-- Michael
On 5 October 2012 01:58, Michael Hope michael.hope@linaro.org wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
The struct looks like this:
struct iphdr { #if defined(__LITTLE_ENDIAN_BITFIELD) __u8 ihl:4, version:4; #elif defined (__BIG_ENDIAN_BITFIELD) __u8 version:4, ihl:4; #else #error "Please fix <asm/byteorder.h>" #endif __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
In a normal build (there's some magic for special checkers) __be32 is a plain __u32 so the struct should be at least 4-byte aligned. If somehow it is not, that is the real bug.
On 10/04/2012 08:26 PM, Mans Rullgard wrote:
On 5 October 2012 01:58, Michael Hope michael.hope@linaro.org wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
The struct looks like this:
struct iphdr { #if defined(__LITTLE_ENDIAN_BITFIELD) __u8 ihl:4, version:4; #elif defined (__BIG_ENDIAN_BITFIELD) __u8 version:4, ihl:4; #else #error "Please fix <asm/byteorder.h>" #endif __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
In a normal build (there's some magic for special checkers) __be32 is a plain __u32 so the struct should be at least 4-byte aligned. If somehow it is not, that is the real bug.
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
Rob
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
On 10/04/2012 08:26 PM, Mans Rullgard wrote:
On 5 October 2012 01:58, Michael Hope michael.hope@linaro.org wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
The struct looks like this:
struct iphdr { #if defined(__LITTLE_ENDIAN_BITFIELD) __u8 ihl:4, version:4; #elif defined (__BIG_ENDIAN_BITFIELD) __u8 version:4, ihl:4; #else #error "Please fix <asm/byteorder.h>" #endif __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
In a normal build (there's some magic for special checkers) __be32 is a plain __u32 so the struct should be at least 4-byte aligned. If somehow it is not, that is the real bug.
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
On 10/04/2012 09:25 PM, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
On 10/04/2012 08:26 PM, Mans Rullgard wrote:
On 5 October 2012 01:58, Michael Hope michael.hope@linaro.org wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
The struct looks like this:
struct iphdr { #if defined(__LITTLE_ENDIAN_BITFIELD) __u8 ihl:4, version:4; #elif defined (__BIG_ENDIAN_BITFIELD) __u8 version:4, ihl:4; #else #error "Please fix <asm/byteorder.h>" #endif __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
In a normal build (there's some magic for special checkers) __be32 is a plain __u32 so the struct should be at least 4-byte aligned. If somehow it is not, that is the real bug.
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
Unfortunately, declaring the struct or __be32* cast as packed have no effect. I still get an ldm emitted.
Rob
On Oct 4, 2012, at 8:04 PM, Rob Herring robherring2@gmail.com wrote:
On 10/04/2012 09:25 PM, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
On 10/04/2012 08:26 PM, Mans Rullgard wrote:
On 5 October 2012 01:58, Michael Hope michael.hope@linaro.org wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I think the assembly is correct - GCC knows that iph is aligned and knows the offsets of both x and id. Happy to be corrected if I'm wrong, but I think the assembly is valid given the C code.
The struct looks like this:
struct iphdr { #if defined(__LITTLE_ENDIAN_BITFIELD) __u8 ihl:4, version:4; #elif defined (__BIG_ENDIAN_BITFIELD) __u8 version:4, ihl:4; #else #error "Please fix <asm/byteorder.h>" #endif __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
In a normal build (there's some magic for special checkers) __be32 is a plain __u32 so the struct should be at least 4-byte aligned. If somehow it is not, that is the real bug.
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
Unfortunately, declaring the struct or __be32* cast as packed have no effect. I still get an ldm emitted.
what is value of r2 ? and can you pastebin the .i file somewhere ?
Rob
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
I don't think there's going to be a satisfactory answer to this issue. I think we're going to be stuck between GCC people saying that the kernel is buggy, and the network people refusing to fix this as they have done in the past.
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
On 5 October 2012 09:24, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter?
It matters if we want to fix it.
On Fri, Oct 05, 2012 at 09:33:04AM +0100, Mans Rullgard wrote:
On 5 October 2012 09:24, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter?
It matters if we want to fix it.
I can't help you there. Ask the networking people.
On 5 October 2012 09:33, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:33:04AM +0100, Mans Rullgard wrote:
On 5 October 2012 09:24, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: > This struct is the IP header, so a struct ptr is just set to the > beginning of the received data. Since ethernet headers are 14 bytes, > often the IP header is not aligned unless the NIC can place the frame at > a 2 byte offset (which is something I need to investigate). So this > function cannot make any assumptions about the alignment. Does the ABI > define structs have some minimum alignment? Does the struct need to be > declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter?
It matters if we want to fix it.
I can't help you there. Ask the networking people.
I'm asking anyone who is listening. Since you've (apparently) seen the claim being made, perhaps you can point to an email or so with more information. I'd hate to think you're just making up excuses.
On Fri, Oct 05, 2012 at 09:37:38AM +0100, Mans Rullgard wrote:
On 5 October 2012 09:33, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:33:04AM +0100, Mans Rullgard wrote:
On 5 October 2012 09:24, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote: > On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: > > This struct is the IP header, so a struct ptr is just set to the > > beginning of the received data. Since ethernet headers are 14 bytes, > > often the IP header is not aligned unless the NIC can place the frame at > > a 2 byte offset (which is something I need to investigate). So this > > function cannot make any assumptions about the alignment. Does the ABI > > define structs have some minimum alignment? Does the struct need to be > > declared as packed or something? > > The ABI defines the alignment of structs as the maximum alignment of its > members. Since this struct contains 32-bit members, the alignment for the > whole struct becomes 32 bits as well. Declaring it as packed tells gcc it > might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter?
It matters if we want to fix it.
I can't help you there. Ask the networking people.
I'm asking anyone who is listening. Since you've (apparently) seen the claim being made, perhaps you can point to an email or so with more information. I'd hate to think you're just making up excuses.
Sorry, I'm just not going to bother - especially given your attitude of "you're just making up excuses" - you just accused me of being a liar. Thanks for that.
We have the merge window open at the moment, and I've only just got back from being on holiday for the first half of that - so I have new tree conflicts to deal with, and need to finish preparing my tree for Linus. I'm not going to waste the precious time that the merge window is open for searching lots of mail archives for this when others can do the same, or even try sending the Linux networking people a patch adding __packed to their structures and seeing the response.
Mans Rullgard writes:
On 5 October 2012 09:33, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:33:04AM +0100, Mans Rullgard wrote:
On 5 October 2012 09:24, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote: > On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: > > This struct is the IP header, so a struct ptr is just set to the > > beginning of the received data. Since ethernet headers are 14 bytes, > > often the IP header is not aligned unless the NIC can place the frame at > > a 2 byte offset (which is something I need to investigate). So this > > function cannot make any assumptions about the alignment. Does the ABI > > define structs have some minimum alignment? Does the struct need to be > > declared as packed or something? > > The ABI defines the alignment of structs as the maximum alignment of its > members. Since this struct contains 32-bit members, the alignment for the > whole struct becomes 32 bits as well. Declaring it as packed tells gcc it > might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter?
It matters if we want to fix it.
I can't help you there. Ask the networking people.
I'm asking anyone who is listening. Since you've (apparently) seen the claim being made, perhaps you can point to an email or so with more information. I'd hate to think you're just making up excuses.
I'm pretty sure I've seen DaveM make this statement (not allowing the packed attribute) and also stating that alignment faults in the kernel itself MUST be handled silently and correctly. (And DaveM is of course also the maintainer of SPARC, another machine with strict-alignment requirements.)
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
Rob
Rob Herring writes:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote:
This struct is the IP header, so a struct ptr is just set to the beginning of the received data. Since ethernet headers are 14 bytes, often the IP header is not aligned unless the NIC can place the frame at a 2 byte offset (which is something I need to investigate). So this function cannot make any assumptions about the alignment. Does the ABI define structs have some minimum alignment? Does the struct need to be declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
If you suspect a GCC bug, please prepare a standalone user-space test case and submit it to GCC's bugzilla (I can do the latter if you absolutely do not want to). It wouldn't be the first alignment-related GCC bug...
On 10/05/2012 08:51 AM, Mikael Pettersson wrote:
Rob Herring writes:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: > This struct is the IP header, so a struct ptr is just set to the > beginning of the received data. Since ethernet headers are 14 bytes, > often the IP header is not aligned unless the NIC can place the frame at > a 2 byte offset (which is something I need to investigate). So this > function cannot make any assumptions about the alignment. Does the ABI > define structs have some minimum alignment? Does the struct need to be > declared as packed or something?
The ABI defines the alignment of structs as the maximum alignment of its members. Since this struct contains 32-bit members, the alignment for the whole struct becomes 32 bits as well. Declaring it as packed tells gcc it might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
If you suspect a GCC bug, please prepare a standalone user-space test case and submit it to GCC's bugzilla (I can do the latter if you absolutely do not want to). It wouldn't be the first alignment-related GCC bug...
Here's a testcase. Compiled on ubuntu precise with "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16; typedef unsigned short __sum16; typedef unsigned int __u32; typedef unsigned char __u8; typedef __u32 __be32; typedef u16 __be16;
struct iphdr { __u8 ihl:4, version:4; __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
#define ntohl(x) __swab32((__u32)(__be32)(x)) #define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x) { __asm__ ("rev %0, %1" : "=r" (x) : "r" (x)); return x; }
int main(void * buffer, unsigned int *p_id) { unsigned int id; int flush = 1; const struct iphdr *iph = buffer; __u32 len = *p_id; id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ len) | (id ^ IP_DF)); id >>= 16; *p_id = id; return flush; }
On 5 October 2012 17:01, Rob Herring robherring2@gmail.com wrote:
Here's a testcase. Compiled on ubuntu precise with "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16; typedef unsigned short __sum16; typedef unsigned int __u32; typedef unsigned char __u8; typedef __u32 __be32; typedef u16 __be16;
struct iphdr { __u8 ihl:4, version:4; __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
#define ntohl(x) __swab32((__u32)(__be32)(x)) #define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x) { __asm__ ("rev %0, %1" : "=r" (x) : "r" (x)); return x; }
int main(void * buffer, unsigned int *p_id) { unsigned int id; int flush = 1; const struct iphdr *iph = buffer; __u32 len = *p_id;
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ len) | (id ^ IP_DF)); id >>= 16; *p_id = id; return flush;}
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
On Fri, Oct 05, 2012 at 11:37:40PM +0100, Mans Rullgard wrote:
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
Again, not going to happen. DaveM is on record for saying as much, but I guess you're going to reject that as well, so I'm not sure why I'm even bothering to reply.
On Fri, 5 Oct 2012, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 11:37:40PM +0100, Mans Rullgard wrote:
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
Again, not going to happen. DaveM is on record for saying as much, but I guess you're going to reject that as well, so I'm not sure why I'm even bothering to reply.
May I suggest to send a recap of this thread to netdev@vger.kernel.org and see what davem has to say this time? Otherwise we are just going in circle.
Nicolas
On 5 October 2012 23:42, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 11:37:40PM +0100, Mans Rullgard wrote:
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
Again, not going to happen.
There are only two options for fixing this:
1. Ensure the struct is always aligned. 2. Declare it packed (and fix casts).
Refusing to do either leaves us with a broken kernel. Is that what you want?
On Sat, 6 Oct 2012, Mans Rullgard wrote:
On 5 October 2012 23:42, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 11:37:40PM +0100, Mans Rullgard wrote:
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
Again, not going to happen.
There are only two options for fixing this:
- Ensure the struct is always aligned.
- Declare it packed (and fix casts).
Refusing to do either leaves us with a broken kernel. Is that what you want?
Once again, please bring this up with davem and also CC netdev@vger.kernel.org as he's the point of contention here.
Nicolas
On Sat, Oct 06, 2012 at 05:04:33PM +0100, Mans Rullgard wrote:
On 5 October 2012 23:42, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 11:37:40PM +0100, Mans Rullgard wrote:
The problem is the (__be32 *) casts. This is a normal pointer to a 32-bit, which is assumed to be aligned, and the cast overrides the packed attribute from the struct. Dereferencing these cast expressions must be done with the macros from asm/unaligned.h
Again, not going to happen.
There are only two options for fixing this:
- Ensure the struct is always aligned.
- Declare it packed (and fix casts).
Refusing to do either leaves us with a broken kernel. Is that what you want?
How about you start reading the emails that you receive rather than seemingly insisting that I somehow fix the Linux networking stack - which would involve me talking to someone who has publically tried to oust me from being ARM maintainer, and whom would probably reject any attempt to fix this _because_ I was the person involved in proposing the fix?
It's far better that someone else sorts this out, they are much more likely to get a favourable response.
Rob Herring writes:
On 10/05/2012 08:51 AM, Mikael Pettersson wrote:
Rob Herring writes:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote: > On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: >> This struct is the IP header, so a struct ptr is just set to the >> beginning of the received data. Since ethernet headers are 14 bytes, >> often the IP header is not aligned unless the NIC can place the frame at >> a 2 byte offset (which is something I need to investigate). So this >> function cannot make any assumptions about the alignment. Does the ABI >> define structs have some minimum alignment? Does the struct need to be >> declared as packed or something? > > The ABI defines the alignment of structs as the maximum alignment of its > members. Since this struct contains 32-bit members, the alignment for the > whole struct becomes 32 bits as well. Declaring it as packed tells gcc it > might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
If you suspect a GCC bug, please prepare a standalone user-space test case and submit it to GCC's bugzilla (I can do the latter if you absolutely do not want to). It wouldn't be the first alignment-related GCC bug...
Here's a testcase. Compiled on ubuntu precise with "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16; typedef unsigned short __sum16; typedef unsigned int __u32; typedef unsigned char __u8; typedef __u32 __be32; typedef u16 __be16;
struct iphdr { __u8 ihl:4, version:4; __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
#define ntohl(x) __swab32((__u32)(__be32)(x)) #define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x) { __asm__ ("rev %0, %1" : "=r" (x) : "r" (x)); return x; }
int main(void * buffer, unsigned int *p_id) { unsigned int id; int flush = 1; const struct iphdr *iph = buffer; __u32 len = *p_id; id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ len) | (id ^ IP_DF)); id >>= 16; *p_id = id; return flush; }
I was referring to your statement that adding __packed to the types involved didn't prevent GCC from emitting aligned memory accesses. The test case above only shows that if the source code lies to GCC then things break...
On 12-10-05 12:01 PM, Rob Herring wrote:
On 10/05/2012 08:51 AM, Mikael Pettersson wrote:
Rob Herring writes:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
On 5 October 2012 08:12, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote: > On 5 October 2012 02:56, Rob Herring robherring2@gmail.com wrote: >> This struct is the IP header, so a struct ptr is just set to the >> beginning of the received data. Since ethernet headers are 14 bytes, >> often the IP header is not aligned unless the NIC can place the frame at >> a 2 byte offset (which is something I need to investigate). So this >> function cannot make any assumptions about the alignment. Does the ABI >> define structs have some minimum alignment? Does the struct need to be >> declared as packed or something? > > The ABI defines the alignment of structs as the maximum alignment of its > members. Since this struct contains 32-bit members, the alignment for the > whole struct becomes 32 bits as well. Declaring it as packed tells gcc it > might be unaligned (in addition to removing any holes within).
This has come up before in the past.
The Linux network folk will _not_ allow - in any shape or form - for this struct to be marked packed (it's the struct which needs to be marked packed) because by doing so, it causes GCC to issue byte loads/ stores on architectures where there isn't a problem, and that decreases the performance of the Linux IP stack unnecessarily.
Which architectures? I have never seen anything like that.
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
If you suspect a GCC bug, please prepare a standalone user-space test case and submit it to GCC's bugzilla (I can do the latter if you absolutely do not want to). It wouldn't be the first alignment-related GCC bug...
Here's a testcase. Compiled on ubuntu precise with "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16; typedef unsigned short __sum16; typedef unsigned int __u32; typedef unsigned char __u8; typedef __u32 __be32; typedef u16 __be16;
struct iphdr { __u8 ihl:4, version:4; __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
I was reading this thread with some interest. AFAIK, with the default alignment rules the above struct is packed; there will be no holes in it.
#define ntohl(x) __swab32((__u32)(__be32)(x)) #define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x) { __asm__ ("rev %0, %1" : "=r" (x) : "r" (x)); return x; }
int main(void * buffer, unsigned int *p_id) { unsigned int id; int flush = 1; const struct iphdr *iph = buffer; __u32 len = *p_id; id = ntohl(*(__be32 *)&iph->id);
The above statement is the problem. I think it is poorly written networking code. It takes the address of a 16 bit quantity (aligned on a halfword address), attempts to do a type conversion using pointers, then dereference it. I would have thought:
id = ntohs(iph->id);
would have been enough.
Scott
On 9 October 2012 15:05, Scott Bambrough scott.bambrough@linaro.org wrote:
On 12-10-05 12:01 PM, Rob Herring wrote:
Here's a testcase. Compiled on ubuntu precise with "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16; typedef unsigned short __sum16; typedef unsigned int __u32; typedef unsigned char __u8; typedef __u32 __be32; typedef u16 __be16;
struct iphdr { __u8 ihl:4, version:4; __u8 tos; __be16 tot_len; __be16 id; __be16 frag_off; __u8 ttl; __u8 protocol; __sum16 check; __be32 saddr; __be32 daddr; /*The options start here. */ };
I was reading this thread with some interest. AFAIK, with the default alignment rules the above struct is packed; there will be no holes in it.
Correct. The problem here is that something is passing around a pointer to such a struct which is not 4-byte aligned as required by the ABI rules.
#define ntohl(x) __swab32((__u32)(__be32)(x)) #define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x) { __asm__ ("rev %0, %1" : "=r" (x) : "r" (x)); return x; }
int main(void * buffer, unsigned int *p_id) { unsigned int id; int flush = 1; const struct iphdr *iph = buffer; __u32 len = *p_id;
id = ntohl(*(__be32 *)&iph->id);The above statement is the problem. I think it is poorly written networking code. It takes the address of a 16 bit quantity (aligned on a halfword address),
The 'id' member starts 4 bytes into the struct, so if the struct is properly aligned, there will be no fault here. The problem is that networking code does not always align these structs correctly.
attempts to do a type conversion using pointers, then dereference it. I would have thought:
id = ntohs(iph->id);
would have been enough.
I'm assuming the is intentionally merging two 16-bit fields into one 32-bit value. What you suggest would do something rather different.
On Fri, Oct 05, 2012 at 07:24:44AM -0500, Rob Herring wrote:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
Even so, please test the patch I've sent you in the sub-thread - that needs testing whether or not GCC is at fault. Will's patch to add the warnings _has_ uncovered a potential issue with the use of __get_user() in some parts of the ARM specific kernel, and I really need you to test that while you're experiencing this problem.
On 10/05/2012 09:05 AM, Russell King - ARM Linux wrote:
On Fri, Oct 05, 2012 at 07:24:44AM -0500, Rob Herring wrote:
On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
Does it matter? I'm just relaying the argument against adding __packed which was used before we were forced (by the networking folk) to implement the alignment fault handler.
It doesn't really matter what will be accepted or not as adding __packed to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm. The only way I've found to eliminate the alignment fault is adding a barrier between the 2 loads. That seems like a compiler issue to me if there is not a better fix.
Even so, please test the patch I've sent you in the sub-thread - that needs testing whether or not GCC is at fault. Will's patch to add the warnings _has_ uncovered a potential issue with the use of __get_user() in some parts of the ARM specific kernel, and I really need you to test that while you're experiencing this problem.
I've tested your patch and it appears to fix things. Thanks!
Now on to getting rid of faults on practically every single received IP packet:
Multi: 9871002
RX packets:9872010 errors:0 dropped:0 overruns:0 frame:0
Rob
On 10/04/2012 07:58 PM, Michael Hope wrote:
On 5 October 2012 12:10, Rob Herring robherring2@gmail.com wrote:
I've been scratching my head with a "scheduling while atomic" bug I started seeing on 3.6. I can easily reproduce this problem when doing a wget on my system. It ultimately seems to be a combination of factors. The "scheduling while atomic" bug is triggered in do_alignment which gets triggered by this code in net/ipv4/af_inet.c, line 1356:
id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF)); id >>= 16;
This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)":
c02ac020: e8920840 ldm r2, {r6, fp} c02ac024: e6bfbf3b rev fp, fp c02ac028: e6bf6f36 rev r6, r6 c02ac02c: e22bc901 eor ip, fp, #16384 ; 0x4000 c02ac030: e0266008 eor r6, r6, r8 c02ac034: e18c6006 orr r6, ip, r6
which generates alignment faults on the ldm. These are silent until this commit is applied:
Hi Rob. I assume that iph is something like:
struct foo { u32 x; char id[8]; };
struct foo *iph;
GCC merged the two adjacent loads of x and id into one ldm. This is an ARM specific optimisation done in load_multiple_sequence() and enabled with -fpeephole2.
I'm probably on some watch list now after searching for peephole...
It's not clear what all turning this off would affect. Is it just struct or array loading? Or does this turn off lots of different optimizations?
Rob
linaro-toolchain@lists.linaro.org
-
Khem Raj -
Mans Rullgard -
Michael Hope -
Mikael Pettersson -
Nicolas Pitre -
Rob Herring -
Russell King - ARM Linux -
Scott Bambrough