Kernel unaligned access at __skb_flow_dissect

There is an unaligned access at __skb_flow_dissect when it calls ip6_flowlabel() with the call stack

  __skb_flow_dissect+0x2a8/0x87c
  eth_get_headlen+0x5c/0xaxa4
  ixgbe_clean_rx_irq+0x5cc/0xb20 [ixgbe]
  ixgbe_poll+0x5a4/0x760 [ixgbe]
  net_rx_action+0x13c/0x354
    :

Essentially, ixgbe_pull_tail() is trying to figure out how much to pull, in order to have an aligned buffer:

        pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);

        /* align pull length to size of long to optimize memcpy performance */
        skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));

and seems like the unaligned access is unavoidable here (see comments in __skb_get_poff, for example).

This (below) is what I came up with, to get rid of the unaligned access errors on sparc, Is there a better solution? (Not having access to struct ip6_hdr in this file made put_unaligned usage non-obvious)

net/core/flow_dissector.c +11 -0

@@ -102,6 +102,17 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int 
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static inline __be32 ip6_flowlabel_align(const u8 *hdr)
+{
+       union {
+               __u8 w[4];
+               __u32 flow;
+       } ip6_flow;
+
+       memcpy(ip6_flow.w, hdr, 4);
+       return (ip6_flow.flow & IPV6_FLOWLABEL_MASK);
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specifie

Why ipv6 stack itself does not trigger the issue ?

Maybe the driver itself does not properly align IP headers on sparc ?

Make sure NET_IP_ALIGN is 2 on your build.

Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

I would try following ixgbe fix (sorry, totally untested, but you get the idea...)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0

index c4003a88bbf6..7ba64ed463a6 100644

From: Eric Dumazet <[email protected]>
Date: Fri, 29 Jan 2016 10:40:10 -0800

Quoted 14 lines

I would try following ixgbe fix (sorry, totally untested, but you get the idea...)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1 -1

index c4003a88bbf6..7ba64ed463a6 100644
@@ -1525,7 +1525,7 @@ static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring,
 
 	bi->dma = dma;
 	bi->page = page;
-	bi->page_offset = 0;
+	bi->page_offset = NET_IP_ALIGN;
 

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring, · line 1531

Quoted 2 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0

 	return true;
 }

Hmmm, wouldn't this waste 2 bytes on all pages? Or just the head one with the headers?

Quoted 49 lines · Sowmini Varadhan

There is an unaligned access at __skb_flow_dissect when it calls ip6_flowlabel() with the call stack

  __skb_flow_dissect+0x2a8/0x87c
  eth_get_headlen+0x5c/0xaxa4
  ixgbe_clean_rx_irq+0x5cc/0xb20 [ixgbe]
  ixgbe_poll+0x5a4/0x760 [ixgbe]
  net_rx_action+0x13c/0x354
    :

Essentially, ixgbe_pull_tail() is trying to figure out how much to pull, in order to have an aligned buffer:

        pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);

        /* align pull length to size of long to optimize memcpy performance */
        skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));

and seems like the unaligned access is unavoidable here (see comments in __skb_get_poff, for example).

This (below) is what I came up with, to get rid of the unaligned access errors on sparc, Is there a better solution? (Not having access to struct ip6_hdr in this file made put_unaligned usage non-obvious)

net/core/flow_dissector.c +11 -0

@@ -102,6 +102,17 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int 
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static inline __be32 ip6_flowlabel_align(const u8 *hdr)
+{
+       union {
+               __u8 w[4];
+               __u32 flow;
+       } ip6_flow;
+
+       memcpy(ip6_flow.w, hdr, 4);
+       return (ip6_flow.flow & IPV6_FLOWLABEL_MASK);
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specifie

net/core/flow_dissector.c ipv6: · line 247

Quoted 8 lines

net/core/flow_dissector.c +1 -1

@@ -230,7 +241,7 @@ ipv6:
                        key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
                }
 
-               flow_label = ip6_flowlabel(iph);
+               flow_label = ip6_flowlabel_align((const u8 *)iph);
                if (flow_label) {
                        if (dissector_uses_key(flow_dissector,
                                               FLOW_DISSECTOR_KEY_FLOW_LABEL)) {

Why ipv6 stack itself does not trigger the issue ?

Maybe the driver itself does not properly align IP headers on sparc ?

Make sure NET_IP_ALIGN is 2 on your build.

Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

Quoted 6 lines

Why ipv6 stack itself does not trigger the issue ?
Maybe the driver itself does not properly align IP headers on sparc ? Make sure NET_IP_ALIGN is 2 on your build.
Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

Hmmm.... it seems that flow dissector can "support L2 GRE", leading to unaligned accesses since a header is 14 bytes (not multiple of 4)

IP stacks in linux assume IP headers are always aligned to 4 bytes, so it means having a GRE header like this would align trap on some arches.

Quoted 61 lines · Eric Dumazet

Quoted 49 lines · Sowmini Varadhan

There is an unaligned access at __skb_flow_dissect when it calls ip6_flowlabel() with the call stack

  __skb_flow_dissect+0x2a8/0x87c
  eth_get_headlen+0x5c/0xaxa4
  ixgbe_clean_rx_irq+0x5cc/0xb20 [ixgbe]
  ixgbe_poll+0x5a4/0x760 [ixgbe]
  net_rx_action+0x13c/0x354
    :

Essentially, ixgbe_pull_tail() is trying to figure out how much to pull, in order to have an aligned buffer:

        pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);

        /* align pull length to size of long to optimize memcpy performance */
        skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));

and seems like the unaligned access is unavoidable here (see comments in __skb_get_poff, for example).

This (below) is what I came up with, to get rid of the unaligned access errors on sparc, Is there a better solution? (Not having access to struct ip6_hdr in this file made put_unaligned usage non-obvious)

net/core/flow_dissector.c +11 -0

@@ -102,6 +102,17 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static inline __be32 ip6_flowlabel_align(const u8 *hdr)
+{
+       union {
+               __u8 w[4];
+               __u32 flow;
+       } ip6_flow;
+
+       memcpy(ip6_flow.w, hdr, 4);
+       return (ip6_flow.flow & IPV6_FLOWLABEL_MASK);
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specifie

net/core/flow_dissector.c ipv6: · line 247

Quoted 8 lines

net/core/flow_dissector.c +1 -1

@@ -230,7 +241,7 @@ ipv6:
                        key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
                }
 
-               flow_label = ip6_flowlabel(iph);
+               flow_label = ip6_flowlabel_align((const u8 *)iph);
                if (flow_label) {
                        if (dissector_uses_key(flow_dissector,
                                               FLOW_DISSECTOR_KEY_FLOW_LABEL)) {

Why ipv6 stack itself does not trigger the issue ?

Quoted 7 lines

Maybe the driver itself does not properly align IP headers on sparc ?

Make sure NET_IP_ALIGN is 2 on your build.

Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

Hmmm.... it seems that flow dissector can "support L2 GRE", leading to unaligned accesses since a header is 14 bytes (not multiple of 4)

commit e1733de2243609073534cf56afb146a62af3c3d8

Author: Michael Dalton <[email protected]>
Date:   Mon Mar 11 06:52:28 2013 +0000

flow_dissector: support L2 GRE

Add support for L2 GRE tunnels, so that RPS can be more effective.

Michael, do we still need this ?

IP stacks in linux assume IP headers are always aligned to 4 bytes, so it means having a GRE header like this would align trap on some arches.

Doesn't every IP/VXLAN packet contains unaligned headers? Why don't those create alignment issues (like when stack looks at addresses)?

Quoted 2 lines · Tom Herbert

Doesn't every IP/VXLAN packet contains unaligned headers? Why don't those create alignment issues (like when stack looks at addresses)?

They do.

http://comments.gmane.org/gmane.linux.network/370672

some of it was fixed in https://lkml.org/lkml/2015/7/20/63.
There's still some NET_IP_ALIGN missing. IIRC, I was seeing this for mldv3, but the fix has to be done carefully, by someone who knows how to fully regression test it.

I dont know if other tunneling methods manage to get the NET_IP_ALIGN correct in every case.

Also, while sparc complains about unaligned access in most cases, some sins may pass under the radar, and other platforms dont even generate traps, so it's easy to not know that there's a problem, a lot of the time.

--Sowmini

Quoted 2 lines · Tom Herbert

Doesn't every IP/VXLAN packet contains unaligned headers? Why don't those create alignment issues (like when stack looks at addresses)?

They do.

http://comments.gmane.org/gmane.linux.network/370672

some of it was fixed in https://lkml.org/lkml/2015/7/20/63.
There's still some NET_IP_ALIGN missing. IIRC, I was seeing this for mldv3, but the fix has to be done carefully, by someone who knows how to fully regression test it.

I dont know if other tunneling methods manage to get the NET_IP_ALIGN correct in every case.

Also, while sparc complains about unaligned access in most cases, some sins may pass under the radar, and other platforms dont even generate traps, so it's easy to not know that there's a problem, a lot of the time.

--Sowmini

The sparc documentation is pretty clear
http://docs.oracle.com/cd/E19253-01/816-4854/hwovr-2/index.html, seems like unaligned accesses are not allowed in the architecture.

Quoted 3 lines · Tom Herbert

The sparc documentation is pretty clear
http://docs.oracle.com/cd/E19253-01/816-4854/hwovr-2/index.html, seems like unaligned accesses are not allowed in the architecture.

yes, but looks like you can paper over some of this with memcpy (as was happening with the saddr ref in skb_flow_dissect that puzzled me and Eric because it did not generate any traps).

But even within flow dissector, to be completely correct, we need to replace all 32-bit accesses with the mempy (flow_label, mpls label, keyid) and be vigilant about new ones coming in. For that matter, ..

Quoted 3 lines · Tom Herbert

But even within flow dissector, to be completely correct, we need to replace all 32-bit accesses with the mempy (flow_label, mpls label, keyid) and be vigilant about new ones coming in. For that matter, ..

well, one question that came to my mind when I was looking at this is:
why does eth_get_headlen try to compute the flow hash even before the driver has had a chance to pull things into an aligned buffer? Isn't that just risking even more unaligned accesses?

Seems like it might be just as well to pull up a fixed number of bytes (ixgbe want min of 60 bytes),

Hmm, thanks for pointing that out. It looks like this is called by a couple drivers as part of pulling up the data to get alignment. I am actually surprised they are doing this. Flow dissector is not the cheapest function in the world and it seems like a shame to be calling it this early and not even getting a hash for the skb. Also, this is another instance of touching the data so if we do get to the point of trying steering packets without cache miss (another thread); this undermines that. Seems like it might be just as well to pull up a fixed number of bytes (ixgbe want min of 60 bytes), or don't pull up anything avoid the cache miss here and let the stack pull up as needed.

Quoted 11 lines · Tom Herbert

Hmm, thanks for pointing that out. It looks like this is called by a couple drivers as part of pulling up the data to get alignment. I am actually surprised they are doing this. Flow dissector is not the cheapest function in the world and it seems like a shame to be calling it this early and not even getting a hash for the skb. Also, this is another instance of touching the data so if we do get to the point of trying steering packets without cache miss (another thread); this undermines that. Seems like it might be just as well to pull up a fixed number of bytes (ixgbe want min of 60 bytes), or don't pull up anything avoid the cache miss here and let the stack pull up as needed.

Except that when for example mlx4 is able to pull only the headers instead of a fixed amount of bytes, we increased the performance, because GRO could cook packets twice as big for tunneled traffic.

( commit cfecec56ae7c7c40f23fbdac04acee027ca3bd66 )

1) Increasing performance by prefetching headers is a separate matter, completely orthogonal to this.

2) Taking the opportunity to also give back the l4 hash as a bonus has been considered, but tests showed no clear benefit.

For typical cases where l4 hash is not used (RPS and RFS being not
enabled by default on linux), computing it brings nothing but added complexity.

Neither is GRE enabled by default in Linux and it is not a typical case. So that patch is an optimization for a very narrow use case that impacts the core data path for everyone. Please at least consider making it configurable.

Why ipv6 stack itself does not trigger the issue ?

Maybe the driver itself does not properly align IP headers on sparc ?

Make sure NET_IP_ALIGN is 2 on your build.

Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

From: Eric Dumazet <[email protected]>
Date: Fri, 29 Jan 2016 10:33:48 -0800

Quoted 4 lines

Why ipv6 stack itself does not trigger the issue ?

Maybe the driver itself does not properly align IP headers on sparc ?

Quoted 5 lines

Make sure NET_IP_ALIGN is 2 on your build.

Note that x86 does not care, but a driver should always align Ethernet header to NET_IP_ALIGN, so that IP headers are aligned to 4 bytes boundaries.

Eric, please reread the original posting, the driver is trying to do exactly this.

This is the part in the driver where it is trying to figure out how much to pull in order to align the packet before it gets sent into the stack.

That's why only the flow dissector sees this problem.

But really adding unaligned() accesses in flow dissector would slow it quite a lot on MIPS and others.

I have no idea why reading iph->saddr or iph->daddr would not hit the problem, but accessing the 32bit ipv6 flow label would be an issue.

Something is fishy.

But really adding unaligned() accesses in flow dissector would slow it quite a lot on MIPS and others.

Quoted 5 lines · Eric Dumazet

I have no idea why reading iph->saddr or iph->daddr would not hit the problem, but accessing the 32bit ipv6 flow label would be an issue.

Something is fishy.

I was wondering about this myself. Even on sparc, I only first ran into the errors for ipv6. I dont know if the fact that the saddr is memcpy'ed masks the error (even though the problem is still there).

Quoted 7 lines · Sowmini Varadhan

Quoted 5 lines · Eric Dumazet

I have no idea why reading iph->saddr or iph->daddr would not hit the problem, but accessing the 32bit ipv6 flow label would be an issue.

Something is fishy.

Quoted 4 lines

I was wondering about this myself. Even on sparc, I only first ran into the errors for ipv6. I dont know if the fact that the saddr is memcpy'ed masks the error (even though the problem is still there).

Oh right, recent work in flow dissector added all these memcpy()

I was still looking at linux-4.3 ;)

Quoted 7 lines · Sowmini Varadhan

Quoted 5 lines · Eric Dumazet

I have no idea why reading iph->saddr or iph->daddr would not hit the problem, but accessing the 32bit ipv6 flow label would be an issue.

Something is fishy.

Quoted 4 lines

I was wondering about this myself. Even on sparc, I only first ran into the errors for ipv6. I dont know if the fact that the saddr is memcpy'ed masks the error (even though the problem is still there).

Oh right, recent work in flow dissector added all these memcpy()

I was still looking at linux-4.3 ;)

It has to be something recent. I know back when I wrote the code I tested it on a few different architectures and had to add a few bits in __skb_get_poff so that it would read doff as a u8 instead of bitfield in a u32.

Looking at the code it seems like this should be an easy fix. Just swap the two lines that acquire and test the flow label with the check for dissector_uses_key(... _KEY_FLOW_LABEL). Then ixgbe will stop trying to grab flow labels.

Quoted 9 lines · Alexander Duyck

It has to be something recent. I know back when I wrote the code I tested it on a few different architectures and had to add a few bits in __skb_get_poff so that it would read doff as a u8 instead of bitfield in a u32.

Looking at the code it seems like this should be an easy fix. Just swap the two lines that acquire and test the flow label with the check for dissector_uses_key(... _KEY_FLOW_LABEL). Then ixgbe will stop trying to grab flow labels.

Note that if we properly align headers, we also align TCP/UDP payload.

Meaning that copying data in recvmsg() is likely faster in some cpus. (The ones having NET_IP_ALIGN set to 2 presumably)

It also means DMA becomes dramatically slower as it introduces a partial write access for the start of every frame. It is why we had set NET_IP_ALIGN to 0 on x86 since DMA was becoming more expensive when unaligned then reading IP unaligned headers.

The gain on recvmsg would probably be minimal. The only time I have seen any significant speed-up for copying is if you can get both ends aligned to something like 16B.

Quoted 4 lines · Alexander Duyck

It also means DMA becomes dramatically slower as it introduces a partial write access for the start of every frame. It is why we had set NET_IP_ALIGN to 0 on x86 since DMA was becoming more expensive when unaligned then reading IP unaligned headers.

Well, I guess that if you have an arch where DMA accesses are slow and NET_IP_ALIGN = 2, you are out of luck. This is why some platforms are better than others.

Quoted 4 lines

The gain on recvmsg would probably be minimal. The only time I have seen any significant speed-up for copying is if you can get both ends aligned to something like 16B.

On modern intel cpus, this does not matter at all, sure. It took a while before "rep movsb" finally did the right thing.

memcpy() and friends implementations are much slower on some older arches (when dealing with unaligned src/dst)

arch/mips/lib/memcpy.S is a gem ;)

Sent: 29 January 2016 22:29

On modern intel cpus, this does not matter at all, sure. It took a while before "rep movsb" finally did the right thing.

From: Eric Dumazet

Quoted 1 line

Sent: 29 January 2016 22:29

...

Quoted 2 lines

On modern intel cpus, this does not matter at all, sure. It took a while before "rep movsb" finally did the right thing.

Unfortunately memcpy_to_io() etc now map to 'rep movsb', and that can only be optimisied for cached addresses.

So copies to/from pcie space get done as byte copies, slower than slow.

The same is true when usespace has used mmap() to directly access pcie memory.

There isn't a standard wrapper than generates 'rep movsd'.

net/core/flow_dissector.c +8 -0

@@ -102,6 +102,17 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int 
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static inline __be32 ip6_flowlabel_align(const u8 *hdr)
+{
+       union {
+               __u8 w[4];
+               __u32 flow;
+       } ip6_flow;
+
+       memcpy(ip6_flow.w, hdr, 4);

This patch fixes an issue with unaligned accesses when using eth_get_headlen on a page that was DMA aligned instead of being IP aligned. The fact is when trying to check the length we don't need to be looking at the flow label so we can reorder the checks to first check if we are supposed to gather the flow label and then make the call to actually get it.

Reported-by Sowmini Varadhan <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

---

Quoted 10 lines

This patch fixes an issue with unaligned accesses when using eth_get_headlen on a page that was DMA aligned instead of being IP aligned. The fact is when trying to check the length we don't need to be looking at the flow label so we can reorder the checks to first check if we are supposed to gather the flow label and then make the call to actually get it.

Reported-by Sowmini Varadhan <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

---

You did not quite follow the discussion Alexander, we did not exactly took a decision about this bug.

As we mentioned, there are other parts that need care.

key_keyid->keyid = *keyid;

Please address them, instead of having to 'wait' for the next crash.

From: Eric Dumazet <[email protected]>
Date: Fri, 29 Jan 2016 19:23:54 -0800

Quoted 21 lines · Alexander Duyck

Quoted 10 lines

This patch fixes an issue with unaligned accesses when using eth_get_headlen on a page that was DMA aligned instead of being IP aligned. The fact is when trying to check the length we don't need to be looking at the flow label so we can reorder the checks to first check if we are supposed to gather the flow label and then make the call to actually get it.

Reported-by Sowmini Varadhan <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

---

You did not quite follow the discussion Alexander, we did not exactly took a decision about this bug.

As we mentioned, there are other parts that need care.

key_keyid->keyid = *keyid;

Please address them, instead of having to 'wait' for the next crash.

Indeed, this is a more fundamental issue.

This change in and of itself isn't bad, and is probably a suitable micro-optimization for net-next, but it doesn't fix the bug in question.

BTW, even a memcpy(&key_addrs->v4addrs, &iph->saddr, 8) could crash, as the compiler can certainly assume src and dst are 4 bytes aligned, and could use word accesses when inlining memcpy() even on Sparc.

Apparently the compiler used by Sowmini is gentle.

Quoted 5 lines · Eric Dumazet

BTW, even a memcpy(&key_addrs->v4addrs, &iph->saddr, 8) could crash, as the compiler can certainly assume src and dst are 4 bytes aligned, and could use word accesses when inlining memcpy() even on Sparc.

Apparently the compiler used by Sowmini is gentle.

One more subtlety that I missed until now..

eth_get_headlen passes in flow_keys_buf_dissector (NOT flow_keys_dissector!)

So FLOW_DISSECTOR_KEY_IPV4_ADDRS is not set, and this helps to dodge the unaligned iph->saddr access.

But as others have pointed out, much of this code is brittle because it's accessing the data before the driver has had a chance to align things. The page_offset initialization of NET_IP_ALIGN, with all its weaknesses, at least matches (in principle) the prescription used for the xmit path.

--Sowmini

That is not the only case, If a GRE TEB packet is ever received and flow dissector is called for any reason (like skb_get_hash) there's going to be problems-- and this doesn't require GRE to even be configured on the host.

I have a patch that changes the 32-bit accesses in flow_dissector to use get_unaligned_be32. I don't have access to any machines that care about alignment (only x86). Do you know if there is an alternate way to test this other than running on architecture like Sparc?

Quoted 4 lines · Eric Dumazet

BTW, even a memcpy(&key_addrs->v4addrs, &iph->saddr, 8) could crash, as the compiler can certainly assume src and dst are 4 bytes aligned, and could use word accesses when inlining memcpy() even on Sparc.

Quoted 1 line

Apparently the compiler used by Sowmini is gentle.

One more subtlety that I missed until now..

eth_get_headlen passes in flow_keys_buf_dissector (NOT flow_keys_dissector!)

So FLOW_DISSECTOR_KEY_IPV4_ADDRS is not set, and this helps to dodge the unaligned iph->saddr access.

But as others have pointed out, much of this code is brittle because it's accessing the data before the driver has had a chance to align things. The page_offset initialization of NET_IP_ALIGN, with all its weaknesses, at least matches (in principle) the prescription used for the xmit path.

As far as the NET_IP_ALIGN on the page offset I think it is a horrible idea. Basically it means we have to allocate at least 1K more space than we need since page sizes are powers of 2, and buffer sizes in hardware are measured in 1K increments.

Quoted 4 lines · Alexander Duyck

As far as the NET_IP_ALIGN on the page offset I think it is a horrible idea. Basically it means we have to allocate at least 1K more space than we need since page sizes are powers of 2, and buffer sizes in hardware are measured in 1K increments.

I thought Ethernet frames were something like ~1500 bytes.

Can't you program the NIC to not receive frames bigger than MTU + 18, or at least 2046 ?

Of course, if the NIC is able to push 2048 bytes (instead of 2046 max), using an offset of 2 is not going to work.

The NIC hardware only allows us to set receive buffer sizes in 1K increments. I think fm10k may be an exception in that it may be able to support 512 byte, but otherwise igb and ixgbe are strictly set in 1K increments. So basically if we did the NET_IP_ALIGN of the page we are limited to 1K buffers if we were to retain the current page reuse, or we could go up to 3K if we were to allocate order 1 pages for receive buffers as currently occurs with FCoE.

I would really prefer to keep the pages DMA aligned, and the skb->data IP aligned. If nothing else it has the advantage of actually having the proper alignment on all the headers if I only pull the outer headers and leave the inner headers in the page.. :-/

From: Alexander Duyck <[email protected]>
Date: Sat, 30 Jan 2016 17:13:35 -0800

Quoted 8 lines

The NIC hardware only allows us to set receive buffer sizes in 1K increments. I think fm10k may be an exception in that it may be able to support 512 byte, but otherwise igb and ixgbe are strictly set in 1K increments. So basically if we did the NET_IP_ALIGN of the page we are limited to 1K buffers if we were to retain the current page reuse, or we could go up to 3K if we were to allocate order 1 pages for receive buffers as currently occurs with FCoE.

Quoted 4 lines

I would really prefer to keep the pages DMA aligned, and the skb->data IP aligned. If nothing else it has the advantage of actually having the proper alignment on all the headers if I only pull the outer headers and leave the inner headers in the page.. :-/

I am unconvinced by your arguments. People getting hit by these arguments don't care. For those not hitting the problem, nothing is going to change.

x86 uses NET_IP_ALIGN==0, and therefore nothing will change for the only platform people care a lot about. PowerPC too.

For platforms with NET_IP_ALIGN==2 or similar, all of these cases where 1K extra buffer is allocator or _whatever_ other side effect you think is such as huge deal... trust me it's _nothing_ compared to the alternative:

1) Unaligned accesses

2) Constantly having to say "oops this case, oops that case, oops shit we have to handle this too" as we constantly patch up flow dissector and other paths.

Please, pass aligned buffers to the flow dissector, and let's put this behind us, _provably_, for good.

Thanks.

If nothing else we can discuss this further in Seville as I am just not convinced changing the drivers is the right way to go about fixing this, and I won't have the time to really get around to doing any of it until I have come back from the conference.

Quoted 5 lines

If nothing else we can discuss this further in Seville as I am just not convinced changing the drivers is the right way to go about fixing this, and I won't have the time to really get around to doing any of it until I have come back from the conference.

Ok, let me think about this some more.

It's a shame that "put two garbage bytes into the FIFO at the beginning of every frame" isn't a standard hw feature. Or maybe I misunderstand what's going on in all of these drivers...

Sent: 31 January 2016 03:45
To: [email protected]

Quoted 4 lines

I would really prefer to keep the pages DMA aligned, and the skb->data IP aligned. If nothing else it has the advantage of actually having the proper alignment on all the headers if I only pull the outer headers and leave the inner headers in the page.. :-/

I am unconvinced by your arguments. People getting hit by these arguments don't care. For those not hitting the problem, nothing is going to change.

x86 uses NET_IP_ALIGN==0, and therefore nothing will change for the only platform people care a lot about. PowerPC too.

For platforms with NET_IP_ALIGN==2 or similar, all of these cases where 1K extra buffer is allocator or _whatever_ other side effect you think is such as huge deal... trust me it's _nothing_ compared to the alternative:

1) Unaligned accesses

From: David Miller

Quoted 2 lines

Sent: 31 January 2016 03:45
To: [email protected]

...

Quoted 17 lines

Quoted 4 lines

I would really prefer to keep the pages DMA aligned, and the skb->data IP aligned. If nothing else it has the advantage of actually having the proper alignment on all the headers if I only pull the outer headers and leave the inner headers in the page.. :-/

I am unconvinced by your arguments. People getting hit by these arguments don't care. For those not hitting the problem, nothing is going to change.

x86 uses NET_IP_ALIGN==0, and therefore nothing will change for the only platform people care a lot about. PowerPC too.

For platforms with NET_IP_ALIGN==2 or similar, all of these cases where 1K extra buffer is allocator or _whatever_ other side effect you think is such as huge deal... trust me it's _nothing_ compared to the alternative:

Quoted 1 line

1) Unaligned accesses

Remember the even if you do a 'realignment copy' of the IP header, at some point the 'userdata' of the packet has to be accessed. Mostly this will be with memcpy() and you want that copy to be aligned.

I really can't believe just how many ethernet chips are now being designed that can't write received frames to '4n+2' boundaries.
It isn't even a new problem. Sun fixed the sbus 'DMA' part so that it would to sbus burst transfers to 4n+2 aligned buffers a long time ago...

Sent: 01 February 2016 18:18

Quoted 5 lines

Quoted 1 line

1) Unaligned accesses

Remember the even if you do a 'realignment copy' of the IP header, at some point the 'userdata' of the packet has to be accessed. Mostly this will be with memcpy() and you want that copy to be aligned.

The problem is, aligned by what? For x86 anything other than 16 byte alignment will require some realignment anyway since internally it is essentially doing some SSE style copy based on rep movsb.

I'm sure it
is similar for other architectures that have moved on from 32b to 64b in that they want to move by the largest available data unit and 2 byte alignment vs 4 byte doesn't make that much difference anymore.

The thing you have to keep in mind is that your standard TCP frame has 66 bytes of headers(14 Ethernet, 20 IP, 20 TCP, 12 TCP options). This means that you are crossing over into the next cache line by either 2 or 4 bytes depending on NET_IP_ALIGN. On a system with an 8 byte long it doesn't really make a difference one way or the other since it is still unaligned in terms of the fastest possible memcpy.

Quoted 4 lines

I really can't believe just how many ethernet chips are now being designed that can't write received frames to '4n+2' boundaries.
It isn't even a new problem. Sun fixed the sbus 'DMA' part so that it would to sbus burst transfers to 4n+2 aligned buffers a long time ago...

The problem here is motivation. It makes sense that Sun would want their headers to be IP aligned since a Sparc processor still needs that. Most NIC vendors don't really care about the more obscure architectures, and even 32 bit is falling out of favor. The market share is just too small and in many cases customers don't run Linux OSes on them anyway so even driver support is minimal. They mostly care about x86_64, PowerPC, and 64 bit ARM. And all 3 of those architectures have efficient unaligned accesses.