[PATCH 0/3] A few minor clean-ups to eth_type_trans

[PATCH 0/3] A few minor clean-ups to eth_type_trans

[Alexander Duyck]

This series addresses a few minor issues I found in eth_type_trans that
that allow us to gain back something like 3 or more cycles per packet.  

The first change is to drop the byte swap since it isn't necessary.  On x86
we could just check the first byte and compare that against the upper 8
bits of the Ethertype to determine if we are dealing with a size value or
not.

The second makes it so that the value we read in to test for multicast can
be used for the address comparison.  This allows us to avoid a second read
of the destination address.

The final change is to avoid some unneeded instructions in computing the
Ethernet header pointer.  When we start the call the Ethernet header is at
skb->data, so we just use that rather than computing mac_header, and then
adding that back to skb->head.

---

Alexander Duyck (3):
      etherdev: Avoid unnecessary byte swap in check for Ethertype
      etherdev: Process is_multicast_ether_addr at same size as other operations
      etherdev: Use skb->data to retrieve Ethernet header instead of eth_hdr


 include/linux/etherdevice.h |   24 +++++++++++++++++++++++-
 net/ethernet/eth.c          |    7 ++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

--

[PATCH 1/3] etherdev: Avoid unnecessary byte swap in check for Ethertype

[Alexander Duyck]

This change takes advantage of the fact that ETH_P_802_3_MIN is aligned to
512 so as a result we can actually ignore the lower 8b when comparing the
Ethertype to ETH_P_802_3_MIN.  This allows us to avoid a byte swap by simply
masking the value and comparing it to the byte swapped value for
ETH_P_802_3_MIN.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 net/ethernet/eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index f3bad41d725f..60069318d5d1 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -178,7 +178,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(netdev_uses_dsa(dev)))
 		return htons(ETH_P_XDSA);
 
-	if (likely(ntohs(eth->h_proto) >= ETH_P_802_3_MIN))
+	if (likely((eth->h_proto & htons(0xFF00)) >= htons(ETH_P_802_3_MIN)))
 		return eth->h_proto;
 
 	/*

Re: [PATCH 1/3] etherdev: Avoid unnecessary byte swap in check for Ethertype

[Eric Dumazet]

On Thu, 2015-04-30 at 14:53 -0700, Alexander Duyck wrote:
> This change takes advantage of the fact that ETH_P_802_3_MIN is aligned to
> 512 so as a result we can actually ignore the lower 8b when comparing the
> Ethertype to ETH_P_802_3_MIN.  This allows us to avoid a byte swap by simply
> masking the value and comparing it to the byte swapped value for
> ETH_P_802_3_MIN.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>  net/ethernet/eth.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index f3bad41d725f..60069318d5d1 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -178,7 +178,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>  	if (unlikely(netdev_uses_dsa(dev)))
>  		return htons(ETH_P_XDSA);
>  
> -	if (likely(ntohs(eth->h_proto) >= ETH_P_802_3_MIN))
> +	if (likely((eth->h_proto & htons(0xFF00)) >= htons(ETH_P_802_3_MIN)))
>  		return eth->h_proto;

Then, a byte operation on x86 is shorter/faster than u16 one.

You also could use

if (likely(*(u8 *)&eth->h_proto >= (ETH_P_802_3_MIN>>8)))
	return eth->h_proto;

I would at least leave a comment here to explain the logic.

Re: [PATCH 1/3] etherdev: Avoid unnecessary byte swap in check for Ethertype

[Alexander Duyck]

On 04/30/2015 04:03 PM, Eric Dumazet wrote:
> On Thu, 2015-04-30 at 14:53 -0700, Alexander Duyck wrote:
>> This change takes advantage of the fact that ETH_P_802_3_MIN is aligned to
>> 512 so as a result we can actually ignore the lower 8b when comparing the
>> Ethertype to ETH_P_802_3_MIN.  This allows us to avoid a byte swap by simply
>> masking the value and comparing it to the byte swapped value for
>> ETH_P_802_3_MIN.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>>   net/ethernet/eth.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index f3bad41d725f..60069318d5d1 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -178,7 +178,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>>   	if (unlikely(netdev_uses_dsa(dev)))
>>   		return htons(ETH_P_XDSA);
>>   
>> -	if (likely(ntohs(eth->h_proto) >= ETH_P_802_3_MIN))
>> +	if (likely((eth->h_proto & htons(0xFF00)) >= htons(ETH_P_802_3_MIN)))
>>   		return eth->h_proto;
> Then, a byte operation on x86 is shorter/faster than u16 one.
>
> You also could use
>
> if (likely(*(u8 *)&eth->h_proto >= (ETH_P_802_3_MIN>>8)))
> 	return eth->h_proto;
>
> I would at least leave a comment here to explain the logic.

Actually a byte operation itself is not faster.  Note in the next line 
we are returning the value.  So what you typically end up with by doing 
it that way would be 2 reads, one for the u8 and one for the u16 return 
value.  That is actually what I am trying to address in the second patch 
in the set since we were doing a 8b test on the first byte of the 
address followed by a 64b read.

The advantage with the way I wrote this is that the compiler itself 
should be able to sort out how it wants to test the value while 
accessing it in a 16b size.  So at worst case it is a mask and compare, 
followed by a return of the value.  From what I have seen the compiler 
seems to be smart enough on x86 anyway to just convert this into a one 
byte compare on AL and then return the result in AX.  I would suspect 
that for bit-endian systems it would likely just perform the compare.

- Alex

Re: [PATCH 1/3] etherdev: Avoid unnecessary byte swap in check for Ethertype

[Eric Dumazet]

On Thu, 2015-04-30 at 16:24 -0700, Alexander Duyck wrote:

> Actually a byte operation itself is not faster.  Note in the next line 
> we are returning the value.  So what you typically end up with by doing 
> it that way would be 2 reads, one for the u8 and one for the u16 return 
> value.  That is actually what I am trying to address in the second patch 
> in the set since we were doing a 8b test on the first byte of the 
> address followed by a 64b read.
> 
> The advantage with the way I wrote this is that the compiler itself 
> should be able to sort out how it wants to test the value while 
> accessing it in a 16b size.  So at worst case it is a mask and compare, 
> followed by a return of the value.  From what I have seen the compiler 
> seems to be smart enough on x86 anyway to just convert this into a one 
> byte compare on AL and then return the result in AX.  I would suspect 
> that for bit-endian systems it would likely just perform the compare.
> 

My compiler (4.8.2 (Ubuntu 4.8.2-19ubuntu1)) does the following :

 62d:	0f b7 42 0c          	movzwl 0xc(%rdx),%eax
 631:	0f b6 d0             	movzbl %al,%edx
 634:	83 fa 05             	cmp    $0x5,%edx
 637:	7e 02                	jle    63b <eth_type_trans+0x8b>
 639:	c9                   	leaveq 
 63a:	c3                   	retq   

Presumably this would be possible

          	movzwl 0xc(%rdx),%eax
            	cmp    $0x5,%al
               	jle    63b <eth_type_trans+0x8b>
               	leaveq 
               	retq   

Re: [PATCH 1/3] etherdev: Avoid unnecessary byte swap in check for Ethertype

[Alexander Duyck]

On 04/30/2015 05:13 PM, Eric Dumazet wrote:
> On Thu, 2015-04-30 at 16:24 -0700, Alexander Duyck wrote:
>
>> Actually a byte operation itself is not faster.  Note in the next line
>> we are returning the value.  So what you typically end up with by doing
>> it that way would be 2 reads, one for the u8 and one for the u16 return
>> value.  That is actually what I am trying to address in the second patch
>> in the set since we were doing a 8b test on the first byte of the
>> address followed by a 64b read.
>>
>> The advantage with the way I wrote this is that the compiler itself
>> should be able to sort out how it wants to test the value while
>> accessing it in a 16b size.  So at worst case it is a mask and compare,
>> followed by a return of the value.  From what I have seen the compiler
>> seems to be smart enough on x86 anyway to just convert this into a one
>> byte compare on AL and then return the result in AX.  I would suspect
>> that for bit-endian systems it would likely just perform the compare.
>>
>
> My compiler (4.8.2 (Ubuntu 4.8.2-19ubuntu1)) does the following :
>
>   62d:	0f b7 42 0c          	movzwl 0xc(%rdx),%eax
>   631:	0f b6 d0             	movzbl %al,%edx
>   634:	83 fa 05             	cmp    $0x5,%edx
>   637:	7e 02                	jle    63b <eth_type_trans+0x8b>
>   639:	c9                   	leaveq
>   63a:	c3                   	retq
>
> Presumably this would be possible
>
>            	movzwl 0xc(%rdx),%eax
>              	cmp    $0x5,%al
>                 	jle    63b <eth_type_trans+0x8b>
>                 	leaveq
>                 	retq
>
>

My compiler (5.0.1 (Red Hat 5.0.1-0.1)) does like what you have in the 
"would be possible" example.  What I end up with is something like this:
  648:   0f b7 42 0c             movzwl 0xc(%rdx),%eax
  64c:   3c 05                   cmp    $0x5,%al
  64e:   76 40                   jbe    690 <eth_type_trans+0xc0>

The assembler before my patch was:
  652:   0f b7 40 0c             movzwl 0xc(%rax),%eax
  656:   89 c2                   mov    %eax,%edx
  658:   66 c1 c2 08             rol    $0x8,%dx
  65c:   0f b7 d2                movzwl %dx,%edx
  65f:   81 fa ff 05 00 00       cmp    $0x5ff,%edx
  665:   7e 41                   jle    6a8 <eth_type_trans+0xd8>

The savings isn't meant to be anything huge for the patch, maybe a cycle 
or two.  I suspect the before on your system is probably something 
similar to what I had so we are still probably dropping at least 2 
instructions.

- Alex

[PATCH 2/3] etherdev: Process is_multicast_ether_addr at same size as other operations

[Alexander Duyck]

This change makes it so that we process the address in
is_multicast_ether_addr at the same size as the other calls.  This allows
us to avoid duplicate reads when used with other calls such as
is_zero_ether_addr or eth_addr_copy.  In addition I have added a 64 bit
version of the function so in eth_type_trans we can process the destination
address as a 64 bit value throughout.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 include/linux/etherdevice.h |   24 +++++++++++++++++++++++-
 net/ethernet/eth.c          |    2 +-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 606563ef8a72..c4a10f991fe0 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -110,7 +110,29 @@ static inline bool is_zero_ether_addr(const u8 *addr)
  */
 static inline bool is_multicast_ether_addr(const u8 *addr)
 {
-	return 0x01 & addr[0];
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+	u32 a = *(const u32 *)addr;
+#else
+	u16 a = *(const u16 *)addr;
+#endif
+#ifdef __BIG_ENDIAN
+	return 0x01 & (a >> ((sizeof(a) * 8) - 8));
+#else
+	return 0x01 & a;
+#endif
+}
+
+static inline bool is_multicast_ether_addr_64bits(const u8 addr[6+2])
+{
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
+#ifdef __BIG_ENDIAN
+	return 0x01 & ((*(const u64 *)addr) >> 56);
+#else
+	return 0x01 & (*(const u64 *)addr);
+#endif
+#else
+	return is_multicast_ether_addr(addr);
+#endif
 }
 
 /**
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 60069318d5d1..21c211e9fd5a 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -159,7 +159,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	skb_pull_inline(skb, ETH_HLEN);
 	eth = eth_hdr(skb);
 
-	if (unlikely(is_multicast_ether_addr(eth->h_dest))) {
+	if (unlikely(is_multicast_ether_addr_64bits(eth->h_dest))) {
 		if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
 			skb->pkt_type = PACKET_BROADCAST;
 		else

[PATCH 3/3] etherdev: Use skb->data to retrieve Ethernet header instead of eth_hdr

[Alexander Duyck]

Avoid recomputing the Ethernet header location and instead just use the
pointer provided by skb->data.  The problem with using eth_hdr is that the
compiler wasn't smart enough to realize that skb->head + skb->mac_header
was the same thing as skb->data before it added ETH_HLEN.  By just caching
it off before calling skb_pull_inline we can avoid a few unnecessary
instructions.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 net/ethernet/eth.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 21c211e9fd5a..314e4c5a5a5e 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -156,8 +156,9 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
+
+	eth = (struct ethhdr *)skb->data;
 	skb_pull_inline(skb, ETH_HLEN);
-	eth = eth_hdr(skb);
 
 	if (unlikely(is_multicast_ether_addr_64bits(eth->h_dest))) {
 		if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))

Re: [PATCH 0/3] A few minor clean-ups to eth_type_trans

[Alexei Starovoitov]

On Thu, Apr 30, 2015 at 02:53:42PM -0700, Alexander Duyck wrote:
> This series addresses a few minor issues I found in eth_type_trans that
> that allow us to gain back something like 3 or more cycles per packet.  
> 
> The first change is to drop the byte swap since it isn't necessary.  On x86
> we could just check the first byte and compare that against the upper 8
> bits of the Ethertype to determine if we are dealing with a size value or
> not.
> 
> The second makes it so that the value we read in to test for multicast can
> be used for the address comparison.  This allows us to avoid a second read
> of the destination address.
> 
> The final change is to avoid some unneeded instructions in computing the
> Ethernet header pointer.  When we start the call the Ethernet header is at
> skb->data, so we just use that rather than computing mac_header, and then
> adding that back to skb->head.

Great stuff! Excellent optimizations.

Only the comment 'ETH_P_802_3_MIN is aligned to 512' through me off.
It's divisible by 256 that matters.

Re: [PATCH 0/3] A few minor clean-ups to eth_type_trans

[Alexander Duyck]

On 04/30/2015 03:35 PM, Alexei Starovoitov wrote:
> On Thu, Apr 30, 2015 at 02:53:42PM -0700, Alexander Duyck wrote:
>> This series addresses a few minor issues I found in eth_type_trans that
>> that allow us to gain back something like 3 or more cycles per packet.
>>
>> The first change is to drop the byte swap since it isn't necessary.  On x86
>> we could just check the first byte and compare that against the upper 8
>> bits of the Ethertype to determine if we are dealing with a size value or
>> not.
>>
>> The second makes it so that the value we read in to test for multicast can
>> be used for the address comparison.  This allows us to avoid a second read
>> of the destination address.
>>
>> The final change is to avoid some unneeded instructions in computing the
>> Ethernet header pointer.  When we start the call the Ethernet header is at
>> skb->data, so we just use that rather than computing mac_header, and then
>> adding that back to skb->head.
> Great stuff! Excellent optimizations.
>
> Only the comment 'ETH_P_802_3_MIN is aligned to 512' through me off.
> It's divisible by 256 that matters.

Yeah, it is 0x0600 hex so we can ignore the lower 8 bits, or in the case 
of little-endian systems the upper 8 bits.  I think when I had 
originally written the patch I was using a mask of 0xFE00, but then I 
realized that all the compiler cared about is knowing which byte it is 
supposed to compare against.

- Alex

RE: [PATCH 0/3] A few minor clean-ups to eth_type_trans

[David Laight]

From: Alexander Duyck
> Sent: 01 May 2015 00:12
...
> > Only the comment 'ETH_P_802_3_MIN is aligned to 512' through me off.
> > It's divisible by 256 that matters.
> 
> Yeah, it is 0x0600 hex so we can ignore the lower 8 bits, or in the case
> of little-endian systems the upper 8 bits.  I think when I had
> originally written the patch I was using a mask of 0xFE00, but then I
> realized that all the compiler cared about is knowing which byte it is
> supposed to compare against.

Isn't the limit actually 1500 (0x5dc) not 1536 (0x600).
Certainly the longest 8802.3 frame has a 'length' field of 1500
followed by the 3 bytes LLC header and 1497 bytes of 'userdata'.
(If you do ISO transport nothing is ever word aligned.)

Whether any 'ethertypes' from 0x5dd to 0x5ff were ever allocated
(and plausibly still in use) is a separate question.
Some below 1500 were allocated.

	David

Re: [PATCH 0/3] A few minor clean-ups to eth_type_trans

[Alexander Duyck]

On 05/01/2015 04:30 AM, David Laight wrote:
> From: Alexander Duyck
>> Sent: 01 May 2015 00:12
> ...
>>> Only the comment 'ETH_P_802_3_MIN is aligned to 512' through me off.
>>> It's divisible by 256 that matters.
>> Yeah, it is 0x0600 hex so we can ignore the lower 8 bits, or in the case
>> of little-endian systems the upper 8 bits.  I think when I had
>> originally written the patch I was using a mask of 0xFE00, but then I
>> realized that all the compiler cared about is knowing which byte it is
>> supposed to compare against.
> Isn't the limit actually 1500 (0x5dc) not 1536 (0x600).
> Certainly the longest 8802.3 frame has a 'length' field of 1500
> followed by the 3 bytes LLC header and 1497 bytes of 'userdata'.
> (If you do ISO transport nothing is ever word aligned.)
>
> Whether any 'ethertypes' from 0x5dd to 0x5ff were ever allocated
> (and plausibly still in use) is a separate question.
> Some below 1500 were allocated.
>
> 	David

Actually it comes straight from the IEEE Std 802.3, sections 3.2.6. If 
the field value is less than or equal to 1500 (05DC hex), then the field 
is length.  If the field is greater than or equal to 1536 (0600 hex), 
then it is an Ethertype.  The region from 1501 to 1535 isn't really 
defined in the standard in terms of behavior, but then again if that 
value is used it likely isn't an 802.3 frame anyway. For the purposes of 
this check all we care about is the 802.3 Ethertypes which the standard 
says are at 0x600 and above.

- Alex

Re: [PATCH 0/3] A few minor clean-ups to eth_type_trans

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Thu, 30 Apr 2015 14:53:42 -0700

> This series addresses a few minor issues I found in eth_type_trans that
> that allow us to gain back something like 3 or more cycles per packet.  
> 
> The first change is to drop the byte swap since it isn't necessary.  On x86
> we could just check the first byte and compare that against the upper 8
> bits of the Ethertype to determine if we are dealing with a size value or
> not.
> 
> The second makes it so that the value we read in to test for multicast can
> be used for the address comparison.  This allows us to avoid a second read
> of the destination address.
> 
> The final change is to avoid some unneeded instructions in computing the
> Ethernet header pointer.  When we start the call the Ethernet header is at
> skb->data, so we just use that rather than computing mac_header, and then
> adding that back to skb->head.

Series applied, thanks Alex.