Alignment issues with freescale FEC driver

Hello all,

We're seeing alignment issues from the ethernet stack on an i.MX6UL board:

root at mx6ul:~# cat /proc/cpu/alignment
User: 0
System: 470010 (inet_gro_receive+0x104/0x278)

This seems to be related to the ip header alignment, and there was much discussion in mailing list threads [1] and [2].

In particular, Russell referred to a patch here, but I haven't been able to find it:
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html

Eric Dumazet also suggested a path toward fixing it, but I don't quite understand the suggestion:
http://www.spinics.net/lists/netdev/msg213166.htm

The immediate problem is addressed by just reading the id and frag_offs fields in the iphdr structure as shown in this patch:

commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
Author: Eric Nelson <[email protected]>
Date: Fri Sep 23 08:26:03 2016 -0700

net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit value

Change-Id Idc7122c22c13ca078be31907d30ab1c3148ba807

Signed-off-by Eric Nelson <[email protected]>

net/ipv4/af_inet.c +4 -3

index 0cc98b1..c17ef6e 100644
@@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
 sk_buff **head,
 unsigned int hlen;
 unsigned int off;
 unsigned int id;
+ unsigned int frag;
 int flush = 1;
 int proto;

@@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
 sk_buff **head,
 if (unlikely(ip_fast_csum((u8 *)iph, 5)))
 goto out_unlock;
 
- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
 ~IP_DF));
 

for (p = *head; p; p = p->next) {
struct iphdr *iph2;

Quoted 6 lines · Eric Nelson

Hello all,

We're seeing alignment issues from the ethernet stack on an i.MX6UL board:

Quoted 9 lines

- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & ~IP_DF));

Quoted 3 lines

for (p = *head; p; p = p->next) {
struct iphdr *iph2;

This solves nothing, because a few lines after you'll have yet another unaligned access :

((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {

So you might have one less problematic access, out of hundreds of them all over the places.

Really the problem is that whole stack depends on the assumption that IP headers are aligned on arches that care
(ie where NET_IP_ALIGN == 2)

If your build does have NET_IP_ALIGN = 2 and you get a fault here, it might be because of a buggy driver.

The other known case is some GRE encapsulations that break the assumption, and this is discussed somewhere else.

Thanks Eric,

Quoted 7 lines · Eric Dumazet

Quoted 6 lines · Eric Nelson

Hello all,

We're seeing alignment issues from the ethernet stack on an i.MX6UL board:

<snip>

Quoted 9 lines

Quoted 9 lines

- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & ~IP_DF));

Quoted 7 lines

Quoted 3 lines

for (p = *head; p; p = p->next) {
struct iphdr *iph2;

This solves nothing, because a few lines after you'll have yet another unaligned access :

Oddly, it does prevent the vast majority (90%+) of the alignment errors.

I believe this is because the compiler is generating an ldm instruction when the ntohl() call is used, but I'm stumped about why these aren't generating faults:

Quoted 6 lines

((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {

So you might have one less problematic access, out of hundreds of them all over the places.

Quoted 7 lines

Really the problem is that whole stack depends on the assumption that IP headers are aligned on arches that care
(ie where NET_IP_ALIGN == 2)

If your build does have NET_IP_ALIGN = 2 and you get a fault here, it might be because of a buggy driver.

NET_IP_ALIGN is set to 2.

Quoted 3 lines

The other known case is some GRE encapsulations that break the assumption, and this is discussed somewhere else.

I don't think that's the case.

# CONFIG_IPV6_GRE is not set

Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on a 4-byte boundary.

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

Quoted 2 lines

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts two padding bits on transmit. ENETx_RACC has the same.

What about your hardware?

Quoted 7 lines · Andrew Lunn

Quoted 2 lines

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts two padding bits on transmit. ENETx_RACC has the same.

What about your hardware?

The iMX6 FEC also has that ability - as part of my FEC patch stack from ages ago, I implemented support for it.

"net:fec: implement almost zero-copy receive path"

in my public fec-testing branch.

That patch stack is sadly now totally dead and I've no interest in reviving it myself. There was some interest from others in taking my patch stack over, but that went quiet.

Quoted 2 lines

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts two padding bits on transmit. ENETx_RACC has the same.

What about your hardware?

From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:

From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:

"RX FIFO Shift-16

When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary."

Same for the i.MX6UL.

I'm not sure what it will take to use this, but it seems to be exactly what we're looking for.

To: eric at nelint.com
Cc: andrew at lunn.ch; edumazet at google.com; Andy Duan <[email protected]>; otavio at ossystems.com.br;
netdev at vger.kernel.org; troy.kisky at boundarydevices.com; rmk+kernel at arm.linux.org.uk; cjb.sw.nospam at gmail.com; linux-arm- kernel at lists.infradead.org
Subject: Re: Alignment issues with freescale FEC driver

From: Eric Nelson <[email protected]>
Date: Fri, 23 Sep 2016 11:35:17 -0700

Quoted 8 lines

From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:

"RX FIFO Shift-16

When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary."

Quoted 4 lines

Same for the i.MX6UL.

I'm not sure what it will take to use this, but it seems to be exactly what we're looking for.

+1

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

...

drivers/net/ethernet/freescale/fec_main.c fec_restart(struct net_device *ndev) · line 951

Quoted 2 lines

drivers/net/ethernet/freescale/fec_main.c +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

If probably ought to be predicated on NET_IP_ALIGN as well.

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

drivers/net/ethernet/freescale/fec_main.c fec_restart(struct net_device *ndev) · line 951

Quoted 2 lines

drivers/net/ethernet/freescale/fec_main.c +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

drivers/net/ethernet/freescale/fec_main.c fec_restart(struct net_device *ndev) · line 951

Quoted 2 lines

drivers/net/ethernet/freescale/fec_main.c +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that the data is dma'd to offset -2 in the skb and then ensure that the end of frame is set correctly.

Thanks David,

Quoted 2 lines · David Laight

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 8 lines · Eric Nelson

Thanks David,

Quoted 2 lines · David Laight

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

That's wrong. FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of the packet, not in the middle of the packet:

7 RX FIFO Shift-16
SHIFT16
When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary.

NOTE: This function only affects the FIFO storage and has no
influence on the statistics, which use the actual length of the frame received.

0 Disabled.
1 Instructs the MAC to write two additional bytes in front
of each frame received into the RX FIFO.

*in front* of the frame - that's before the Ethernet header. Not between the ethernet and IP headers.

Sent: 28 September 2016 18:15

Quoted 29 lines · David Laight

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

drivers/net/ethernet/freescale/fec_main.c fec_restart(struct net_device *ndev) · line 951

Quoted 2 lines

drivers/net/ethernet/freescale/fec_main.c +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 4 lines

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them.

Quoted 3 lines

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

Right, but I think the max is already high enough that this isn't a problem.

Quoted 4 lines

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

This is well within the 2048-byte allocation, even with optional headers for VLAN etc.

Quoted 4 lines

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Quoted 4 lines

OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that the data is dma'd to offset -2 in the skb and then ensure that the end of frame is set correctly.

That's what the RACC SHIFT16 bit does.

The FEC hardware isn't capable of DMA'ing to an un-aligned address. On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.

On other (PPC?) architectures, it requires 32-bit alignment. This is handled by the rx_align field.

From: Eric Nelson

Quoted 42 lines

Sent: 28 September 2016 18:15

Quoted 29 lines · David Laight

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

drivers/net/ethernet/freescale/fec_main.c fec_restart(struct net_device *ndev) · line 951

Quoted 2 lines

drivers/net/ethernet/freescale/fec_main.c +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 8 lines

Quoted 4 lines

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them.

Ok, I didn't see that call being added by this patch.

Quoted 15 lines

Quoted 3 lines

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

Right, but I think the max is already high enough that this isn't a problem.

Quoted 4 lines

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

Quoted 2 lines

This is well within the 2048-byte allocation, even with optional headers for VLAN etc.

Hmm...

That (probably) means all the skb the driver allocates are actually 4k. It would be much better to reduce the size so that the entire skb (with packet buffer) is less than 2k.

Quoted 5 lines

Quoted 4 lines

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Even though it is always currently set is isn't really ideal to have a driver that breaks if it isn't set.
This could easily happen at some point in the future if the ethernet logic is put with a different cpu.

From Documentation/unaligned-memory-access.txt, I take it that this

Quoted 6 lines

Quoted 4 lines

OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that the data is dma'd to offset -2 in the skb and then ensure that the end of frame is set correctly.

That's what the RACC SHIFT16 bit does.

No, that causes the ethernet controller to add 2 bytes to the frame. You then need to change the dma target address to match.

Otherwise if a new version of the silicon stops ignoring the low address with the frame will be misaligned in the buffer.

The receive frame length will also (probably) be 2 larger than the actual frame - so you need to set the end point correctly as well. IP will probably ignore the 2 bytes of pad I think you are generating.

Quoted 5 lines

The FEC hardware isn't capable of DMA'ing to an un-aligned address. On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.

On other (PPC?) architectures, it requires 32-bit alignment. This is handled by the rx_align field.

That isn't entirely relevant.

If the kernel is being built with NET_IP_ALIGN set to 0 you should align the destination mac address on a 4n boundary
(Or rather the skb are likely to be allocated that way).

If it causes misaligned memory reads later on that is a different problem.

The MAC driver has aligned the frames as it was told to.

David

Sent: 30 September 2016 14:27
Thanks for the feedback David,

Quoted 72 lines · David Laight

From: Eric Nelson

Quoted 42 lines

Sent: 28 September 2016 18:15

Quoted 29 lines · David Laight

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 8 lines

Quoted 4 lines

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them.

Ok, I didn't see that call being added by this patch.

Quoted 17 lines

Quoted 3 lines

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

Right, but I think the max is already high enough that this isn't a problem.

Quoted 4 lines

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

This is well within the 2048-byte allocation, even with optional headers for VLAN etc.

Quoted 6 lines

Hmm...

That (probably) means all the skb the driver allocates are actually 4k. It would be much better to reduce the size so that the entire skb (with packet buffer) is less than 2k.

That seems worthwhile, but un-related to this patch.

It appears to me that the received packets could be allocated as

PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN

(+2 if FEC_RACC_SHIFT16 is used)

Quoted 11 lines

Quoted 5 lines

Quoted 4 lines

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Even though it is always currently set is isn't really ideal to have a driver that breaks if it isn't set.
This could easily happen at some point in the future if the ethernet logic is put with a different cpu.

After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and how it should be used.

From Documentation/unaligned-memory-access.txt, I take it that this should be configured on a per-architecture basis, and it seems to be set to zero on both PPC and x86.

I wonder if this is proper though. It seems that its' use might depend on the I/O subsystem(s) in use as much as the architecture.

From: Eric Nelson

Quoted 76 lines

Sent: 30 September 2016 14:27
Thanks for the feedback David,

Quoted 72 lines · David Laight

From: Eric Nelson

Quoted 42 lines

Sent: 28 September 2016 18:15

Quoted 29 lines · David Laight

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 8 lines

Quoted 4 lines

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them.

Ok, I didn't see that call being added by this patch.

Quoted 17 lines

Quoted 3 lines

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

Right, but I think the max is already high enough that this isn't a problem.

Quoted 4 lines

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

This is well within the 2048-byte allocation, even with optional headers for VLAN etc.

Quoted 8 lines

Quoted 6 lines

Hmm...

That (probably) means all the skb the driver allocates are actually 4k. It would be much better to reduce the size so that the entire skb (with packet buffer) is less than 2k.

That seems worthwhile, but un-related to this patch.

Indeed.

Quoted 2 lines

It appears to me that the received packets could be allocated as

Quoted 3 lines

PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN

(+2 if FEC_RACC_SHIFT16 is used)

No.
The packet buffers need to be allocated NET_IP_ALIGN + PKT_MAXBUF_SIZE byte long and (I assume) aligned on a fep->rx_align byte boundary.

If NET_IP_ALIGN is set (to 2) then FEC_RACC_SHIFT16 must also me set so that the ethernet frame itself is 4n+2 aligned.

Quoted 19 lines

Quoted 11 lines

Quoted 5 lines

Quoted 4 lines

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Even though it is always currently set is isn't really ideal to have a driver that breaks if it isn't set.
This could easily happen at some point in the future if the ethernet logic is put with a different cpu.

After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and how it should be used.

From Documentation/unaligned-memory-access.txt, I take it that this should be configured on a per-architecture basis, and it seems to be set to zero on both PPC and x86.

Quoted 2 lines

I wonder if this is proper though. It seems that its' use might depend on the I/O subsystem(s) in use as much as the architecture.

...

If the cpu cannot do misaligned memory cycles then NET_IP_ALIGN must be 2 and all receive frames must be aligned like that.

If the cpu can do misaligned memory cycles then the alignment of receive ethernet frames doesn't matter that much.
NET_IP_ALIGN is likely to be set to zero because the cost of the cpu doing misaligned transfers it likely to be a lot less than that of un-optimised dma accesses to misaligned memory [1] [2].

If NET_IP_ALIGN is zero then I believe that ethernet drivers are allowed to build skb that have the frame on a 4n+2 alignment. This is probably sensible if the hardware can write the two bytes. (DM might correct me there.)

David

[1] The original sparc sbus 'DMA' part did multiple 16bit transfers instead of a burst of 32bit transfers. This meant the buffer had to be misaligned and a software copy done to align the frames. Fixed in the DMA+ part.

[2] PCIe writes are likely to be much faster if they contain entire cache lines of data.

On ARM, the CPU can't handle misaligned memory cycles without taking an alignment fault and NET_IP_ALIGN is set to 2.

On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y but I find it hard to believe that taking alignment faults is more efficient than adding two bytes to the start of the frame.

Sent: 01 October 2016 20:52

Quoted 2 lines · Eric Nelson

On ARM, the CPU can't handle misaligned memory cycles without taking an alignment fault and NET_IP_ALIGN is set to 2.

Let's get this right... With Linux on MMU parts:

On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding store instructions are handled in hardware without any exception being raised.

On pre-ARMv6, such instructions raise an alignment exception, and we fix up the load/store manually.

Where things behave the same is with the LDM (load multiple) and STM (store multiple) instructions. Hardware does not fix these up if they are unaligned: it is expected that the base address will always be aligned to a 32-bit word.

For some reason, the compiler guys have decided it's okay to use these instructions as an optimisation, and I see no way to disable this behaviour.

The issue with the networking layer is that it passes around structure pointers which may not be "naturally aligned" - technically it goes against the C standard specs. However, you'll find it hard to argue against this, so we have to accept that the networking people expect it to work.

The optimisation that the C compiler uses (using LDM to access multiple 32-bit consecutive words) is legal and efficient when the structure pointers are aligned as it expects, but that all breaks if the pointer is not so aligned. So, raising it as a bug against the C compiler isn't going to work either.

What may work is to raise a feature request with compiler people to have a mechanism to disable the LDM/STM optimisation for code where we know that pointers may not be naturally aligned.

Quoted 2 lines · Eric Nelson

On ARM, the CPU can't handle misaligned memory cycles without taking an alignment fault and NET_IP_ALIGN is set to 2.

Let's get this right... With Linux on MMU parts:

On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding store instructions are handled in hardware without any exception being raised.

On pre-ARMv6, such instructions raise an alignment exception, and we fix up the load/store manually.

Where things behave the same is with the LDM (load multiple) and STM (store multiple) instructions. Hardware does not fix these up if they are unaligned: it is expected that the base address will always be aligned to a 32-bit word.

For some reason, the compiler guys have decided it's okay to use these instructions as an optimisation, and I see no way to disable this behaviour.

Quoted 3 lines

On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y but I find it hard to believe that taking alignment faults is more efficient than adding two bytes to the start of the frame.

... see above, hence why CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set. It's only when the compiler decides to do silly things that things go wrong. However, net code does not care about that configuration setting, so it's irrelevant to this discussion.

The issue with the networking layer is that it passes around structure pointers which may not be "naturally aligned" - technically it goes against the C standard specs. However, you'll find it hard to argue against this, so we have to accept that the networking people expect it to work.

The optimisation that the C compiler uses (using LDM to access multiple 32-bit consecutive words) is legal and efficient when the structure pointers are aligned as it expects, but that all breaks if the pointer is not so aligned. So, raising it as a bug against the C compiler isn't going to work either.

What may work is to raise a feature request with compiler people to have a mechanism to disable the LDM/STM optimisation for code where we know that pointers may not be naturally aligned.

To: David Laight <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; Andy Duan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] net: fec: align IP header in hardware

Thanks for the feedback David,

Quoted 48 lines · David Laight

From: Eric Nelson

Quoted 47 lines

Sent: 28 September 2016 18:15

Quoted 29 lines · David Laight

From: Eric Nelson

Quoted 28 lines

Sent: 26 September 2016 19:40
Hi David,

Quoted 6 lines · David Laight

From: Eric Nelson

Quoted 5 lines

Sent: 24 September 2016 15:42
The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer.

Quoted 8 lines

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length.

In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

If probably ought to be predicated on NET_IP_ALIGN as well.

Can you elaborate?

From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame.

That's right. Two bytes of junk between the MAC header and the IP header.

Quoted 4 lines

In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer.

Quoted 20 lines

Quoted 3 lines

As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them.

Ok, I didn't see that call being added by this patch.

Quoted 14 lines

Quoted 3 lines

The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less.

Right, but I think the max is already high enough that this isn't a problem.

Quoted 4 lines

If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2.

As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE

(1522).

Quoted 10 lines

Quoted 3 lines

This is well within the 2048-byte allocation, even with optional headers for VLAN etc.

Hmm...

That (probably) means all the skb the driver allocates are actually 4k. It would be much better to reduce the size so that the entire skb (with packet buffer) is less than 2k.

That seems worthwhile, but un-related to this patch.

It appears to me that the received packets could be allocated as

PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN

(+2 if FEC_RACC_SHIFT16 is used)

Quoted 11 lines

Quoted 5 lines

Quoted 4 lines

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'.

NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Even though it is always currently set is isn't really ideal to have a driver that breaks if it isn't set.
This could easily happen at some point in the future if the ethernet logic is put with a different cpu.

After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and how it should be used.

From Documentation/unaligned-memory-access.txt, I take it that this should be configured on a per-architecture basis, and it seems to be set to zero on both PPC and x86.

I wonder if this is proper though. It seems that its' use might depend on the I/O subsystem(s) in use as much as the architecture.

For example, it might be desirable to have a different value for a PCIe interface than for an integrated MAC like the FEC.

Looking at the example of the 3c59x driver, I see a pattern of an allocation that adds NET_IP_ALIGN followed by an skb->reserve() of NET_IP_ALIGN before determining the target address to end up with allocation with 4n+2 alignment.

This seems somewhat equivalent to this patch, except that we're using the allocated address as the target and using skb_pull_inline afterwards.

Andy, is the FEC used on any PPC SOCs?

If so, then this patch may cause a DMA of 2 extra bytes per frame unecessarily although the driver doesn't special-case the allocation to align the IP header, so this is still probably preferred.

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that the data is dma'd to offset -2 in the skb and then ensure that the end of frame is set correctly.

That's what the RACC SHIFT16 bit does.

No, that causes the ethernet controller to add 2 bytes to the frame. You then need to change the dma target address to match.

Or use skb_pull_inline to ignore the two bytes.

Quoted 3 lines

Otherwise if a new version of the silicon stops ignoring the low address with the frame will be misaligned in the buffer.

I'm not sure I understand this.

Quoted 4 lines

The receive frame length will also (probably) be 2 larger than the actual frame - so you need to set the end point correctly as well. IP will probably ignore the 2 bytes of pad I think you are generating.

The received frame length **is** 2 bytes longer, but these are eaten by skb_pull_inline().

Quoted 11 lines

Quoted 5 lines

The FEC hardware isn't capable of DMA'ing to an un-aligned address. On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.

On other (PPC?) architectures, it requires 32-bit alignment. This is handled by the rx_align field.

That isn't entirely relevant.

If the kernel is being built with NET_IP_ALIGN set to 0 you should align the destination mac address on a 4n boundary (Or rather the skb are likely to be allocated that way).

They're not currently allocated that way. The routine fec_enet_alloc_rxq_buffers forces the allocations to 32 or 128-bit alignment through the routine fec_enet_new_rxbdp().

Quoted 1 line

If it causes misaligned memory reads later on that is a different problem.

That's the problem this patch is designed to address. Without this patch, the IP header is always mis-aligned.

Quoted 5 lines

The MAC driver has aligned the frames as it was told to.

David

Regards,

Eric

To: [email protected]
Cc: [email protected]; [email protected]; Andy Duan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Eric Nelson <[email protected]>
Subject: [PATCH 0/3] net: fec: updates to align IP header

This patch series is the outcome of investigation into very high numbers of alignment faults on kernel 4.1.33 from the linux-fslc
tree:

    https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx

The first two patches remove support for the receive accelerator (RACC) from the i.MX25 and i.MX27 SoCs which don't support the function.

The third patch enables hardware alignment of the ethernet packet payload (and especially the IP header) to prevent alignment faults in the IP stack.

Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on the order of 70k alignment faults during a 100MiB transfer using wget.

Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a much more modest improvement from 10's of faults, and it's not clear why that's the case.

Eric Nelson (3):
net: fec: remove QUIRK_HAS_RACC from i.mx25
net: fec: remove QUIRK_HAS_RACC from i.mx27
net: fec: align IP header in hardware

 drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

--
2.7.4

From: Eric Nelson <[email protected]> Sent: Saturday, September 24, 2016 10:42 PM

Quoted 31 lines

To: [email protected]
Cc: [email protected]; [email protected]; Andy Duan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Eric Nelson <[email protected]>
Subject: [PATCH 0/3] net: fec: updates to align IP header

This patch series is the outcome of investigation into very high numbers of alignment faults on kernel 4.1.33 from the linux-fslc
tree:

    https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx

The first two patches remove support for the receive accelerator (RACC) from the i.MX25 and i.MX27 SoCs which don't support the function.

The third patch enables hardware alignment of the ethernet packet payload (and especially the IP header) to prevent alignment faults in the IP stack.

Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on the order of 70k alignment faults during a 100MiB transfer using wget.

Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a much more modest improvement from 10's of faults, and it's not clear why that's the case.

Eric Nelson (3):
net: fec: remove QUIRK_HAS_RACC from i.mx25
net: fec: remove QUIRK_HAS_RACC from i.mx27
net: fec: align IP header in hardware

Quoted 5 lines

 drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

--
2.7.4

I will investigate the diff between 4.1 and 4.8. Thanks.

Acked-by Fugang Duan <[email protected]>

This patch series is the outcome of investigation into very high numbers of alignment faults on kernel 4.1.33 from the linux-fslc tree:

    https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx

The first two patches remove support for the receive accelerator (RACC) from the i.MX25 and i.MX27 SoCs which don't support the function.

The third patch enables hardware alignment of the ethernet packet payload (and especially the IP header) to prevent alignment faults in the IP stack.

Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on the order of 70k alignment faults during a 100MiB transfer using wget.

Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a much more modest improvement from 10's of faults, and it's not clear why that's the case.

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

From: Eric Nelson <[email protected]>
Date: Fri, 23 Sep 2016 10:33:29 -0700

Quoted 2 lines

Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right?

I wish hardware designers didn't do this.

There is no conflict between DMA alignment and properly offseting the packet data by two bytes.

All hardware designers have to do is allow 2 padding bytes to be emitted by the chip before the actual packet data.

Then the longword or whatever DMA transfer alignment is met whilst still giving the necessary flexibility for where the packet data lands.

Oddly, it does prevent the vast majority (90%+) of the alignment errors.

I believe this is because the compiler is generating an ldm instruction when the ntohl() call is used, but I'm stumped about why these aren't generating faults:

I don't think that's the case.

# CONFIG_IPV6_GRE is not set

Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on a 4-byte boundary.

Does the ldm instruction require 8-byte alignment?

There's definitely a compiler-version dependency involved here, since using gcc 4.9 also reduced the number of faults dramatically.

Quoted 5 lines · Eric Nelson

Oddly, it does prevent the vast majority (90%+) of the alignment errors.

I believe this is because the compiler is generating an ldm instruction when the ntohl() call is used, but I'm stumped about why these aren't generating faults:

ldm generates alignment faults when the address is not aligned to a 32-bit boundary. ldr on ARMv6+ does not.

Quoted 7 lines

I don't think that's the case.

# CONFIG_IPV6_GRE is not set

Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on a 4-byte boundary.

Quoted 4 lines

Does the ldm instruction require 8-byte alignment?

There's definitely a compiler-version dependency involved here, since using gcc 4.9 also reduced the number of faults dramatically.

Well, I don't think it's that gcc related:

User:           0
System:         312855 (ip6_route_input+0x6c/0x1e0)
Skipped:        0
Half:           0
Word:           0
DWord:          2
Multi:          312853

c06d8998 <ip6_route_input>:

c06d89ac:       e1a04000        mov     r4, r0
c06d89b0:       e1d489b4        ldrh    r8, [r4, #148]  ; 0x94
c06d89b8:       e594a0a0        ldr     sl, [r4, #160]  ; 0xa0
c06d89cc:       e08ac008        add     ip, sl, r8
c06d89d4:       e28c3018        add     r3, ip, #24
c06d89dc:       e28c7008        add     r7, ip, #8
c06d89e4:       e893000f        ldm     r3, {r0, r1, r2, r3}
c06d89ec:       e24be044        sub     lr, fp, #68     ; 0x44
c06d89f4:       e24b5054        sub     r5, fp, #84     ; 0x54
c06d89fc:       e885000f        stm     r5, {r0, r1, r2, r3}
c06d8a04:       e897000f        ldm     r7, {r0, r1, r2, r3}
c06d8a10:       e88e000f        stm     lr, {r0, r1, r2, r3}

This is from:

        struct flowi6 fl6 = {
                .flowi6_iif = l3mdev_fib_oif(skb->dev),
                .daddr = iph->daddr,
                .saddr = iph->saddr,
                .flowlabel = ip6_flowinfo(iph),
                .flowi6_mark = skb->mark,
                .flowi6_proto = iph->nexthdr,
        };

specifically, I suspect, the saddr and daddr initialisations.

There's not much to get away from this - the FEC on iMX requires a 16-byte alignment for DMA addresses, which violates the network stack's requirement for the ethernet packet to be received with a two byte offset. So the IP header (and IPv6 headers) will always be mis-aligned in memory, which leads to a huge number of alignment faults.

There's not much getting away from this - the problem is not in the networking stack, but the FEC hardware/network driver. See:

        struct  fec_enet_private *fep = netdev_priv(ndev);
        int off;

        off = ((unsigned long)skb->data) & fep->rx_align;
        if (off)
                skb_reserve(skb, fep->rx_align + 1 - off);

        bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));

in fec_enet_new_rxbdp().

So the question is: should we just live with this and acknowledge a performance penalty of bad alignment or do something about it?

I'm not sure the cost (or the details) of Eric's proposed fix of allocating and copying the header to another skb.

Quoted 2 lines · Eric Nelson

So the question is: should we just live with this and acknowledge a performance penalty of bad alignment or do something about it?

Well, I've no interest in trying to do anything with the FEC driver anymore, as I'll just generate another big patch stack which won't make it into the kernel in a timely fashion - my last attempt at improving the FEC driver was dogged with conflicting changes and I gave up with it in the end. I ended up spending a full cycle rebasing, re-testing, and re-evaluating their performance only to find that I'd missed the merge window again, and other conflicting changes got merged which meant that I had to start from the beginning again.

Quoted 2 lines

I'm not sure the cost (or the details) of Eric's proposed fix of allocating and copying the header to another skb.

I had a quick look at this, and although Eric's idea may be a good idea, it doesn't contain enough details for me to be able to implement it - eg, I've no idea how to attach the 128-byte skb to the beginning of a previously allocated skb containing the rest of the packet. I've just looked through linux/skbuff.h and I can't see anything that takes two sk_buff's that would do the job.

However, I don't think that's necessary in this case, because the iMX6 FEC supports the 16-bit alignment of the packet, if only it was enabled in hardware and the driver caters for it.

Quoted 2 lines · Eric Nelson

So the question is: should we just live with this and acknowledge a performance penalty of bad alignment or do something about it?

Well, I've no interest in trying to do anything with the FEC driver anymore, as I'll just generate another big patch stack which won't make it into the kernel in a timely fashion - my last attempt at improving the FEC driver was dogged with conflicting changes and I gave up with it in the end. I ended up spending a full cycle rebasing, re-testing, and re-evaluating their performance only to find that I'd missed the merge window again, and other conflicting changes got merged which meant that I had to start from the beginning again.