eth_get_headlen() and unaligned accesses...

eth_get_headlen() and unaligned accesses...

[David Miller]

So, we have a bit of a problem, this is on sparc64:

[423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203]  [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624]  [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521]  [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323]  [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128]  [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083]  [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184]  [00000000008243c8] net_rx_action+0xa8/0x1c0

The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.

So any larger than 16-bit access to the IP and later headers will be
unaligned.

We have various ways we can deal with this based upon the capabilities
of the chips involved.  Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?

Re: eth_get_headlen() and unaligned accesses...

[Alexander Duyck]

On 10/09/2014 05:12 PM, David Miller wrote:
> So, we have a bit of a problem, this is on sparc64:
>
> [423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
> [423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
> [423475.767854] Call Trace:
> [423475.772877]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
> [423475.785203]  [000000000042a824] sun4v_do_mna+0x84/0xa0
> [423475.795624]  [0000000000406cd0] sun4v_mna+0x5c/0x68
> [423475.805521]  [000000000081d330] __skb_flow_get_ports+0x70/0xe0
> [423475.817323]  [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
> [423475.829128]  [0000000000843c98] eth_get_headlen+0x38/0xa0
> [423475.840083]  [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
> [423475.851184]  [00000000008243c8] net_rx_action+0xa8/0x1c0
>
> The chip DMA's to the beginning of a frag page and (unless timestamps
> are enabled) that's where the ethernet header begins.
>
> So any larger than 16-bit access to the IP and later headers will be
> unaligned.
>
> We have various ways we can deal with this based upon the capabilities
> of the chips involved.  Can we configure the IGB to put 2 "don't care"
> bytes at the beginning of the packet?

The problem is the igb part expects to be able to use 2K buffers which 
means it will always try to use the full half of a page.  I had 
forgotten that the function this replaced had worked with unaligned 
accesses as all of the fields I was pulling were only 16b in width.  I 
think I assumed that this function was already setup to handle that.

Actually the fix should be pretty simple.  Just do what we already 
appear to be doing for the iph_to_flow_copy_addrs.  We can use memcpy to 
copy the 4 bytes for the port data instead of doing the direct assignment.

I'll try to submit a patch, just need to see if I have a tree setup as 
it has been a couple weeks.

Thanks,

Alex

Re: eth_get_headlen() and unaligned accesses...

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Thu, 09 Oct 2014 20:10:01 -0700

> On 10/09/2014 05:12 PM, David Miller wrote:
>> So, we have a bit of a problem, this is on sparc64:
>>
>> [423475.740836] Kernel unaligned access at TPC[81d330]
>> __skb_flow_get_ports+0x70/0xe0
>> [423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
>> [423475.767854] Call Trace:
>> [423475.772877]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
>> [423475.785203]  [000000000042a824] sun4v_do_mna+0x84/0xa0
>> [423475.795624]  [0000000000406cd0] sun4v_mna+0x5c/0x68
>> [423475.805521]  [000000000081d330] __skb_flow_get_ports+0x70/0xe0
>> [423475.817323]  [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
>> [423475.829128]  [0000000000843c98] eth_get_headlen+0x38/0xa0
>> [423475.840083]  [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
>> [423475.851184]  [00000000008243c8] net_rx_action+0xa8/0x1c0
>>
>> The chip DMA's to the beginning of a frag page and (unless timestamps
>> are enabled) that's where the ethernet header begins.
>>
>> So any larger than 16-bit access to the IP and later headers will be
>> unaligned.
>>
>> We have various ways we can deal with this based upon the capabilities
>> of the chips involved.  Can we configure the IGB to put 2 "don't care"
>> bytes at the beginning of the packet?
> 
> The problem is the igb part expects to be able to use 2K buffers which
> means it will always try to use the full half of a page.

Let me try to ask this again.

Can you configure the MAC to put two garbage bytes at the head of
the packet data as it feeds it into the DMA fifos on the IGB chip?

This is an essential (again: _essential_) feature for chips that
manage RX buffers as power-of-2 chunks of pages, as it is the only
cheap way to get the IP headers 32-bit aligned in those power-of-2 DMA
buffer blocks.

That would solve the whole problem.

RE: eth_get_headlen() and unaligned accesses...

[David Laight]

From: David Miller
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Thu, 09 Oct 2014 20:10:01 -0700
> 
> > On 10/09/2014 05:12 PM, David Miller wrote:
> >> So, we have a bit of a problem, this is on sparc64:
> >>
> >> [423475.740836] Kernel unaligned access at TPC[81d330]
> >> __skb_flow_get_ports+0x70/0xe0
> >> [423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
> >> [423475.767854] Call Trace:
> >> [423475.772877]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
> >> [423475.785203]  [000000000042a824] sun4v_do_mna+0x84/0xa0
> >> [423475.795624]  [0000000000406cd0] sun4v_mna+0x5c/0x68
> >> [423475.805521]  [000000000081d330] __skb_flow_get_ports+0x70/0xe0
> >> [423475.817323]  [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
> >> [423475.829128]  [0000000000843c98] eth_get_headlen+0x38/0xa0
> >> [423475.840083]  [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
> >> [423475.851184]  [00000000008243c8] net_rx_action+0xa8/0x1c0
> >>
> >> The chip DMA's to the beginning of a frag page and (unless timestamps
> >> are enabled) that's where the ethernet header begins.
> >>
> >> So any larger than 16-bit access to the IP and later headers will be
> >> unaligned.
> >>
> >> We have various ways we can deal with this based upon the capabilities
> >> of the chips involved.  Can we configure the IGB to put 2 "don't care"
> >> bytes at the beginning of the packet?
> >
> > The problem is the igb part expects to be able to use 2K buffers which
> > means it will always try to use the full half of a page.
> 
> Let me try to ask this again.
> 
> Can you configure the MAC to put two garbage bytes at the head of
> the packet data as it feeds it into the DMA fifos on the IGB chip?
> 
> This is an essential (again: _essential_) feature for chips that
> manage RX buffers as power-of-2 chunks of pages, as it is the only
> cheap way to get the IP headers 32-bit aligned in those power-of-2 DMA
> buffer blocks.

Not only the IP header, at some point all of the data is likely to
accessed (preferably) as words - if only as a final copy of the
userdata somewhere.

So if you can't efficiently DMA the destination MAC address to a 4n+2
boundary you might as well do a realigning copy of the entire frame
into a separate skb.
Which probably means you should use a different ethernet adapter.

This isn't exactly a new problem.
One of the changes between the Sbus 'DMA' and 'DMA+' parts was that the
latter would do Sbus bursts for transfers (from the lance) that started
misaligned.

	David

[PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[alexander.duyck]

From: Alexander Duyck <alexander.h.duyck@redhat.com>

This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.

In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value.  This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.

Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 include/net/flow_keys.h         |   10 ++++++----
 net/core/flow_dissector.c       |   21 +++++++++++++--------
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c9ac06c..326eb3d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2993,7 +2993,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 		return false;
 	}
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
-		fk->ports = skb_flow_get_ports(skb, noff, proto);
+		skb_flow_get_ports(fk, skb, noff, proto);
 
 	return true;
 }
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 7ee2df0..66235f4 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -33,11 +33,13 @@ static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys
 {
 	return __skb_flow_dissect(skb, flow, NULL, 0, 0, 0);
 }
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen_proto);
-static inline __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+			  int thoff, u8 ip_proto, void *data, int hlen_proto);
+static inline void skb_flow_get_ports(struct flow_keys *flow,
+				      const struct sk_buff *skb, int thoff,
+				      u8 ip_proto)
 {
-	return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
+	__skb_flow_get_ports(flow, skb, thoff, ip_proto, NULL, 0);
 }
 u32 flow_hash_from_keys(struct flow_keys *keys);
 unsigned int flow_get_hlen(const unsigned char *data, unsigned int max_len,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 8560dea..baf8fe3 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -28,6 +28,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
 
 /**
  * __skb_flow_get_ports - extract the upper layer ports and return them
+ * @flow: Flow key to place port informatin into
  * @skb: sk_buff to extract the ports from
  * @thoff: transport header offset
  * @ip_proto: protocol for which to get port offset
@@ -37,8 +38,8 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
  * The function will try to retrieve the ports at offset thoff + poff where poff
  * is the protocol port offset returned from proto_ports_offset
  */
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+			  int thoff, u8 ip_proto, void *data, int hlen)
 {
 	int poff = proto_ports_offset(ip_proto);
 
@@ -48,15 +49,19 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 	}
 
 	if (poff >= 0) {
-		__be32 *ports, _ports;
+		__be32 *ports;
 
 		ports = __skb_header_pointer(skb, thoff + poff,
-					     sizeof(_ports), data, hlen, &_ports);
-		if (ports)
-			return *ports;
+					     sizeof(flow->ports), data, hlen,
+					     &flow->ports);
+		if (ports) {
+			if (&flow->ports != ports)
+				memcpy(&flow->ports, ports, sizeof(flow->ports));
+			return;
+		}
 	}
 
-	return 0;
+	memset(&flow->ports, 0, sizeof(flow->ports));
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
@@ -231,7 +236,7 @@ ipv6:
 
 	flow->n_proto = proto;
 	flow->ip_proto = ip_proto;
-	flow->ports = __skb_flow_get_ports(skb, nhoff, ip_proto, data, hlen);
+	__skb_flow_get_ports(flow, skb, nhoff, ip_proto, data, hlen);
 	flow->thoff = (u16) nhoff;
 
 	return true;

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: alexander.duyck@gmail.com
Date: Thu, 09 Oct 2014 21:03:28 -0700

> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> 
> This patch addresses a kernel unaligned access bug seen on a sparc64 system
> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
> be32 pointer which was then having the value directly returned.
> 
> In order to keep the handling of the ports consistent with how we were
> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
> with a memcpy to the flow key ports value.  This way it should stay a
> memcpy on systems that cannot handle unaligned access, and will likely be
> converted to a 32b assignment on the systems that can support it.
> 
> Reported-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>

Guess what the compiler will output for the memcpy()....

	*(u32 *)dest = *(u32 *)src;

Using memcpy() is never a valid way to avoid misaligned loads and
stores.

If the types have a given alignment, the compiler can legitimately
expand the memcpy() inline with suitably sized loads and stores.

Please see my other reply in the original thread, we have to use
the hardware when we can to manage this situation, by configuring
it to output two garbage bytes before the packet contents when
DMA'ing into power-of-2 aligned blocks of memory.

Thanks.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Alexander Duyck]

On 10/09/2014 09:47 PM, David Miller wrote:
> From: alexander.duyck@gmail.com
> Date: Thu, 09 Oct 2014 21:03:28 -0700
>
>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>>
>> This patch addresses a kernel unaligned access bug seen on a sparc64 system
>> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
>> be32 pointer which was then having the value directly returned.
>>
>> In order to keep the handling of the ports consistent with how we were
>> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
>> with a memcpy to the flow key ports value.  This way it should stay a
>> memcpy on systems that cannot handle unaligned access, and will likely be
>> converted to a 32b assignment on the systems that can support it.
>>
>> Reported-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> Guess what the compiler will output for the memcpy()....
>
> 	*(u32 *)dest = *(u32 *)src;
>
> Using memcpy() is never a valid way to avoid misaligned loads and
> stores.

If needed we could convert ports to a __be16 I suppose.

> If the types have a given alignment, the compiler can legitimately
> expand the memcpy() inline with suitably sized loads and stores.

That is what I get for trying to come up with a fix at the end of the 
day.  Although it does leave me scratching my head why the IPv4 and IPv6 
addresses were not causing unaligned accesses or were they triggering 
them as well?

> Please see my other reply in the original thread, we have to use
> the hardware when we can to manage this situation, by configuring
> it to output two garbage bytes before the packet contents when
> DMA'ing into power-of-2 aligned blocks of memory.
>
> Thanks.

The problem is the igb / ixgbe / fm10k hardware doesn't have a means of 
inserting padding from its side.  The whole point of the xxx_get_headlen 
functions was to determine the size needed in order to generate the 
correct memcpy so that we could generate an aligned header.  It sounds 
like we can't do that with this function now because there are multiple 
instances where the data will be accessed unaligned if it isn't aligned 
in the first place.

The way I see it now there are two possible solutions.  The first one 
being to update the flow hash functions to work with unaligned accesses, 
and the second being to split the get_headlen code flow off and for now 
use my original code which already had all the alignment issues 
resolved.  I'm open to suggestions either way, but for now I will try to 
just address the items you have pointed out here with the current patch 
and submit a v2.

Thanks,

Alex

RE: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Laight]

From: Alexander Duyck
> On 10/09/2014 09:47 PM, David Miller wrote:
> > From: alexander.duyck@gmail.com
> > Date: Thu, 09 Oct 2014 21:03:28 -0700
> >
> >> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> >>
> >> This patch addresses a kernel unaligned access bug seen on a sparc64 system
> >> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
> >> be32 pointer which was then having the value directly returned.
> >>
> >> In order to keep the handling of the ports consistent with how we were
> >> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
> >> with a memcpy to the flow key ports value.  This way it should stay a
> >> memcpy on systems that cannot handle unaligned access, and will likely be
> >> converted to a 32b assignment on the systems that can support it.
> >>
> >> Reported-by: David S. Miller <davem@davemloft.net>
> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> > Guess what the compiler will output for the memcpy()....
> >
> > 	*(u32 *)dest = *(u32 *)src;
> >
> > Using memcpy() is never a valid way to avoid misaligned loads and
> > stores.
> 
> If needed we could convert ports to a __be16 I suppose.

You would have to cast it somewhere where the compiler cannot
tell what the original type was.
This usually means you have to call a non-static function, which
might have to be in a different compilation unit.

...
> That is what I get for trying to come up with a fix at the end of the
> day.  Although it does leave me scratching my head why the IPv4 and IPv6
> addresses were not causing unaligned accesses or were they triggering
> them as well?

I think there is code to copy the IP and TCP headers to aligned memory
before they are parsed.

...
> 
> The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> inserting padding from its side...

Shoot the hardware engineers.

You aren't going to get the performance you expect from a 10Ge card
unless the rx buffers are 'correctly' aligned.

	David

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Alexander Duyck]

On 10/10/2014 07:57 AM, David Laight wrote:
> From: Alexander Duyck
>> On 10/09/2014 09:47 PM, David Miller wrote:
>>> From: alexander.duyck@gmail.com
>>> Date: Thu, 09 Oct 2014 21:03:28 -0700
>>>
>>>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>>
>>>> This patch addresses a kernel unaligned access bug seen on a sparc64 system
>>>> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
>>>> be32 pointer which was then having the value directly returned.
>>>>
>>>> In order to keep the handling of the ports consistent with how we were
>>>> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
>>>> with a memcpy to the flow key ports value.  This way it should stay a
>>>> memcpy on systems that cannot handle unaligned access, and will likely be
>>>> converted to a 32b assignment on the systems that can support it.
>>>>
>>>> Reported-by: David S. Miller <davem@davemloft.net>
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> Guess what the compiler will output for the memcpy()....
>>>
>>> 	*(u32 *)dest = *(u32 *)src;
>>>
>>> Using memcpy() is never a valid way to avoid misaligned loads and
>>> stores.
>> If needed we could convert ports to a __be16 I suppose.
> You would have to cast it somewhere where the compiler cannot
> tell what the original type was.
> This usually means you have to call a non-static function, which
> might have to be in a different compilation unit.
>
> ...

The pointer itself gets assigned from skb->data + offset, since 
skb->data is an unsigned char I would think that it should be safe to 
cast it as a __be16 and have that stick.  It is only if we cast it as a 
__be32 that it should be an issue as we are casting a value that starts 
off only byte aligned.

>> That is what I get for trying to come up with a fix at the end of the
>> day.  Although it does leave me scratching my head why the IPv4 and IPv6
>> addresses were not causing unaligned accesses or were they triggering
>> them as well?
> I think there is code to copy the IP and TCP headers to aligned memory
> before they are parsed.
>
> ...

The code I am talking about is run before we actually get into the 
header parsing.  So for example if you take a look at 
iph_to_flow_copy_addrs that should be getting called before we even get 
to the code that is supposedly triggering this and it is doing 32b 
aligned accesses for the source and destination IP addresses.

>> The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
>> inserting padding from its side...
> Shoot the hardware engineers.
>
> You aren't going to get the performance you expect from a 10Ge card
> unless the rx buffers are 'correctly' aligned.
>
> 	David
>

I think you might be coming to this a little late.  The igb and ixgbe 
drivers had been working this way for a long time.  We did a memcpy to 
move the headers from the page and into the skb->data at an aligned 
offset.  In order to determine the length to memcpy we had a function 
that could walk through the DMA aligned data to get the header length.  
The function for that was replaced with the __skb_flow_dissect as it was 
considered a duplication of code with the flow_dissection functions.  
However that is obviously not the case now that we are hitting these 
alignment issues.

The question I have in all this is do I push forward and make 
__skb_flow_dissect work with unaligned accesses, or do I back off and 
put something equivilent to igb/ixgbe_get_headlen functions in the 
kernel in order to deal with the unaligned accesses as they had no 
issues with them since they were only concerned with getting the header 
length and kept all accesses 16b aligned.

Thanks,

Alex

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Eric Dumazet]

On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:

> I think you might be coming to this a little late.  The igb and ixgbe 
> drivers had been working this way for a long time.  We did a memcpy to 
> move the headers from the page and into the skb->data at an aligned 
> offset.  In order to determine the length to memcpy we had a function 
> that could walk through the DMA aligned data to get the header length.  
> The function for that was replaced with the __skb_flow_dissect as it was 
> considered a duplication of code with the flow_dissection functions.  
> However that is obviously not the case now that we are hitting these 
> alignment issues.
> 
> The question I have in all this is do I push forward and make 
> __skb_flow_dissect work with unaligned accesses, or do I back off and 
> put something equivilent to igb/ixgbe_get_headlen functions in the 
> kernel in order to deal with the unaligned accesses as they had no 
> issues with them since they were only concerned with getting the header 
> length and kept all accesses 16b aligned.
> 

I see nothing wrong dealing with unaligned accesses, as these helpers
are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Alexander Duyck]

On 10/10/2014 08:29 AM, Eric Dumazet wrote:
> On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:
>
>> I think you might be coming to this a little late.  The igb and ixgbe
>> drivers had been working this way for a long time.  We did a memcpy to
>> move the headers from the page and into the skb->data at an aligned
>> offset.  In order to determine the length to memcpy we had a function
>> that could walk through the DMA aligned data to get the header length.
>> The function for that was replaced with the __skb_flow_dissect as it was
>> considered a duplication of code with the flow_dissection functions.
>> However that is obviously not the case now that we are hitting these
>> alignment issues.
>>
>> The question I have in all this is do I push forward and make
>> __skb_flow_dissect work with unaligned accesses, or do I back off and
>> put something equivilent to igb/ixgbe_get_headlen functions in the
>> kernel in order to deal with the unaligned accesses as they had no
>> issues with them since they were only concerned with getting the header
>> length and kept all accesses 16b aligned.
>>
> I see nothing wrong dealing with unaligned accesses, as these helpers
> are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.

Still it means possibly hurting performance on those platforms that 
don't have it defined.

If I just use get_unaligned that is pretty easy in terms of cleanup for 
the ports and IPv4 addresses, the IPv6 will still be a significant 
hurdle to overcome though.

Thanks,

Alex

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Fri, 10 Oct 2014 09:50:17 -0700

> If I just use get_unaligned that is pretty easy in terms of cleanup
> for the ports and IPv4 addresses, the IPv6 will still be a significant
> hurdle to overcome though.

Actually, it's not that simple.

When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.

So we might need to sprinkle a "attribute((packed))" here and there
to make it work.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Alexander Duyck]

On 10/10/2014 10:58 AM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Fri, 10 Oct 2014 09:50:17 -0700
>
>> If I just use get_unaligned that is pretty easy in terms of cleanup
>> for the ports and IPv4 addresses, the IPv6 will still be a significant
>> hurdle to overcome though.
> Actually, it's not that simple.
>
> When the compiler sees things like "th->doff" it will load the 32-bit
> word that 4-bit field contains and extract the value using shifts and
> masking.
>
> So we might need to sprinkle a "attribute((packed))" here and there
> to make it work.

I'll do some digging.  I know I had this working in igb/ixgbe before so
I probably just need to add a few small tweaks to resolve the remaining
issues for v4 of the patch.

Thanks,

Alex

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Fri, 10 Oct 2014 11:02:04 -0700

> On 10/10/2014 10:58 AM, David Miller wrote:
>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>> Date: Fri, 10 Oct 2014 09:50:17 -0700
>>
>>> If I just use get_unaligned that is pretty easy in terms of cleanup
>>> for the ports and IPv4 addresses, the IPv6 will still be a significant
>>> hurdle to overcome though.
>> Actually, it's not that simple.
>>
>> When the compiler sees things like "th->doff" it will load the 32-bit
>> word that 4-bit field contains and extract the value using shifts and
>> masking.
>>
>> So we might need to sprinkle a "attribute((packed))" here and there
>> to make it work.
> 
> I'll do some digging.  I know I had this working in igb/ixgbe before so
> I probably just need to add a few small tweaks to resolve the remaining
> issues for v4 of the patch.

Ok, I think v3 + resolving the th->doff thing will get rid of everything.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Fri, 10 Oct 2014 11:02:04 -0700

> On 10/10/2014 10:58 AM, David Miller wrote:
>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>> Date: Fri, 10 Oct 2014 09:50:17 -0700
>>
>>> If I just use get_unaligned that is pretty easy in terms of cleanup
>>> for the ports and IPv4 addresses, the IPv6 will still be a significant
>>> hurdle to overcome though.
>> Actually, it's not that simple.
>>
>> When the compiler sees things like "th->doff" it will load the 32-bit
>> word that 4-bit field contains and extract the value using shifts and
>> masking.
>>
>> So we might need to sprinkle a "attribute((packed))" here and there
>> to make it work.
> 
> I'll do some digging.  I know I had this working in igb/ixgbe before so
> I probably just need to add a few small tweaks to resolve the remaining
> issues for v4 of the patch.

Your original code works because you do things like "byte[12] & 0xf0" to
extract these fields.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)

> Your original code works because you do things like "byte[12] & 0xf0" to
> extract these fields.

Changing that th->doff sequence to instead be:

		const u8 *bp;
		u8 buf[13];

		bp = __skb_header_pointer(skb, poff, sizeof(buf),
					  data, hlen, &buf);
		if (!bp)
			return poff;

		poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
		break;

on top of your v3 patch works for me.

Please double-check my calculations.

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Alexander Duyck]

On 10/10/2014 11:22 AM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)
>
>> Your original code works because you do things like "byte[12] & 0xf0" to
>> extract these fields.
> Changing that th->doff sequence to instead be:
>
> 		const u8 *bp;
> 		u8 buf[13];
>
> 		bp = __skb_header_pointer(skb, poff, sizeof(buf),
> 					  data, hlen, &buf);
> 		if (!bp)
> 			return poff;
>
> 		poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
> 		break;
>
> on top of your v3 patch works for me.
>
> Please double-check my calculations.

Any reason why you are grabbing all 13 bytes instead of just the 1 we 
care about?  Seems like we could just use a u8 buf instead of the array 
since we are only grabbing doff.

Thanks,

Alex

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Fri, 10 Oct 2014 11:53:35 -0700

> On 10/10/2014 11:22 AM, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)
>>
>>> Your original code works because you do things like "byte[12] & 0xf0"
>>> to
>>> extract these fields.
>> Changing that th->doff sequence to instead be:
>>
>> 		const u8 *bp;
>> 		u8 buf[13];
>>
>> 		bp = __skb_header_pointer(skb, poff, sizeof(buf),
>> 					  data, hlen, &buf);
>> 		if (!bp)
>> 			return poff;
>>
>> 		poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
>> 		break;
>>
>> on top of your v3 patch works for me.
>>
>> Please double-check my calculations.
> 
> Any reason why you are grabbing all 13 bytes instead of just the 1 we
> care about?  Seems like we could just use a u8 buf instead of the
> array since we are only grabbing doff.

No reason, just a thinko, my brain implemented this as if skb_header_pointer
worked like skb_pull :-/

RE: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Laight]

From: David Miller
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Fri, 10 Oct 2014 09:50:17 -0700
> 
> > If I just use get_unaligned that is pretty easy in terms of cleanup
> > for the ports and IPv4 addresses, the IPv6 will still be a significant
> > hurdle to overcome though.
> 
> Actually, it's not that simple.
> 
> When the compiler sees things like "th->doff" it will load the 32-bit
> word that 4-bit field contains and extract the value using shifts and
> masking.
> 
> So we might need to sprinkle a "attribute((packed))" here and there
> to make it work.

Marking a structure 'packed' forces the compiler to generate byte accesses.
It is enough to mark the 32bit members with __attribute__((aligned(2)))
(or use a typedef for u32 that includes that attribute).
Then the compiler will use 16bit accesses for that field.

	David

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Eric Dumazet]

On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:

> I think there is code to copy the IP and TCP headers to aligned memory
> before they are parsed.

There is no such thing. You are here on netdev list, please read the
code before doing such claims.

> ...
> > 
> > The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> > inserting padding from its side...
> 
> Shoot the hardware engineers.
> 
> You aren't going to get the performance you expect from a 10Ge card
> unless the rx buffers are 'correctly' aligned.

That is simply not true on current x86 cpus. They simply dont care at
all.

You cannot blame Intel for other arches.

RE: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Laight]

From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 10 October 2014 16:34
> To: David Laight
> Cc: 'alexander.h.duyck@redhat.com'; David Miller; alexander.duyck@gmail.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
> 
> On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
> 
> > I think there is code to copy the IP and TCP headers to aligned memory
> > before they are parsed.
> 
> There is no such thing. You are here on netdev list, please read the
> code before doing such claims.

I did say 'I think'...
I must be thinking of some similar code somewhere else.
Possibly just the code that ensures the header isn't fragmented.

> > > The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> > > inserting padding from its side...
> >
> > Shoot the hardware engineers.
> >
> > You aren't going to get the performance you expect from a 10Ge card
> > unless the rx buffers are 'correctly' aligned.
> 
> That is simply not true on current x86 cpus. They simply dont care at
> all.

I was referring to using them on sparc64, not x86.

I know that current intel x86 cpu have support for misaligned 'rep movsd',
but I thought there was still a small cost (maybe one clock) for
single word transfers.
So maybe they care 'just a little bit'.

> You cannot blame Intel for other arches.

True, but this does mean that you don't really want to use these adapters
on a system that can't to unaligned accesses.

	David

Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 10 Oct 2014 08:33:39 -0700

> On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
> 
>> I think there is code to copy the IP and TCP headers to aligned memory
>> before they are parsed.
> 
> There is no such thing. You are here on netdev list, please read the
> code before doing such claims.

+1

[PATCH v2] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[alexander.duyck]

From: Alexander Duyck <alexander.h.duyck@redhat.com>

This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.

In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value.  This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---

v2: Changed ports to a __be16 * to prevent optimization for __be32 *

 drivers/net/bonding/bond_main.c |    2 +-
 include/net/flow_keys.h         |   10 ++++++----
 net/core/flow_dissector.c       |   22 ++++++++++++++--------
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c9ac06c..326eb3d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2993,7 +2993,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 		return false;
 	}
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
-		fk->ports = skb_flow_get_ports(skb, noff, proto);
+		skb_flow_get_ports(fk, skb, noff, proto);
 
 	return true;
 }
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 7ee2df0..66235f4 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -33,11 +33,13 @@ static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys
 {
 	return __skb_flow_dissect(skb, flow, NULL, 0, 0, 0);
 }
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen_proto);
-static inline __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+			  int thoff, u8 ip_proto, void *data, int hlen_proto);
+static inline void skb_flow_get_ports(struct flow_keys *flow,
+				      const struct sk_buff *skb, int thoff,
+				      u8 ip_proto)
 {
-	return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
+	__skb_flow_get_ports(flow, skb, thoff, ip_proto, NULL, 0);
 }
 u32 flow_hash_from_keys(struct flow_keys *keys);
 unsigned int flow_get_hlen(const unsigned char *data, unsigned int max_len,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 8560dea..e789a39 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -28,6 +28,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
 
 /**
  * __skb_flow_get_ports - extract the upper layer ports and return them
+ * @flow: Flow key to place port informatin into
  * @skb: sk_buff to extract the ports from
  * @thoff: transport header offset
  * @ip_proto: protocol for which to get port offset
@@ -37,8 +38,8 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
  * The function will try to retrieve the ports at offset thoff + poff where poff
  * is the protocol port offset returned from proto_ports_offset
  */
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+			  int thoff, u8 ip_proto, void *data, int hlen)
 {
 	int poff = proto_ports_offset(ip_proto);
 
@@ -48,15 +49,20 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 	}
 
 	if (poff >= 0) {
-		__be32 *ports, _ports;
+		__be16 *ports;
 
 		ports = __skb_header_pointer(skb, thoff + poff,
-					     sizeof(_ports), data, hlen, &_ports);
-		if (ports)
-			return *ports;
+					     sizeof(flow->ports), data, hlen,
+					     &flow->ports);
+		if (ports) {
+			if (flow->port16 != ports)
+				memcpy(&flow->ports, ports,
+				       sizeof(flow->ports));
+			return;
+		}
 	}
 
-	return 0;
+	memset(&flow->ports, 0, sizeof(flow->ports));
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
@@ -231,7 +237,7 @@ ipv6:
 
 	flow->n_proto = proto;
 	flow->ip_proto = ip_proto;
-	flow->ports = __skb_flow_get_ports(skb, nhoff, ip_proto, data, hlen);
+	__skb_flow_get_ports(flow, skb, nhoff, ip_proto, data, hlen);
 	flow->thoff = (u16) nhoff;
 
 	return true;

Re: [PATCH v2] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[Eric Dumazet]

On Fri, 2014-10-10 at 07:59 -0700, alexander.duyck@gmail.com wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> 
> This patch addresses a kernel unaligned access bug seen on a sparc64 system
> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
> be32 pointer which was then having the value directly returned.
> 
> In order to keep the handling of the ports consistent with how we were
> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
> with a memcpy to the flow key ports value.  This way it should stay a
> memcpy on systems that cannot handle unaligned access, and will likely be
> converted to a 32b assignment on the systems that can support it.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---

I believe you also need to take care of calls to ipv6_addr_hash()

The IPv4 part also needs something in iph_to_flow_copy_addrs(),
otherwise compiler might assume  &iph->saddr is word aligned.

Re: [PATCH v2] flow-dissector: Fix alignment issue in __skb_flow_get_ports

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 10 Oct 2014 08:36:16 -0700

> On Fri, 2014-10-10 at 07:59 -0700, alexander.duyck@gmail.com wrote:
>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>> 
>> This patch addresses a kernel unaligned access bug seen on a sparc64 system
>> with an igb adapter.  Specifically the __skb_flow_get_ports was returning a
>> be32 pointer which was then having the value directly returned.
>> 
>> In order to keep the handling of the ports consistent with how we were
>> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
>> with a memcpy to the flow key ports value.  This way it should stay a
>> memcpy on systems that cannot handle unaligned access, and will likely be
>> converted to a 32b assignment on the systems that can support it.
>> 
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
> 
> I believe you also need to take care of calls to ipv6_addr_hash()
> 
> The IPv4 part also needs something in iph_to_flow_copy_addrs(),
> otherwise compiler might assume  &iph->saddr is word aligned.

Right, I still get the unaligned accesses even with this patch:

[487667.804777] Kernel unaligned access at TPC[81de40] __skb_get_poff+0xa0/0x100
[487667.818767] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #3
[487667.830930] Call Trace:
[487667.835954]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[487667.848276]  [000000000042a824] sun4v_do_mna+0x84/0xa0
[487667.858698]  [0000000000406cd0] sun4v_mna+0x5c/0x68
[487667.868592]  [000000000081de40] __skb_get_poff+0xa0/0x100
[487667.879531]  [0000000000843d2c] eth_get_headlen+0x6c/0xa0
[487667.890486]  [000000001003ed54] igb_poll+0x8d4/0xf60 [igb]
[487667.901584]  [0000000000824428] net_rx_action+0xa8/0x1c0
[487667.912348]  [000000000046a1fc] __do_softirq+0xdc/0x2e0
[487667.922932]  [000000000042b96c] do_softirq_own_stack+0x2c/0x40
[487667.934751]  [000000000046a6b8] irq_exit+0x98/0xc0
[487667.944473]  [000000000042b900] handler_irq+0xc0/0x100
[487667.954888]  [00000000004208b4] tl0_irq5+0x14/0x20
[487667.964610]  [000000000042c0d4] arch_cpu_idle+0x74/0xa0
[487667.975201]  [0000000000499cdc] cpu_startup_entry+0x17c/0x2c0
[487667.986843]  [0000000000ab69b8] start_kernel+0x408/0x418
[487667.997603]  [00000000008c3628] tlb_fixup_done+0x98/0xb0

Re: eth_get_headlen() and unaligned accesses...

[Tom Herbert]

On Thu, Oct 9, 2014 at 5:12 PM, David Miller <davem@davemloft.net> wrote:
>
> So, we have a bit of a problem, this is on sparc64:
>
> [423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
> [423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
> [423475.767854] Call Trace:
> [423475.772877]  [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
> [423475.785203]  [000000000042a824] sun4v_do_mna+0x84/0xa0
> [423475.795624]  [0000000000406cd0] sun4v_mna+0x5c/0x68
> [423475.805521]  [000000000081d330] __skb_flow_get_ports+0x70/0xe0
> [423475.817323]  [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
> [423475.829128]  [0000000000843c98] eth_get_headlen+0x38/0xa0
> [423475.840083]  [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
> [423475.851184]  [00000000008243c8] net_rx_action+0xa8/0x1c0
>
> The chip DMA's to the beginning of a frag page and (unless timestamps
> are enabled) that's where the ethernet header begins.
>
> So any larger than 16-bit access to the IP and later headers will be
> unaligned.
>
Will this also be a problem for GRE packet carrying Ethernet packet?

> We have various ways we can deal with this based upon the capabilities
> of the chips involved.  Can we configure the IGB to put 2 "don't care"
> bytes at the beginning of the packet?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html