alignment faults in 3.6

"David Laight" [off-list ref] writes:

quoted

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Måns Rullgård
Sent: 11 October 2012 03:27
To: Jon Masters
Cc: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org
Subject: Re: alignment faults in 3.6

Jon Masters [off-list ref] writes:

quoted

Hi everyone,

On 10/05/2012 10:33 AM, Rob Herring wrote:

quoted

On 10/05/2012 09:05 AM, Russell King - ARM Linux wrote:

quoted

On Fri, Oct 05, 2012 at 07:24:44AM -0500, Rob Herring wrote:

quoted

On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:

quoted

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. 

...

quoted

There are exactly two possible solutions:

1. Change the networking code so those structs are always aligned.  This
   might not be (easily) possible.
2. Mark the structs __packed and fix any typecasts like the ones seen in
   this thread.  This will have an adverse effect in cases where the
   structs are in fact aligned.

Both solutions lie squarely in the networking code.  It's time to
involve that list, or we'll never get anywhere.

It might be enough to use __attribute__((aligned(2))) on some structure
members (actually does 'ldm' need 8 byte alignment?? - in which case
aligned(4) is enough).

The aligned attribute can only increase alignment.

-- 
Måns Rullgård
mans@mansr.com

On Thu, 2012-10-11 at 10:45 +0100, Måns Rullgård wrote:

"David Laight" [off-list ref] writes:

quoted

quoted

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Måns Rullgård
Sent: 11 October 2012 03:27
To: Jon Masters
Cc: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org
Subject: Re: alignment faults in 3.6

Jon Masters [off-list ref] writes:

quoted

Hi everyone,

On 10/05/2012 10:33 AM, Rob Herring wrote:

quoted

On 10/05/2012 09:05 AM, Russell King - ARM Linux wrote:

quoted

On Fri, Oct 05, 2012 at 07:24:44AM -0500, Rob Herring wrote:

quoted

On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:

quoted

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. 

...

quoted

There are exactly two possible solutions:

1. Change the networking code so those structs are always aligned.  This
   might not be (easily) possible.
2. Mark the structs __packed and fix any typecasts like the ones seen in
   this thread.  This will have an adverse effect in cases where the
   structs are in fact aligned.

Both solutions lie squarely in the networking code.  It's time to
involve that list, or we'll never get anywhere.

It might be enough to use __attribute__((aligned(2))) on some structure
members (actually does 'ldm' need 8 byte alignment?? - in which case
aligned(4) is enough).

The aligned attribute can only increase alignment.

I have no idea what is the problem, 

-ENOTENOUGHCONTEXT

quoted

It might be enough to use __attribute__((aligned(2))) on some structure
members (actually does 'ldm' need 8 byte alignment?? - in which case
aligned(4) is enough).

The aligned attribute can only increase alignment.

Not true.
If you have:

struct foo {
    short   a;
    int     b __attribute__((aligned(2)));
    short   c;
};

You'll find sizeof (struct foo) is 8, and that, on arm/sparc etc,
gcc will generate 2 16bit accesses (and shifts) for foo.b;

	David

On Thu, 2012-10-11 at 12:00 +0200, Eric Dumazet wrote:

On Thu, 2012-10-11 at 10:45 +0100, Måns Rullgård wrote:

I have no idea what is the problem, 

-ENOTENOUGHCONTEXT

I took a look, and I dont see why/how gcc could use a ldm instruction

Doing so assumed the alignment of the structure was 8 bytes, but its
not.

Networking stack mandates that IP headers are aligned on 4 bytes
boundaries, not 8 bytes.

(Some arches like x86 dont care, so we might have some bugs in some
drivers, but not in the GRO code)

Some drivers are not aware of the NET_IP_ALIGN stuff.
They should be fixed, or else you have alignment faults.

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. */
};

The alignment of iphdr is 4, not 8

doing id = ntohl(*(__be32 *)&iph->id); is valid

doing flush = (u16)((ntohl(*(__be32 *)iph) ^ len) | (id ^ IP_DF)); is valid as well.

If arm compiler decided to use a 8bytes load, thats a compiler bug.

(unless compiler was specifically told that alignment did not matter)

c02ac020:       e8920840        ldm     r2, {r6, fp}    // HERE
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

On Thu, Oct 11, 2012 at 12:22:06PM +0200, Eric Dumazet wrote:

I took a look, and I dont see why/how gcc could use a ldm instruction

Doing so assumed the alignment of the structure was 8 bytes, but its
not.

Networking stack mandates that IP headers are aligned on 4 bytes
boundaries, not 8 bytes.

Err, no.  ldm is "load multiple" not "load double".  It loads multiple
32-bit registers, and its requirement for non-faulting behaviour is for
the pointer to be 4 byte aligned.  However, "load double" requires 8
byte alignment.

On Thu, 2012-10-11 at 11:32 +0100, Russell King - ARM Linux wrote:

On Thu, Oct 11, 2012 at 12:22:06PM +0200, Eric Dumazet wrote:

quoted

I took a look, and I dont see why/how gcc could use a ldm instruction

Doing so assumed the alignment of the structure was 8 bytes, but its
not.

Networking stack mandates that IP headers are aligned on 4 bytes
boundaries, not 8 bytes.

Err, no.  ldm is "load multiple" not "load double".  It loads multiple
32-bit registers, and its requirement for non-faulting behaviour is for
the pointer to be 4 byte aligned.  However, "load double" requires 8
byte alignment.

So if you have an alignment fault, thats because IP header is not
aligned on 4 bytes ?

If so a driver is buggy and must be fixed.

Please send us full stack trace

On Thu, 2012-10-11 at 12:49 +0200, Eric Dumazet wrote:

So if you have an alignment fault, thats because IP header is not
aligned on 4 bytes ?

If so a driver is buggy and must be fixed.

So a driver that does not align the ip header is buggy ?

I always thought it was ok not to do so (with a potential performance
penalty).

I have some MIPS hardware that is not able to DMA on anything but 32bits
aligned addresses (bcm63xx). I tried once to add a memcpy instead of
taking unaligned faults and the result was *much slower* on a ipv4
forwarding test (which is what the hardware is used for).

-- 
Maxime

On Thu, 2012-10-11 at 12:56 +0200, Maxime Bizon wrote:

On Thu, 2012-10-11 at 12:49 +0200, Eric Dumazet wrote:

quoted

So if you have an alignment fault, thats because IP header is not
aligned on 4 bytes ?

If so a driver is buggy and must be fixed.

So a driver that does not align the ip header is buggy ?

exactly.

I always thought it was ok not to do so (with a potential performance
penalty).

Apparently not for some arches.

I have some MIPS hardware that is not able to DMA on anything but 32bits
aligned addresses (bcm63xx). I tried once to add a memcpy instead of
taking unaligned faults and the result was *much slower* on a ipv4
forwarding test (which is what the hardware is used for).

You probably are aware that a driver can use : 

- a fragment to hold the frame given by the hardware, with whatever
alignment is needed by the hardware.

Then allocate an skb with enough room (128 bytes) to pull the headers as
needed later.

skb = netdev_alloc_skb_ip_align(dev, 128);

Attach the fragment to the skb, and feed stack with this skb.
(pull the ethernet header before calling eth_type_trans()

Most modern drivers exactly use this strategy and get nice performance.

Of course, if hardware doesnt need a strange alignment, we can avoid the
fragment and directly use 

skb = netdev_alloc_skb_ip_align(dev, 1536);

So all drivers can be fixed if needed.

On Thu, 2012-10-11 at 13:28 +0200, Eric Dumazet wrote:

You probably are aware that a driver can use : 

- a fragment to hold the frame given by the hardware, with whatever
alignment is needed by the hardware.

Then allocate an skb with enough room (128 bytes) to pull the headers as
needed later.

skb = netdev_alloc_skb_ip_align(dev, 128);

What happen at tx time, supposing that same hardware cannot do SG ?

Aren't we going to memcpy the data into the head ?

-- 
Maxime

On Thu, 2012-10-11 at 13:47 +0200, Maxime Bizon wrote:

On Thu, 2012-10-11 at 13:28 +0200, Eric Dumazet wrote:

quoted

You probably are aware that a driver can use : 

- a fragment to hold the frame given by the hardware, with whatever
alignment is needed by the hardware.

Then allocate an skb with enough room (128 bytes) to pull the headers as
needed later.

skb = netdev_alloc_skb_ip_align(dev, 128);

What happen at tx time, supposing that same hardware cannot do SG ?

Aren't we going to memcpy the data into the head ?

Of course, if you use a forwarding setup, and the tx driver is not SG
capable, performance will be bad (You have to copy the data into a
single skb (linearize the skb)) 

But in 2012, having to use hardware without SG for a router sounds a bad
joke (if cpu speed is _also_ too low)

Adding get_unaligned() everywhere in linux network stacks is not an
option.

We actually want to be able to read the code and fix the bugs, not only
run it on a cheap low end router.

On Thu, 2012-10-11 at 13:54 +0200, Eric Dumazet wrote:

On Thu, 2012-10-11 at 13:47 +0200, Maxime Bizon wrote:

quoted

On Thu, 2012-10-11 at 13:28 +0200, Eric Dumazet wrote:

quoted

You probably are aware that a driver can use : 

- a fragment to hold the frame given by the hardware, with whatever
alignment is needed by the hardware.

Then allocate an skb with enough room (128 bytes) to pull the headers as
needed later.

skb = netdev_alloc_skb_ip_align(dev, 128);

What happen at tx time, supposing that same hardware cannot do SG ?

Aren't we going to memcpy the data into the head ?

Of course, if you use a forwarding setup, and the tx driver is not SG
capable, performance will be bad (You have to copy the data into a
single skb (linearize the skb)) 

By the way, if said driver also has alignments issues, it will probably
copy the packet given by the stack. (Think you added an IPIP
encapsulation)

It would be better for such driver to still pretend it can do SG,
and it does the copy itself, avoiding the prior copies in core stack.

On Thursday 11 October 2012, Eric Dumazet wrote:

On Thu, 2012-10-11 at 11:32 +0100, Russell King - ARM Linux wrote:

quoted

On Thu, Oct 11, 2012 at 12:22:06PM +0200, Eric Dumazet wrote:

quoted

I took a look, and I dont see why/how gcc could use a ldm instruction

Doing so assumed the alignment of the structure was 8 bytes, but its
not.

Networking stack mandates that IP headers are aligned on 4 bytes
boundaries, not 8 bytes.

Err, no.  ldm is "load multiple" not "load double".  It loads multiple
32-bit registers, and its requirement for non-faulting behaviour is for
the pointer to be 4 byte aligned.  However, "load double" requires 8
byte alignment.

So if you have an alignment fault, thats because IP header is not
aligned on 4 bytes ?

If so a driver is buggy and must be fixed.

Please send us full stack trace

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

	Arnd

On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

Thanks Arnd

It seems an accident, since driver doesnt check skb->data alignment at
all (this can change with SLAB debug on/off)

It also incorrectly adds 64 bytes to bfsize, there is no need for this.

(or if its needed, a comment would be nice, because on prior kernels,
this makes skb->head allocations uses kmalloc-4096 instead of
kmalloc-2048 slab cache... With 3.7 its less an issue now we use order-3
pages to deliver fragments for rx skbs

So the following patch should fix the alignment, and makes driver uses
half memory than before for stable kernels

diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
index 16814b3..a895e18 100644
--- a/drivers/net/ethernet/calxeda/xgmac.c
+++ b/drivers/net/ethernet/calxeda/xgmac.c

@@ -671,7 +671,8 @@ static void xgmac_rx_refill(struct xgmac_priv *priv)
 		p = priv->dma_rx + entry;
 
 		if (priv->rx_skbuff[entry] == NULL) {
-			skb = netdev_alloc_skb(priv->dev, priv->dma_buf_sz);
+			skb = netdev_alloc_skb_ip_align(priv->dev,
+							priv->dma_buf_sz);
 			if (unlikely(skb == NULL))
 				break;
 

@@ -703,7 +704,7 @@ static int xgmac_dma_desc_rings_init(struct net_device *dev)
 	/* Set the Buffer size according to the MTU;
 	 * indeed, in case of jumbo we need to bump-up the buffer sizes.
 	 */
-	bfsize = ALIGN(dev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN + 64,
+	bfsize = ALIGN(dev->mtu + ETH_HLEN + ETH_FCS_LEN,
 		       64);
 
 	netdev_dbg(priv->dev, "mtu [%d] bfsize [%d]\n", dev->mtu, bfsize);

On Thu, 2012-10-11 at 13:54 +0200, Eric Dumazet wrote:

Of course, if you use a forwarding setup, and the tx driver is not SG
capable, performance will be bad (You have to copy the data into a
single skb (linearize the skb)) 

But in 2012, having to use hardware without SG for a router sounds a bad
joke (if cpu speed is _also_ too low)

Hey I cannot go back in time, when that hardware was built in 2004 (mips
@250Mhz), it was considered good, and we did manufacture a lot of it, so
it's still maintained.

People run recent kernels on older hardware because they are *encouraged
to do so*.

I fought inside my company to be good kernel citizen, not using
proprietary BSP, rewrite & mainline the drivers, because that was the
community promise: mainline it, we will support it for you, you will get
the latest kernel features for free.


That worked, but with some drawbacks:

 - kernel footprint grew that much (we started from 2.4) that it does
not fit in device flash anymore

 - performance took a hit each time we upgrade, mostly because of cache
footprint growth.

 - as kernel footprint grew, available RAM for conntrack & route cache
entries was smaller each time


But I had to stop upgrading after 2.6.20. Everything below is not
anybody's fault. Bloat is unavoidable for software project that big.

I'm perfectly ok with that, but I don't want to be ridiculed for running
mainline kernel on old hardware.

Adding get_unaligned() everywhere in linux network stacks is not an
option.

We actually want to be able to read the code and fix the bugs, not only
run it on a cheap low end router.

That was not a request, I just needed a clarification. 

Documentation/unaligned-memory-access.txt does not say it's a big no-no,
it says you can give unaligned pointers to the networking stack if you
arch can do unaligned access (with an "efficiency" notion).

MIPS and ARM have a software handler for this, and performance wise in
my case it's better to take the faults, a driver writer may think a
benchmark will dictate what to do.

-- 
Maxime

On Thu, 2012-10-11 at 14:51 +0200, Maxime Bizon wrote:

Hey I cannot go back in time, when that hardware was built in 2004 (mips
@250Mhz), it was considered good, and we did manufacture a lot of it, so
it's still maintained.

People run recent kernels on older hardware because they are *encouraged
to do so*.

I fought inside my company to be good kernel citizen, not using
proprietary BSP, rewrite & mainline the drivers, because that was the
community promise: mainline it, we will support it for you, you will get
the latest kernel features for free.


That worked, but with some drawbacks:

 - kernel footprint grew that much (we started from 2.4) that it does
not fit in device flash anymore

 - performance took a hit each time we upgrade, mostly because of cache
footprint growth.

 - as kernel footprint grew, available RAM for conntrack & route cache
entries was smaller each time


But I had to stop upgrading after 2.6.20. Everything below is not
anybody's fault. Bloat is unavoidable for software project that big.

I'm perfectly ok with that, but I don't want to be ridiculed for running
mainline kernel on old hardware.

Hmm, I am sorry if you felt that, it was not my intent.

quoted

Adding get_unaligned() everywhere in linux network stacks is not an
option.

We actually want to be able to read the code and fix the bugs, not only
run it on a cheap low end router.

That was not a request, I just needed a clarification. 

Documentation/unaligned-memory-access.txt does not say it's a big no-no,
it says you can give unaligned pointers to the networking stack if you
arch can do unaligned access (with an "efficiency" notion).

MIPS and ARM have a software handler for this, and performance wise in
my case it's better to take the faults, a driver writer may think a
benchmark will dictate what to do.

Sure, but all this discussion started because one arch apparently did
not like these mis alignments, and some people complained that network
guys would not add _needed_ get_unaligned_xxx() wrappers ...

So far, linux is 20 years old, I dont think we are going to add wrappers
right now. Machines that could have cared are dying anyway.

Please note we still support NET_IP_ALIGN, even if its 0 on x86.

On 10/11/2012 07:40 AM, Eric Dumazet wrote:

On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:

quoted

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

This in fact does work and eliminates the unaligned traps. However, not
all h/w can do IP aligned DMA (i.MX FEC for example), so I still think
this is a questionable optimization by the compiler. We're saving 1 load
instruction here for data that is likely already in the cache. It may be
legal per the ABI, but the downside of this optimization is much greater
than the upside.

Thanks Arnd

It seems an accident, since driver doesnt check skb->data alignment at
all (this can change with SLAB debug on/off)

It also incorrectly adds 64 bytes to bfsize, there is no need for this.

I'm pretty sure this was needed as the h/w writes out full bursts of
data, but I'll go back and check.

Rob

On Thursday 11 October 2012, Rob Herring wrote:

On 10/11/2012 07:40 AM, Eric Dumazet wrote:

quoted

On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:

quoted

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

This in fact does work and eliminates the unaligned traps. However, not
all h/w can do IP aligned DMA (i.MX FEC for example), so I still think
this is a questionable optimization by the compiler. We're saving 1 load
instruction here for data that is likely already in the cache. It may be
legal per the ABI, but the downside of this optimization is much greater
than the upside.

What about the other approach that Eric suggested for such hardware
in http://www.spinics.net/lists/arm-kernel/msg200206.html ?

	Arnd

On Thu, 2012-10-11 at 08:20 -0500, Rob Herring wrote:

On 10/11/2012 07:40 AM, Eric Dumazet wrote:

quoted

On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:

quoted

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

This in fact does work and eliminates the unaligned traps. However, not
all h/w can do IP aligned DMA (i.MX FEC for example), so I still think
this is a questionable optimization by the compiler. We're saving 1 load
instruction here for data that is likely already in the cache. It may be
legal per the ABI, but the downside of this optimization is much greater
than the upside.

Compiler is asked to perform a 32bit load, it does it.

There is no questionable optimization here. Really.
Please stop pretending this, this makes no sense.

As I said, if some h/w cannot do IP aligned DMA, driver can use a
workaround, or a plain memmove() (some drivers seems to do this to work
around this h/w limitation, just grep for memmove() in drivers/net)

quoted

Thanks Arnd

It seems an accident, since driver doesnt check skb->data alignment at
all (this can change with SLAB debug on/off)

It also incorrectly adds 64 bytes to bfsize, there is no need for this.

I'm pretty sure this was needed as the h/w writes out full bursts of
data, but I'll go back and check.

Maybe the ALIGN() was needed then. But the 64 + NE_IP_ALIGN sounds like
the head room that we allocate/reserve in netdev_alloc_skb_ip_align()

So you allocate this extra room twice.

Thanks

On 10/11/2012 08:47 AM, Eric Dumazet wrote:

On Thu, 2012-10-11 at 08:20 -0500, Rob Herring wrote:

quoted

On 10/11/2012 07:40 AM, Eric Dumazet wrote:

quoted

On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:

quoted

Rob Herring as the original reporter has dropped off the Cc list, adding
him back.

I assume that the calxeda xgmac driver is the culprit then. It uses
netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
xgmac_rx_refill but it is not clear whether it does so intentionally
or by accident.

This in fact does work and eliminates the unaligned traps. However, not
all h/w can do IP aligned DMA (i.MX FEC for example), so I still think
this is a questionable optimization by the compiler. We're saving 1 load
instruction here for data that is likely already in the cache. It may be
legal per the ABI, but the downside of this optimization is much greater
than the upside.

Compiler is asked to perform a 32bit load, it does it.

Not exactly. It is asked to to perform 2 32-bit loads which are combined
into a single ldm (load multiple) which cannot handle unaligned
accesses. Here's a simple example that does the same thing:

void test(char * buf)
{
	printf("%d, %d\n", *((unsigned int *)&buf[0]), *((unsigned int *)&buf[4]));
}

So I guess the only ABI legal unaligned access is in a packed struct.

There is no questionable optimization here. Really.
Please stop pretending this, this makes no sense.

I'm not the one calling the networking stack bad code.

I can fix my h/w, so I'll stop caring about this. Others can all get
bitten by this new behavior in gcc 4.7.

Rob

As I said, if some h/w cannot do IP aligned DMA, driver can use a
workaround, or a plain memmove() (some drivers seems to do this to work
around this h/w limitation, just grep for memmove() in drivers/net)

quoted

quoted

Thanks Arnd

It seems an accident, since driver doesnt check skb->data alignment at
all (this can change with SLAB debug on/off)

It also incorrectly adds 64 bytes to bfsize, there is no need for this.

I'm pretty sure this was needed as the h/w writes out full bursts of
data, but I'll go back and check.

Maybe the ALIGN() was needed then. But the 64 + NE_IP_ALIGN sounds like
the head room that we allocate/reserve in netdev_alloc_skb_ip_align()

So you allocate this extra room twice.

Thanks

Not exactly. It is asked to to perform 2 32-bit loads which are combined
into a single ldm (load multiple) which cannot handle unaligned
accesses. Here's a simple example that does the same thing:

void test(char * buf)
{
	printf("%d, %d\n", *((unsigned int *)&buf[0]), *((unsigned int *)&buf[4]));
}

Have you actually looked at what an ARM processor traditionally did
with misaligned memory reads?
While useful, it probably wasn't what was intended.

Actually, and IIRC, some very recent ARM cpus will do the 'expected'
thing for single-word loads from misaligned addesses.
However they almost certainly won't for ldm/stm.

The 'ldm' optimisation for adjacent memory loads is also dubious.
On at least some ARMs it is very slow (might only be strongarms).

So I guess the only ABI legal unaligned access is in a packed struct.

Correct. And you mustn't try casting the address, the compiler is
allowed to remember where it came from.
(This causes a lot of grief...)

If you are targeting the ARM cpu that can do misaligned transfers,
then gcc should generate single instructions for misaligned structure
members, and never do the 'ldm' optimisations.

But, the IP header is expected to be aligned.

	David

On Thu, 2012-10-11 at 10:23 -0500, Rob Herring wrote:

On 10/11/2012 08:47 AM, Eric Dumazet wrote:

quoted

Compiler is asked to perform a 32bit load, it does it.

Not exactly. It is asked to to perform 2 32-bit loads which are combined
into a single ldm (load multiple) which cannot handle unaligned
accesses. Here's a simple example that does the same thing:

Thats simply not true. You are severely mistaken.

ldm does a load of seeral 32bit words.

And the compiler would not use it if the alignment was not matching the
prereq (alignment >= 4)

void test(char * buf)
{
	printf("%d, %d\n", *((unsigned int *)&buf[0]), *((unsigned int *)&buf[4]));
}

But you completely miss the fact that network doesnt pass a "char *buf"
but a "be32 *buf". Your example is not relevant at all.

So the compiler is absolutely right, and network stack is _right_ too.

The prereq is that IP header are aligned to 4 bytes boundary.

Denying this fact is not going to help you

So I guess the only ABI legal unaligned access is in a packed struct.

quoted

There is no questionable optimization here. Really.
Please stop pretending this, this makes no sense.

I'm not the one calling the networking stack bad code.

Once you understand the issues, you can explain us where is the bad
code. But given you say "Bug is in compiler, and/or network stack, but
my driver is fine", its not very wise.

For the moment, the bug is in your driver.

I can fix my h/w, so I'll stop caring about this. Others can all get
bitten by this new behavior in gcc 4.7.

Again compiler is fine. How should we say that so that you stop your
rants ?

Stop trying to find an excuse, dont try to fool us, this is really
embarrassing. Just fix the driver, there is no shame to fix an error.

On 11 October 2012 11:32, Russell King - ARM Linux
[off-list ref] wrote:

On Thu, Oct 11, 2012 at 12:22:06PM +0200, Eric Dumazet wrote:

quoted

I took a look, and I dont see why/how gcc could use a ldm instruction

Doing so assumed the alignment of the structure was 8 bytes, but its
not.

Networking stack mandates that IP headers are aligned on 4 bytes
boundaries, not 8 bytes.

Err, no.  ldm is "load multiple" not "load double".  It loads multiple
32-bit registers, and its requirement for non-faulting behaviour is for
the pointer to be 4 byte aligned.  However, "load double" requires 8
byte alignment.

It got better with ARMv6 where LDRD/STRD only require 4 byte alignment
(the only 8 byte alignment is required by LDREXD/STREXD). But on ARMv5
LDRD/STRD 8 byte alignment is indeed required (otherwise
unpredictable).

-- 
Catalin