vlan: use ether_addr_equal_64bits to instead of ether_addr_equal

PATCH In review Ding Tianhong 2 participants 6 messages Mar 3, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ding Tianhong

author

2014-03-03 01:15 UTC #0 lore raw

PATCH 1/1 vlan: use ether_addr_equal_64bits to instead of ether_addr_equal

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.

Signed-off-by Ding Tianhong <[email protected]>

---
 net/8021q/vlan.c      | 10 +++++-----
 net/8021q/vlan_core.c |  2 +-
 net/8021q/vlan_dev.c  | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

net/8021q/vlan.c +5 -5

index ec99099..16fb0f4 100644
@@ -286,19 +286,19 @@ static void vlan_sync_address(struct net_device *dev,
 	struct vlan_dev_priv *vlan = vlan_dev_priv(vlandev);
 
 	/* May be called without an actual change */
-	if (ether_addr_equal(vlan->real_dev_addr, dev->dev_addr))
+	if (ether_addr_equal_64bits(vlan->real_dev_addr, dev->dev_addr))
 		return;
 
 	/* vlan address was different from the old address and is equal to
 	 * the new address */
-	if (!ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) &&
-	    ether_addr_equal(vlandev->dev_addr, dev->dev_addr))
+	if (!ether_addr_equal_64bits(vlandev->dev_addr, vlan->real_dev_addr) &&
+	    ether_addr_equal_64bits(vlandev->dev_addr, dev->dev_addr))
 		dev_uc_del(dev, vlandev->dev_addr);
 
 	/* vlan address was equal to the old address and is different from
 	 * the new address */
-	if (ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) &&
-	    !ether_addr_equal(vlandev->dev_addr, dev->dev_addr))
+	if (ether_addr_equal_64bits(vlandev->dev_addr, vlan->real_dev_addr) &&
+	    !ether_addr_equal_64bits(vlandev->dev_addr, dev->dev_addr))
 		dev_uc_add(dev, vlandev->dev_addr);
 
 	ether_addr_copy(vlan->real_dev_addr, dev->dev_addr);

net/8021q/vlan_core.c +1 -1

index 6ee48aa..39cbb46 100644
@@ -26,7 +26,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 		/* Our lower layer thinks this is not local, let's make sure.
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
-		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
+		if (ether_addr_equal_64bits(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 	}

net/8021q/vlan_dev.c +5 -5

index 76d8fab..5e1edd6 100644
@@ -286,7 +286,7 @@ static int vlan_dev_open(struct net_device *dev)
 	    !(vlan->flags & VLAN_FLAG_LOOSE_BINDING))
 		return -ENETDOWN;
 
-	if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) {
+	if (!ether_addr_equal_64bits(dev->dev_addr, real_dev->dev_addr)) {
 		err = dev_uc_add(real_dev, dev->dev_addr);
 		if (err < 0)
 			goto out;

@@ -319,7 +319,7 @@ clear_allmulti:
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(real_dev, -1);
 del_unicast:
-	if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr))
+	if (!ether_addr_equal_64bits(dev->dev_addr, real_dev->dev_addr))
 		dev_uc_del(real_dev, dev->dev_addr);
 out:
 	netif_carrier_off(dev);

@@ -338,7 +338,7 @@ static int vlan_dev_stop(struct net_device *dev)
 	if (dev->flags & IFF_PROMISC)
 		dev_set_promiscuity(real_dev, -1);
 
-	if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr))
+	if (!ether_addr_equal_64bits(dev->dev_addr, real_dev->dev_addr))
 		dev_uc_del(real_dev, dev->dev_addr);
 
 	netif_carrier_off(dev);

@@ -357,13 +357,13 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
 	if (!(dev->flags & IFF_UP))
 		goto out;
 
-	if (!ether_addr_equal(addr->sa_data, real_dev->dev_addr)) {
+	if (!ether_addr_equal_64bits(addr->sa_data, real_dev->dev_addr)) {
 		err = dev_uc_add(real_dev, addr->sa_data);
 		if (err < 0)
 			return err;
 	}
 
-	if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr))
+	if (!ether_addr_equal_64bits(dev->dev_addr, real_dev->dev_addr))
 		dev_uc_del(real_dev, dev->dev_addr);
 
 out:

Signature · 1 line

1.8.0

Joe Perches

2014-03-03 01:37 UTC #1 lore raw

Quoted 4 lines · Ding Tianhong

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.

It's true that ether_addr_equal_64bits is faster for 64 bit architectures. It's 1 64 bit compare and 1 48 bit mask instead of a 32 bit and a 16 bit compare.

I think unless the code is _extremely_ performance or fast-path sensitive, ether_addr_equal should be used.

I believe the vlan_do_receive use qualifies.

Are any of the others on the fast path?

Do you have any performance numbers?

Joe Perches

2014-03-03 01:49 UTC #2 lore raw

In reply to Ding Tianhong

Quoted 4 lines · Ding Tianhong

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.

[]

net/8021q/vlan_core.c

Quoted 1 line

net/8021q/vlan_core.c +0 -0

[]

net/8021q/vlan_core.c bool vlan_do_receive(struct sk_buff **skbp) · line 31

Quoted 8 lines

net/8021q/vlan_core.c +1 -1

@@ -26,7 +26,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 		/* Our lower layer thinks this is not local, let's make sure.
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
-		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
+		if (ether_addr_equal_64bits(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 	}

Hi again Ding

If you do have performance numbers:

The lines above this ether_addr_equal_64 are:

	if (skb->pkt_type == PACKET_OTHERHOST) {
		/* Our lower layer thinks this is not local, let's make sure.
		 * This allows the VLAN to have a different MAC than the
		 * underlying device, and still route correctly. */
		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
			skb->pkt_type = PACKET_HOST;
	}

Maybe it'd be faster overall to add an unlikely
to the == test

	if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) {

Ding Tianhong

author

2014-03-03 02:50 UTC #3 lore raw

Quoted 31 lines · Joe Perches

Quoted 4 lines · Ding Tianhong
Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.
[]
net/8021q/vlan_core.c
Quoted 1 line
net/8021q/vlan_core.c +0 -0
[]
net/8021q/vlan_core.c bool vlan_do_receive(struct sk_buff **skbp) · line 31
Quoted 8 lines
net/8021q/vlan_core.c +1 -1
@@ -26,7 +26,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 		/* Our lower layer thinks this is not local, let's make sure.
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
-		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
+		if (ether_addr_equal_64bits(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 	}
Hi again Ding
If you do have performance numbers:
The lines above this ether_addr_equal_64 are:
	if (skb->pkt_type == PACKET_OTHERHOST) {
		/* Our lower layer thinks this is not local, let's make sure.
		 * This allows the VLAN to have a different MAC than the
		 * underlying device, and still route correctly. */
		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
			skb->pkt_type = PACKET_HOST;
	}

Quoted 7 lines

Maybe it'd be faster overall to add an unlikely
to the == test
	if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) {

Sorry, I can't understand it clearly, do you mean that if the skb deliver to the vlan dev, it is impossible that the pkt_type is PACKET_OTHERHOST at most time?

Regards
Ding

Quoted 1 line

Joe Perches

2014-03-03 04:36 UTC #4 lore raw

(adding Jiri Pirko to cc's)

Quoted 32 lines · Ding Tianhong

Quoted 31 lines · Joe Perches
Quoted 4 lines · Ding Tianhong
Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.
[]
net/8021q/vlan_core.c
Quoted 1 line
net/8021q/vlan_core.c +0 -0
[]
net/8021q/vlan_core.c bool vlan_do_receive(struct sk_buff **skbp) · line 31
Quoted 8 lines
net/8021q/vlan_core.c +1 -1
@@ -26,7 +26,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 		/* Our lower layer thinks this is not local, let's make sure.
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
-		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
+		if (ether_addr_equal_64bits(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 	}
Hi again Ding
If you do have performance numbers:
The lines above this ether_addr_equal_64 are:
	if (skb->pkt_type == PACKET_OTHERHOST) {
		/* Our lower layer thinks this is not local, let's make sure.
		 * This allows the VLAN to have a different MAC than the
		 * underlying device, and still route correctly. */
		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
			skb->pkt_type = PACKET_HOST;
	}

Quoted 7 lines

Quoted 4 lines
Maybe it'd be faster overall to add an unlikely
to the == test
	if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) {
Sorry, I can't understand it clearly, do you mean that if the skb deliver to the vlan dev, it is impossible that the pkt_type is PACKET_OTHERHOST at most time?

No, just that it's maybe better to add an unlikely to the test.

Jiri Pirko's commit 0b5c9db1b modified the code from a switch/case to a specific test.

I don't know how likely it is in normal uses for a packet to be delivered as PACKET_OTHERHOST.

I just hoped that if you in fact had performance numbers for your suggested ether_addr_equal_64bits test, you might be able to run those same tests with if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) and see if that was better or worse for performance.

cheers, Joe

Ding Tianhong

author

2014-03-03 06:06 UTC #5 lore raw

Quoted 52 lines · Joe Perches

(adding Jiri Pirko to cc's)
Quoted 32 lines · Ding Tianhong
Quoted 31 lines · Joe Perches
Quoted 4 lines · Ding Tianhong
Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be used when each argument is an array within a structure that contains at least two bytes of data beyond the array, so it is safe to use it for vlan.
[]
net/8021q/vlan_core.c
Quoted 1 line
net/8021q/vlan_core.c +0 -0
[]
net/8021q/vlan_core.c bool vlan_do_receive(struct sk_buff **skbp) · line 31
Quoted 8 lines
net/8021q/vlan_core.c +1 -1
@@ -26,7 +26,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 		/* Our lower layer thinks this is not local, let's make sure.
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
-		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
+		if (ether_addr_equal_64bits(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 	}
Hi again Ding
If you do have performance numbers:
The lines above this ether_addr_equal_64 are:
	if (skb->pkt_type == PACKET_OTHERHOST) {
		/* Our lower layer thinks this is not local, let's make sure.
		 * This allows the VLAN to have a different MAC than the
		 * underlying device, and still route correctly. */
		if (ether_addr_equal(eth_hdr(skb)->h_dest, vlan_dev->dev_addr))
			skb->pkt_type = PACKET_HOST;
	}
Quoted 7 lines
Quoted 4 lines
Maybe it'd be faster overall to add an unlikely
to the == test
	if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) {
Sorry, I can't understand it clearly, do you mean that if the skb deliver to the vlan dev, it is impossible that the pkt_type is PACKET_OTHERHOST at most time?
No, just that it's maybe better to add an unlikely to the test.
Jiri Pirko's commit 0b5c9db1b modified the code
from a switch/case to a specific test.
I don't know how likely it is in normal uses for
a packet to be delivered as PACKET_OTHERHOST.

Quoted 8 lines

I just hoped that if you in fact had performance numbers for your suggested ether_addr_equal_64bits test, you might be able to run those same tests with if (unlikely(skb->pkt_type == PACKET_OTHERHOST)) and see if that was better or worse for performance.
cheers, Joe

OK, I got it and I will review the commit 0b5c9db1b, try to see the performance.

Regards
Ding

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html