[PATCH net-next 19/20] net: plip: slight optimization of addr compare

Use possibly more efficient ether_addr_equal
to instead of memcmp.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/plip/plip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c
index 7b4ff35..26614df 100644
--- a/drivers/net/plip/plip.c
+++ b/drivers/net/plip/plip.c

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

-- 
1.8.0

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

On Fri, 27 Dec 2013, Joe Perches wrote:

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

I'm not sure to competely understand the issues.  Could you explain more?

thanks,
julia

On Fri, 2013-12-27 at 21:12 +0100, Julia Lawall wrote:

On Fri, 27 Dec 2013, Joe Perches wrote:

quoted

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

I'm not sure to competely understand the issues.  Could you explain more?

Hi Julia.

Maybe this explanation is helpful?

ethernet addresses are u8[6] (48 bits)

ether_addr_equal_64bits gets passed a pointer to u8[8]
and is more efficient on 64 bit architectures than
ether_addr_equal because the test can be done with a
single compare and shift.

The idea is not to access past the end of the ethernet
address as appropriate (think pointer to eeprom or other
such end-of-addressable memory conditions)

If a struct containing an ethernet address has additional
members after the ethernet address, or the u8[6] address
passed to ether_addr_equal is not going to access past
the end of memory or the structure, then
ether_addr_equal_64bits should be used in lieu of
ether_addr_equal.

Joe

All ether_addr_equal tests in i40e can use the
slightly more efficient ether_addr_equal_64bits.

All addresses passed to the various functions that
use ether_addr_equal are using structs that have 2
or more bytes of additional data after the mac addr
being tested.

struct i40e_mac_filter.macaddr[6] (followed by s16 vlan)
struct net_device.dev_addr (pointer to char array of MAX_ADDR_LEN)
struct sockaddr.sa_data (array of 14 bytes)
struct netdev_hw_addr.addr (pointer to char array of MAX_ADDR_LEN)

Signed-off-by: Joe Perches <joe@perches.com>
---

On Fri, 2013-12-27 at 12:46 -0800, Joe Perches wrote:

On Fri, 2013-12-27 at 21:12 +0100, Julia Lawall wrote:

quoted

On Fri, 27 Dec 2013, Joe Perches wrote:

quoted

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

I'm not sure to competely understand the issues.  Could you explain more?

Hi Julia.

Maybe this explanation is helpful?

ethernet addresses are u8[6] (48 bits)

ether_addr_equal_64bits gets passed a pointer to u8[8]
and is more efficient on 64 bit architectures than
ether_addr_equal because the test can be done with a
single compare and shift.

The idea is not to access past the end of the ethernet
address as appropriate (think pointer to eeprom or other
such end-of-addressable memory conditions)

If a struct containing an ethernet address has additional
members after the ethernet address, or the u8[6] addressa
passed to ether_addr_equal is not going to access past
the end of memory or the structure, then
ether_addr_equal_64bits should be used in lieu of
ether_addr_equal.

I believe this is correct, but maybe the
conditions for using ether_addr_equal_64bits
could be documented a bit better.

Jeff/Intel folk?  What do you think?

 drivers/net/ethernet/intel/i40e/i40e_main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index efdf8a2..4c1d35c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

@@ -992,7 +992,7 @@ static struct i40e_mac_filter *i40e_find_filter(struct i40e_vsi *vsi,
 		return NULL;
 
 	list_for_each_entry(f, &vsi->mac_filter_list, list) {
-		if ((ether_addr_equal(macaddr, f->macaddr)) &&
+		if (ether_addr_equal_64bits(macaddr, f->macaddr) &&
 		    (vlan == f->vlan)    &&
 		    (!is_vf || f->is_vf) &&
 		    (!is_netdev || f->is_netdev))

@@ -1020,7 +1020,7 @@ struct i40e_mac_filter *i40e_find_mac(struct i40e_vsi *vsi, u8 *macaddr,
 		return NULL;
 
 	list_for_each_entry(f, &vsi->mac_filter_list, list) {
-		if ((ether_addr_equal(macaddr, f->macaddr)) &&
+		if (ether_addr_equal_64bits(macaddr, f->macaddr) &&
 		    (!is_vf || f->is_vf) &&
 		    (!is_netdev || f->is_netdev))
 			return f;

@@ -1209,7 +1209,7 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
 
 	netdev_info(netdev, "set mac address=%pM\n", addr->sa_data);
 
-	if (ether_addr_equal(netdev->dev_addr, addr->sa_data))
+	if (ether_addr_equal_64bits(netdev->dev_addr, addr->sa_data))
 		return 0;
 
 	if (vsi->type == I40E_VSI_MAIN) {

@@ -1409,21 +1409,24 @@ static void i40e_set_rx_mode(struct net_device *netdev)
 
 		if (is_multicast_ether_addr(f->macaddr)) {
 			netdev_for_each_mc_addr(mca, netdev) {
-				if (ether_addr_equal(mca->addr, f->macaddr)) {
+				if (ether_addr_equal_64bits(mca->addr,
+							    f->macaddr)) {
 					found = true;
 					break;
 				}
 			}
 		} else {
 			netdev_for_each_uc_addr(uca, netdev) {
-				if (ether_addr_equal(uca->addr, f->macaddr)) {
+				if (ether_addr_equal_64bits(uca->addr,
+							    f->macaddr)) {
 					found = true;
 					break;
 				}
 			}
 
 			for_each_dev_addr(netdev, ha) {
-				if (ether_addr_equal(ha->addr, f->macaddr)) {
+				if (ether_addr_equal_64bits(ha->addr,
+							    f->macaddr)) {
 					found = true;
 					break;
 				}

@@ -1889,7 +1892,8 @@ int i40e_vsi_kill_vlan(struct i40e_vsi *vsi, s16 vid)
 	list_for_each_entry(f, &vsi->mac_filter_list, list) {
 		if (is_netdev) {
 			if (f->vlan &&
-			    ether_addr_equal(netdev->dev_addr, f->macaddr))
+			    ether_addr_equal_64bits(netdev->dev_addr,
+						    f->macaddr))
 				filter_count++;
 		}
 

On Fri, 2013-12-27 at 13:56 -0800, Joe Perches wrote:

All ether_addr_equal tests in i40e can use the
slightly more efficient ether_addr_equal_64bits.

All addresses passed to the various functions that
use ether_addr_equal are using structs that have 2
or more bytes of additional data after the mac addr
being tested.

struct i40e_mac_filter.macaddr[6] (followed by s16 vlan)
struct net_device.dev_addr (pointer to char array of MAX_ADDR_LEN)
struct sockaddr.sa_data (array of 14 bytes)
struct netdev_hw_addr.addr (pointer to char array of MAX_ADDR_LEN)

Signed-off-by: Joe Perches <joe@perches.com>

It looks good, but I would like Jesse and Shannon to review it since we
just were talking about a similar patch last week.

I will add the patch to my queue for now, thanks Joe.

Hi Julia.

Maybe this explanation is helpful?

ethernet addresses are u8[6] (48 bits)

ether_addr_equal_64bits gets passed a pointer to u8[8]
and is more efficient on 64 bit architectures than
ether_addr_equal because the test can be done with a
single compare and shift.

The idea is not to access past the end of the ethernet
address as appropriate (think pointer to eeprom or other
such end-of-addressable memory conditions)

If a struct containing an ethernet address has additional
members after the ethernet address, or the u8[6] address
passed to ether_addr_equal is not going to access past
the end of memory or the structure, then
ether_addr_equal_64bits should be used in lieu of
ether_addr_equal.

I tried the following semantic patch:

@r@
type T;
T *eth;
identifier fld;
type T1;
T1 *eth1;
identifier fld1;
expression e1;
position p;
@@

ether_addr_equal@p(eth->fld, eth1->fld1)

@ok@
type r.T, t1, t2;
identifier r.fld, fld2;
@@

T { ...
    t1 fld[...];
    t2 fld2;
    ...
  };

@ok1@
type r.T1, t1, t2;
identifier r.fld1, fld2;
@@

T1 { ...
    t1 fld1[...];
    t2 fld2;
    ...
  };

@depends on ok && ok1@
position r.p;
@@

*ether_addr_equal@p(...)

The first rule finds an existing call to ether_addr_equal where both 
arguments are structure field references.  It gets the type of the 
structure in each case, and notes the field name.  The next two rules 
check each of the structure declarations to ensure that the field is 
declared as an array and it is followed by at least one other field.  The 
last rule generates some output for cases where both fields pass the test.

Note that this is not at all exhaustive, because it is not checking cases 
where one argument is a parameter that is passed from some call site where 
the argument is a field that has this property.  Thus, it does not find 
the case in drivers/net/ethernet/intel/i40e/i40e_main.c.  Nevertheless, it 
does find a few occurrences, as shown below.  In this output, - indicates 
an item of interest, not something to be removed.  It is not a patch, even 
though it looks like one.

One can get quite a lot more results if one doesn't require that both 
arguments satisfy the property, ie allowing one argument to be a function 
parameter rather than a structure field reference.  So perhaps it would be 
worth making a (more complicated) semantic patch to find such cases.

julia

diff -u -p /var/linuxes/linux-next/drivers/net/wireless/ipw2x00/libipw_rx.c /tmp/nothing/drivers/net/wireless/ipw2x00/libipw_rx.c

--- /var/linuxes/linux-next/drivers/net/wireless/ipw2x00/libipw_rx.c
+++ /tmp/nothing/drivers/net/wireless/ipw2x00/libipw_rx.c

@@ -1468,7 +1468,6 @@ static inline int is_same_network(struct
 	 * as one network */
 	return ((src->ssid_len == dst->ssid_len) &&
 		(src->channel == dst->channel) &&
-		ether_addr_equal(src->bssid, dst->bssid) &&
 		!memcmp(src->ssid, dst->ssid, src->ssid_len));
 }
 

diff -u -p /var/linuxes/linux-next/drivers/scsi/fcoe/fcoe_ctlr.c /tmp/nothing/drivers/scsi/fcoe/fcoe_ctlr.c

--- /var/linuxes/linux-next/drivers/scsi/fcoe/fcoe_ctlr.c
+++ /tmp/nothing/drivers/scsi/fcoe/fcoe_ctlr.c

@@ -339,7 +339,6 @@ static void fcoe_ctlr_announce(struct fc
 	spin_unlock_bh(&fip->ctlr_lock);
 	sel = fip->sel_fcf;
 
-	if (sel && ether_addr_equal(sel->fcf_mac, fip->dest_addr))
 		goto unlock;
 	if (!is_zero_ether_addr(fip->dest_addr)) {
 		printk(KERN_NOTICE "libfcoe: host%d: "

diff -u -p /var/linuxes/linux-next/drivers/scsi/fcoe/fcoe_sysfs.c /tmp/nothing/drivers/scsi/fcoe/fcoe_sysfs.c

--- /var/linuxes/linux-next/drivers/scsi/fcoe/fcoe_sysfs.c
+++ /tmp/nothing/drivers/scsi/fcoe/fcoe_sysfs.c

@@ -657,7 +657,6 @@ static int fcoe_fcf_device_match(struct
 	if (new->switch_name == old->switch_name &&
 	    new->fabric_name == old->fabric_name &&
 	    new->fc_map == old->fc_map &&
-	    ether_addr_equal(new->mac, old->mac))
 		return 1;
 	return 0;
 }

diff -u -p /var/linuxes/linux-next/drivers/staging/slicoss/slicoss.c /tmp/nothing/drivers/staging/slicoss/slicoss.c

--- /var/linuxes/linux-next/drivers/staging/slicoss/slicoss.c
+++ /tmp/nothing/drivers/staging/slicoss/slicoss.c

@@ -787,8 +787,6 @@ static bool slic_mac_filter(struct adapt
 			struct mcast_address *mcaddr = adapter->mcastaddrs;
 
 			while (mcaddr) {
-				if (ether_addr_equal(mcaddr->address,
-						     ether_frame->ether_dhost)) {
 					adapter->rcv_multicasts++;
 					netdev->stats.multicast++;
 					return true;

diff -u -p /var/linuxes/linux-next/drivers/staging/vt6655/wpactl.c /tmp/nothing/drivers/staging/vt6655/wpactl.c

--- /var/linuxes/linux-next/drivers/staging/vt6655/wpactl.c
+++ /tmp/nothing/drivers/staging/vt6655/wpactl.c

@@ -394,7 +394,6 @@ int wpa_set_keys(PSDevice pDevice, void
 
 		} else {
 			// Key Table Full
-			if (ether_addr_equal(param->addr, pDevice->abyBSSID)) {
 				//DBG_PRN_WLAN03(("return NDIS_STATUS_INVALID_DATA -Key Table Full.2\n"));
 				//spin_unlock_irq(&pDevice->lock);
 				return -EINVAL;

Hi Julia.

Maybe this explanation is helpful?

ethernet addresses are u8[6] (48 bits)

ether_addr_equal_64bits gets passed a pointer to u8[8]
and is more efficient on 64 bit architectures than
ether_addr_equal because the test can be done with a
single compare and shift.

The idea is not to access past the end of the ethernet
address as appropriate (think pointer to eeprom or other
such end-of-addressable memory conditions)

If a struct containing an ethernet address has additional
members after the ethernet address, or the u8[6] address
passed to ether_addr_equal is not going to access past
the end of memory or the structure, then
ether_addr_equal_64bits should be used in lieu of
ether_addr_equal.

After some work on getting more include files to be taken into account, I 
get the following list of cases where both arguments have the form of a 
field reference, the field has array type, and the field is not at the end 
of the structure.  I guess these are all candidates for 
ether_addr_equal_64bits (assuming that they correctly do satisfy these 
properties)?

Most of the uses of ether_addr_equal unfortunately take at least one 
pointer as an argument.  If the pointer is a parameter one has to check 
all of the calls sites.  If the function is static, this is perhaps not 
too bad, but from looking at some examples, it seems that this is often 
not the case.

There seem to also be quite a number of comparisons that use memcmp rather 
than ether_addr_equal.  It seems that using ether_addr_equal would require 
knowing about alignment.  In some cases, the structure field declaration 
explicitly mentions alignment.  I'm not sure how to be sure that it holds 
otherwise.  In particular, many of the relevant structures are declared as 
__packed.  Perhaps that is also an issue for using 
ether_addr_equal_64bits?

julia

From linux-next of 20131224.

file drivers/scsi/fcoe/fcoe_ctlr.c on line 1524
file drivers/scsi/fcoe/fcoe_ctlr.c on line 1528
file drivers/scsi/fcoe/fcoe_ctlr.c on line 342
file drivers/scsi/fcoe/fcoe_ctlr.c on line 1042
file drivers/scsi/fcoe/fcoe_sysfs.c on line 660
file drivers/scsi/fcoe/fcoe.c on line 1465
file drivers/net/wireless/rtlwifi/ps.c on line 926
file drivers/net/wireless/rtlwifi/ps.c on line 481
file drivers/net/wireless/rtlwifi/base.c on line 1296
file drivers/net/wireless/rtlwifi/base.c on line 1784
file drivers/net/wireless/p54/txrx.c on line 311
file drivers/net/wireless/rt2x00/rt2x00dev.c on line 568
file drivers/net/wireless/rt2x00/rt2x00dev.c on line 571
file drivers/net/wireless/ipw2x00/libipw_rx.c on line 1471
file drivers/net/wireless/ath/carl9170/rx.c on line 605
file drivers/net/wireless/ath/carl9170/rx.c on line 606
file drivers/net/wireless/ath/carl9170/rx.c on line 539
file drivers/net/wireless/ath/ath9k/htc_drv_txrx.c on line 1080
file drivers/net/wireless/ath/ath9k/recv.c on line 985
file drivers/net/wireless/ath/ath5k/base.c on line 1248
file drivers/net/wireless/ath/ath5k/base.c on line 1312
file drivers/net/wireless/iwlegacy/3945.c on line 472
file drivers/net/wireless/iwlegacy/3945.c on line 469
file drivers/net/wireless/iwlegacy/common.c on line 3749
file drivers/net/wireless/iwlegacy/common.c on line 3750
file drivers/net/wireless/iwlegacy/common.c on line 3751
file drivers/net/wireless/mwl8k.c on line 1261
file drivers/net/wireless/at76c50x-usb.c on line 1724
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 878
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 879
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 880
file drivers/net/wireless/iwlwifi/dvm/rx.c on line 783
file drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c on line 334
file drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c on line 580
file drivers/net/vxlan.c on line 1371
file drivers/staging/slicoss/slicoss.c on line 790
file drivers/staging/vt6656/wpactl.c on line 230
file drivers/staging/vt6656/dpc.c on line 357
file drivers/staging/vt6656/dpc.c on line 369
file net/bluetooth/bnep/core.c on line 407
file net/bluetooth/bnep/core.c on line 410
file net/wireless/scan.c on line 636
file net/wireless/mlme.c on line 561
file net/wireless/mlme.c on line 575
file net/wireless/mlme.c on line 588
file net/hsr/hsr_framereg.c on line 88
file net/hsr/hsr_framereg.c on line 187
file net/hsr/hsr_framereg.c on line 90
file net/hsr/hsr_framereg.c on line 178
file net/802/stp.c on line 49
file net/mac80211/tx.c on line 1693
file net/mac80211/mesh.c on line 1055
file net/mac80211/mesh.c on line 1005
file net/mac80211/scan.c on line 187
file net/mac80211/scan.c on line 188
file net/mac80211/mlme.c on line 2584
file net/mac80211/mlme.c on line 4036
file net/mac80211/mlme.c on line 3827
file net/mac80211/mlme.c on line 2784
file net/mac80211/mlme.c on line 2814
file net/mac80211/mlme.c on line 2217
file net/mac80211/mlme.c on line 2245
file net/mac80211/mlme.c on line 2708
file net/mac80211/mlme.c on line 2712
file net/mac80211/mlme.c on line 2695
file net/mac80211/iface.c on line 288
file net/mac80211/rx.c on line 1910
file net/mac80211/rx.c on line 1641
file net/mac80211/rx.c on line 1642
file net/mac80211/rx.c on line 3104
file net/mac80211/rx.c on line 2341
file net/mac80211/rx.c on line 2521
file net/mac80211/rx.c on line 2340
file net/mac80211/rx.c on line 2335
file net/mac80211/rx.c on line 3093
file net/mac80211/rx.c on line 3111
file net/mac80211/rx.c on line 3127
file net/mac80211/rx.c on line 3137
file net/mac80211/rx.c on line 3147
file net/mac80211/rx.c on line 3168
file net/mac80211/rx.c on line 3103
file net/mac80211/rx.c on line 2146
file net/mac80211/sta_info.c on line 416
file net/mac80211/status.c on line 595
file net/mac80211/ibss.c on line 1126
file net/mac80211/ibss.c on line 1018
file net/mac80211/ibss.c on line 1437

On Sun, 2013-12-29 at 23:05 +0100, Julia Lawall wrote:

After some work on getting more include files to be taken into account, I 
get the following list of cases where both arguments have the form of a 
field reference, the field has array type, and the field is not at the end 
of the structure.  I guess these are all candidates for 
ether_addr_equal_64bits (assuming that they correctly do satisfy these 
properties)?

I believe yes.

There seem to also be quite a number of comparisons that use memcmp rather 
than ether_addr_equal.

Ding's patch series converts all the memcmp(e1, e1, \(ETH_ALEN\|6\))
via spatch to either ether_addr_equal or ether_addr_equal_unaligned.

I believe he's inspecting the sites to determine if the
conversion should use the _unaligned version.

It seems that using ether_addr_equal would require 
knowing about alignment.  In some cases, the structure field declaration 
explicitly mentions alignment.  I'm not sure how to be sure that it holds 
otherwise.  In particular, many of the relevant structures are declared as 
__packed.  Perhaps that is also an issue for using 
ether_addr_equal_64bits?

No, not really, the packing really doesn't matter, just
whether or not there are 2 more addressable bytes after
the u8 array[6].

Thanks for the list Julia.

From linux-next of 20131224.

file drivers/scsi/fcoe/fcoe_ctlr.c on line 1524
file drivers/scsi/fcoe/fcoe_ctlr.c on line 1528
file drivers/scsi/fcoe/fcoe_ctlr.c on line 342
file drivers/scsi/fcoe/fcoe_ctlr.c on line 1042
file drivers/scsi/fcoe/fcoe_sysfs.c on line 660
file drivers/scsi/fcoe/fcoe.c on line 1465
file drivers/net/wireless/rtlwifi/ps.c on line 926
file drivers/net/wireless/rtlwifi/ps.c on line 481
file drivers/net/wireless/rtlwifi/base.c on line 1296
file drivers/net/wireless/rtlwifi/base.c on line 1784
file drivers/net/wireless/p54/txrx.c on line 311
file drivers/net/wireless/rt2x00/rt2x00dev.c on line 568
file drivers/net/wireless/rt2x00/rt2x00dev.c on line 571
file drivers/net/wireless/ipw2x00/libipw_rx.c on line 1471
file drivers/net/wireless/ath/carl9170/rx.c on line 605
file drivers/net/wireless/ath/carl9170/rx.c on line 606
file drivers/net/wireless/ath/carl9170/rx.c on line 539
file drivers/net/wireless/ath/ath9k/htc_drv_txrx.c on line 1080
file drivers/net/wireless/ath/ath9k/recv.c on line 985
file drivers/net/wireless/ath/ath5k/base.c on line 1248
file drivers/net/wireless/ath/ath5k/base.c on line 1312
file drivers/net/wireless/iwlegacy/3945.c on line 472
file drivers/net/wireless/iwlegacy/3945.c on line 469
file drivers/net/wireless/iwlegacy/common.c on line 3749
file drivers/net/wireless/iwlegacy/common.c on line 3750
file drivers/net/wireless/iwlegacy/common.c on line 3751
file drivers/net/wireless/mwl8k.c on line 1261
file drivers/net/wireless/at76c50x-usb.c on line 1724
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 878
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 879
file drivers/net/wireless/iwlwifi/dvm/rxon.c on line 880
file drivers/net/wireless/iwlwifi/dvm/rx.c on line 783
file drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c on line 334
file drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c on line 580
file drivers/net/vxlan.c on line 1371
file drivers/staging/slicoss/slicoss.c on line 790
file drivers/staging/vt6656/wpactl.c on line 230
file drivers/staging/vt6656/dpc.c on line 357
file drivers/staging/vt6656/dpc.c on line 369
file net/bluetooth/bnep/core.c on line 407
file net/bluetooth/bnep/core.c on line 410
file net/wireless/scan.c on line 636
file net/wireless/mlme.c on line 561
file net/wireless/mlme.c on line 575
file net/wireless/mlme.c on line 588
file net/hsr/hsr_framereg.c on line 88
file net/hsr/hsr_framereg.c on line 187
file net/hsr/hsr_framereg.c on line 90
file net/hsr/hsr_framereg.c on line 178
file net/802/stp.c on line 49
file net/mac80211/tx.c on line 1693
file net/mac80211/mesh.c on line 1055
file net/mac80211/mesh.c on line 1005
file net/mac80211/scan.c on line 187
file net/mac80211/scan.c on line 188
file net/mac80211/mlme.c on line 2584
file net/mac80211/mlme.c on line 4036
file net/mac80211/mlme.c on line 3827
file net/mac80211/mlme.c on line 2784
file net/mac80211/mlme.c on line 2814
file net/mac80211/mlme.c on line 2217
file net/mac80211/mlme.c on line 2245
file net/mac80211/mlme.c on line 2708
file net/mac80211/mlme.c on line 2712
file net/mac80211/mlme.c on line 2695
file net/mac80211/iface.c on line 288
file net/mac80211/rx.c on line 1910
file net/mac80211/rx.c on line 1641
file net/mac80211/rx.c on line 1642
file net/mac80211/rx.c on line 3104
file net/mac80211/rx.c on line 2341
file net/mac80211/rx.c on line 2521
file net/mac80211/rx.c on line 2340
file net/mac80211/rx.c on line 2335
file net/mac80211/rx.c on line 3093
file net/mac80211/rx.c on line 3111
file net/mac80211/rx.c on line 3127
file net/mac80211/rx.c on line 3137
file net/mac80211/rx.c on line 3147
file net/mac80211/rx.c on line 3168
file net/mac80211/rx.c on line 3103
file net/mac80211/rx.c on line 2146
file net/mac80211/sta_info.c on line 416
file net/mac80211/status.c on line 595
file net/mac80211/ibss.c on line 1126
file net/mac80211/ibss.c on line 1018
file net/mac80211/ibss.c on line 1437

On 2013/12/28 1:06, Joe Perches wrote:

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

As the below patch said, that a lot of ether_addr_equal
could be instead of ether_addr_equal_64bits, and I need to
review them and resend.

Regards
Ding 

On Sat, 2013-12-28 at 13:07 +0800, Ding Tianhong wrote:

On 2013/12/28 1:06, Joe Perches wrote:

quoted

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

As the below patch said, that a lot of ether_addr_equal
could be instead of ether_addr_equal_64bits, and I need to
review them and resend.

I don't think so.

I think what you've done so far is fine.

Any conversions to ether_addr_equal_64bit
can be done later.

On 2013/12/28 13:11, Joe Perches wrote:

On Sat, 2013-12-28 at 13:07 +0800, Ding Tianhong wrote:

quoted

On 2013/12/28 1:06, Joe Perches wrote:

quoted

On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote:

quoted

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

[]

quoted

quoted

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c

[]

quoted

quoted

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

That is better though I wonder how many systems are
still using laplink via parallel null-printer cables.

No matter, better is better.

The same optimization using ether_addr_equal_64bits
may be possible to do in other places given other
structs too.

Perhaps it's a possible spatch/coccinelle conversion,

I don't know spatch well enough to know if a
mechanism to check if structure members have other
fields that follow them in the structure or if the
structure member is an array of a minimum size.

Maybe Julia does.  (cc'd)

As the below patch said, that a lot of ether_addr_equal
could be instead of ether_addr_equal_64bits, and I need to
review them and resend.

I don't think so.

I think what you've done so far is fine.

Any conversions to ether_addr_equal_64bit
can be done later.

Ok, thanks.

Regards
Ding

On 2013/12/27 23:48, Eric Dumazet wrote:

On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote:

quoted

Use possibly more efficient ether_addr_equal
to instead of memcmp.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/plip/plip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c
index 7b4ff35..26614df 100644
--- a/drivers/net/plip/plip.c
+++ b/drivers/net/plip/plip.c

@@ -549,7 +549,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct net_device *dev)
 
 	if(*eth->h_dest&1)
 	{
-		if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
+		if(ether_addr_equal(eth->h_dest, dev->broadcast))
 			skb->pkt_type=PACKET_BROADCAST;
 		else
 			skb->pkt_type=PACKET_MULTICAST;

What about :

        if (is_multicast_ether_addr(eth->h_dest)) {
                if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
                        skb->pkt_type = PACKET_BROADCAST;
                else
                        skb->pkt_type = PACKET_MULTICAST;
        }

more better, thanks.

Regards
Ding