Skip to contentSkip to conversation

slight optimization of addr compare for some modules

PATCH In review Ding Tianhong 3 participants 12 messages Dec 16, 2013 – Dec 19, 2013

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Ding Tianhong

author
#0 lore raw

v2 cover slight optimization of addr compare for some modules

Joe Perches add ether_addr_equal_unaligned to test if possibly unaligned to u16 Ethernet addresses are equal.

If CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set, this uses the slightly faster generic routine ether_addr_equal, otherwise this uses memcmp.

So I replace memcmp with ether_addr_equal_unaligned in some place for slight optimization.

v1->v2: modify the patch 02/06's indentation.

Regards.
Ding

Ding Tianhong (6):
bonding: use ether_addr_equal_unaligned for bond addr compare
mac80211: slight optimization of addr compare
fddi: slight optimization of addr compare
isdn: slight optimization of addr compare
pppoe: slight optimization of addr compare
plip: slight optimization of addr compare

 drivers/isdn/i4l/isdn_net.c    |  4 +--
 drivers/net/bonding/bond_3ad.c | 62 ++++++++++++++++++++++--------------------
 drivers/net/plip/plip.c        |  2 +-
 drivers/net/ppp/pppoe.c        |  5 ++--
 net/802/fddi.c                 |  5 ++--
 net/mac80211/iface.c           |  8 +++---
 6 files changed, 46 insertions(+), 40 deletions(-)
Signature · 1 line
1.8.0
#1 lore raw
Quoted 9 lines · Ding Tianhong

Joe Perches add ether_addr_equal_unaligned to test if possibly unaligned to u16 Ethernet addresses are equal.

If CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set, this uses the slightly faster generic routine ether_addr_equal, otherwise this uses memcmp.

So I replace memcmp with ether_addr_equal_unaligned in some place for slight optimization.

Hi.

These seem like sensible cleanups, thanks, but
my name doesn't need to go into the commit log
multiple times like this.

I suggest something like:

Use the recently added and possibly more efficient ether_addr_equal_unaligned.

Are you intending to do more of these?

$ git grep -E "\bmemcmp\s*\([^,]*,[^,]*,\s*(ETH_ALEN|6)\s*\)" * | wc -l
299

Perhaps the majority of these should use ether_addr_equal or ether_addr_equal_unaligned.

#2 lore raw
Quoted 22 lines · Joe Perches
Quoted 9 lines · Ding Tianhong

Joe Perches add ether_addr_equal_unaligned to test if possibly unaligned to u16 Ethernet addresses are equal.

If CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set, this uses the slightly faster generic routine ether_addr_equal, otherwise this uses memcmp.

So I replace memcmp with ether_addr_equal_unaligned in some place for slight optimization.

Hi.

These seem like sensible cleanups, thanks, but
my name doesn't need to go into the commit log
multiple times like this.

I suggest something like:

Use the recently added and possibly more efficient ether_addr_equal_unaligned.

Quoted 9 lines

Are you intending to do more of these?

$ git grep -E "\bmemcmp\s*\([^,]*,[^,]*,\s*(ETH_ALEN|6)\s*\)" * | wc -l
299

Perhaps the majority of these should use ether_addr_equal or ether_addr_equal_unaligned.

yes, it is a juge work to review the whole places and I think it should be finished by several times, maybe start from this patchset.

I will modify the changelog and simplify it. Thanks.

Regards
Ding

Quoted 5 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

#3 lore raw
Quoted 11 lines · Ding Tianhong
Quoted 7 lines · Joe Perches

Are you intending to do more of these?

$ git grep -E "\bmemcmp\s*\([^,]*,[^,]*,\s*(ETH_ALEN|6)\s*\)" * | wc -l
299

Perhaps the majority of these should use ether_addr_equal or ether_addr_equal_unaligned.

yes, it is a juge work to review the whole places and I think it should be finished by several times, maybe start from this patchset.

coccinelle (aka: spatch) can help find and change these

$ cat ether_addr_equal_unaligned.cocci
@@
expression e1;
expression e2;
@@

- !memcmp(e1, e2, 6)
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, 6) == 0
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, 6)
+ !ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, 6) != 0
+ !ether_addr_equal_unaligned(e1, e2)

$ spatch -sp-file ether_addr_equal.cocci drivers/media/dvb-core/dvb_net.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: drivers/media/dvb-core/dvb_net.c
diff = 
--- drivers/media/dvb-core/dvb_net.c
+++ /tmp/cocci-output-18744-96d16c-dvb_net.c
@@ -837,7 +837,8 @@ static void dvb_net_sec(struct net_devic
 	}
 	if (pkt[5] & 0x02) {
 		/* handle LLC/SNAP, see rfc-1042 */
-		if (pkt_len < 24 || memcmp(&pkt[12], "\xaa\xaa\x03\0\0\0", 6)) {
+		if (pkt_len < 24 || !ether_addr_equal_unaligned(&pkt[12],
+								"\xaa\xaa\x03\0\0\0")) {
 			stats->rx_dropped++;
 			return;
 		}

I presume many of these should be ether_addr_equal and not ether_addr_equal_unaligned, but I can't think of a way to automate that easily.

#4 lore raw

Hi again Ding.

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

- !memcmp(e1, e2, \(6\|ETH_ALEN\))
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\)) == 0
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\))
+ !ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+ !ether_addr_equal_unaligned(e1, e2)

author
#5 lore raw
Quoted 39 lines · Joe Perches

Hi again Ding.

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@
(quoted diff) +1 -1
-	!memcmp(e1, e2, \(6\|ETH_ALEN\))
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) == 0
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\))
+	!ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

Review

Quoted 2 lines
(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+	!ether_addr_equal_unaligned(e1, e2)

great, thanks, I 'll try and test.

Regards
ding

Quoted 1 line
author
#6 lore raw
Quoted 45 lines · Ding Tianhong
Quoted 39 lines · Joe Perches

Hi again Ding.

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@
(quoted diff) +1 -1
-	!memcmp(e1, e2, \(6\|ETH_ALEN\))
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) == 0
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\))
+	!ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

Review

Quoted 2 lines
(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+	!ether_addr_equal_unaligned(e1, e2)

great, thanks, I 'll try and test.

Quoted 3 lines

Regards
ding

Hi Joe:

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Regards
Ding

Quoted 2 lines
Quoted 1 line
#7 lore raw
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Joe Perches

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

[]

Quoted 2 lines

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Separate per-maintainer patches are generally good. It can take several attempts to get these applied in all the various trees.

So maybe 1 patch for each of most of these. Maybe some of these like drivers/media, drivers/mtd and drivers/staging could probably be single patches.

$ git grep --name-only -E "\bmemcmp\s*\([^,]+,[^,]+,\s*(ETH_ALEN|6)\s*\)"| \
  sed -r 's@\w+.[ch]$@@' | sort | uniq
arch/x86/kernel/apic/
block/partitions/
drivers/atm/
drivers/infiniband/hw/mlx4/
drivers/infiniband/ulp/ipoib/
drivers/isdn/i4l/
drivers/media/dvb-core/
drivers/media/usb/gspca/
drivers/mtd/
drivers/mtd/nand/
drivers/net/bonding/
drivers/net/ethernet/amd/
drivers/net/ethernet/atheros/atlx/
drivers/net/ethernet/broadcom/bnx2x/
drivers/net/ethernet/chelsio/cxgb3/
drivers/net/ethernet/cisco/enic/
drivers/net/ethernet/emulex/benet/
drivers/net/ethernet/freescale/
drivers/net/ethernet/intel/igbvf/
drivers/net/ethernet/mellanox/mlx4/
drivers/net/ethernet/micrel/
drivers/net/ethernet/neterion/vxge/vxge-
drivers/net/ethernet/qlogic/netxen/
drivers/net/ethernet/qlogic/qlcnic/
drivers/net/ethernet/renesas/
drivers/net/ethernet/seeq/
drivers/net/ethernet/sun/
drivers/net/ethernet/ti/
drivers/net/fddi/skfp/
drivers/net/hamradio/
drivers/net/plip/
drivers/net/ppp/
drivers/net/wireless/
drivers/net/wireless/ath/ath10k/
drivers/net/wireless/ath/ath6kl/
drivers/net/wireless/ath/wcn36xx/
drivers/net/wireless/ath/wil6210/
drivers/net/wireless/brcm80211/brcmfmac/
drivers/net/wireless/cw1200/
drivers/net/wireless/hostap/
drivers/net/wireless/ipw2x00/
drivers/net/wireless/libertas/
drivers/net/wireless/mwifiex/
drivers/net/wireless/orinoco/
drivers/net/wireless/prism54/
drivers/net/wireless/rtlwifi/
drivers/net/wireless/ti/wl1251/
drivers/net/wireless/zd1211rw/
drivers/s390/net/
drivers/scsi/sym53c8xx_2/
drivers/staging/ozwpan/
drivers/staging/rtl8187se/
drivers/staging/rtl8187se/ieee80211/
drivers/staging/rtl8188eu/core/
drivers/staging/rtl8188eu/os_dep/
drivers/staging/rtl8192e/
drivers/staging/rtl8192u/ieee80211/
drivers/staging/rtl8712/
drivers/staging/vt6655/
drivers/staging/vt6656/
drivers/staging/wlan-ng/
drivers/target/iscsi/
drivers/usb/atm/
drivers/usb/storage/
drivers/video/matrox/
fs/sysv/
include/linux/
init/
net/batman-adv/
net/batman-adv/translation-
net/bluetooth/bnep/
net/bluetooth/hidp/
net/bridge/
net/caif/
net/mac80211/
net/sunrpc/auth_gss/
scripts/
author
#8 lore raw
Quoted 95 lines · Joe Perches
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Joe Perches

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

[]

Quoted 2 lines

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Separate per-maintainer patches are generally good. It can take several attempts to get these applied in all the various trees.

So maybe 1 patch for each of most of these. Maybe some of these like drivers/media, drivers/mtd and drivers/staging could probably be single patches.

$ git grep --name-only -E "\bmemcmp\s*\([^,]+,[^,]+,\s*(ETH_ALEN|6)\s*\)"| \
  sed -r 's@\w+.[ch]$@@' | sort | uniq
arch/x86/kernel/apic/
block/partitions/
drivers/atm/
drivers/infiniband/hw/mlx4/
drivers/infiniband/ulp/ipoib/
drivers/isdn/i4l/
drivers/media/dvb-core/
drivers/media/usb/gspca/
drivers/mtd/
drivers/mtd/nand/
drivers/net/bonding/
drivers/net/ethernet/amd/
drivers/net/ethernet/atheros/atlx/
drivers/net/ethernet/broadcom/bnx2x/
drivers/net/ethernet/chelsio/cxgb3/
drivers/net/ethernet/cisco/enic/
drivers/net/ethernet/emulex/benet/
drivers/net/ethernet/freescale/
drivers/net/ethernet/intel/igbvf/
drivers/net/ethernet/mellanox/mlx4/
drivers/net/ethernet/micrel/
drivers/net/ethernet/neterion/vxge/vxge-
drivers/net/ethernet/qlogic/netxen/
drivers/net/ethernet/qlogic/qlcnic/
drivers/net/ethernet/renesas/
drivers/net/ethernet/seeq/
drivers/net/ethernet/sun/
drivers/net/ethernet/ti/
drivers/net/fddi/skfp/
drivers/net/hamradio/
drivers/net/plip/
drivers/net/ppp/
drivers/net/wireless/
drivers/net/wireless/ath/ath10k/
drivers/net/wireless/ath/ath6kl/
drivers/net/wireless/ath/wcn36xx/
drivers/net/wireless/ath/wil6210/
drivers/net/wireless/brcm80211/brcmfmac/
drivers/net/wireless/cw1200/
drivers/net/wireless/hostap/
drivers/net/wireless/ipw2x00/
drivers/net/wireless/libertas/
drivers/net/wireless/mwifiex/
drivers/net/wireless/orinoco/
drivers/net/wireless/prism54/
drivers/net/wireless/rtlwifi/
drivers/net/wireless/ti/wl1251/
drivers/net/wireless/zd1211rw/
drivers/s390/net/
drivers/scsi/sym53c8xx_2/
drivers/staging/ozwpan/
drivers/staging/rtl8187se/
drivers/staging/rtl8187se/ieee80211/
drivers/staging/rtl8188eu/core/
drivers/staging/rtl8188eu/os_dep/
drivers/staging/rtl8192e/
drivers/staging/rtl8192u/ieee80211/
drivers/staging/rtl8712/
drivers/staging/vt6655/
drivers/staging/vt6656/
drivers/staging/wlan-ng/
drivers/target/iscsi/
drivers/usb/atm/
drivers/usb/storage/
drivers/video/matrox/
fs/sysv/
include/linux/
init/
net/batman-adv/
Quoted 10 lines
net/batman-adv/translation-
net/bluetooth/bnep/
net/bluetooth/hidp/
net/bridge/
net/caif/
net/mac80211/
net/sunrpc/auth_gss/
scripts/

great, thanks, my team could make it together.

Regards
Ding

Quoted 2 lines

.

author
#9 lore raw
In reply to Joe Perches
Quoted 16 lines · Joe Perches
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Joe Perches

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

[]

Quoted 2 lines

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Quoted 8 lines

Separate per-maintainer patches are generally good. It can take several attempts to get these applied in all the various trees.

So maybe 1 patch for each of most of these. Maybe some of these like drivers/media, drivers/mtd and drivers/staging could probably be single patches.

Hi Joe:

I found there is a bug in spatch, it could not deal with
- memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+ !ether_addr_equal_unaligned(e1, e2)

caif/cfrfml.c +1 -1
diff -u -p a/caif/cfrfml.c b/caif/cfrfml.c
@@ -79,7 +79,7 @@ static struct cfpkt *rfm_append(struct c
                return NULL;
 
        /* Verify correct header */
-       if (memcmp(seghead, rfml->seghead, 6) != 0)
+       if (!ether_addr_equal_unaligned(seghead, rfml->seghead) != 0)
                return NULL;
 
        tmppkt = cfpkt_append(rfml->incomplete_frm, pkt,

Regards
Ding

Quoted 1 line
#10 lore raw
Quoted 29 lines · Ding Tianhong
Quoted 16 lines · Joe Perches
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Joe Perches

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

[]

Quoted 2 lines

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Quoted 8 lines

Separate per-maintainer patches are generally good. It can take several attempts to get these applied in all the various trees.

So maybe 1 patch for each of most of these. Maybe some of these like drivers/media, drivers/mtd and drivers/staging could probably be single patches.

Hi Joe:

I found there is a bug in spatch, it could not deal with

Review

Quoted 2 lines
(quoted diff) +1 -1
-       memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+       !ether_addr_equal_unaligned(e1, e2)

Not an spatch bug but a defect in the ordering of transforms in the ether_addr_equal_unaligned.cocci file

This should be better:

$ cat ether_addr_equal_unaligned.cocci
@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\)) == 0
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+ !ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- !memcmp(e1, e2, \(6\|ETH_ALEN\))
+ ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

- memcmp(e1, e2, \(6\|ETH_ALEN\))
+ !ether_addr_equal_unaligned(e1, e2)

author
#11 lore raw
Quoted 71 lines · Joe Perches
Quoted 29 lines · Ding Tianhong
Quoted 16 lines · Joe Perches
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Joe Perches

These should still be inspected for appropriate use of ether_addr_equal or ether_addr_equal_unaligned, but a better cocci input sp-file is:

$ cat ether_addr_equal_unaligned.cocci 
@@
expression e1;
expression e2;
@@

[]

Quoted 2 lines

There are too many places need to be changed, should I make it in one patch or several pathset, pls give me some advise. thanks

Quoted 8 lines

Separate per-maintainer patches are generally good. It can take several attempts to get these applied in all the various trees.

So maybe 1 patch for each of most of these. Maybe some of these like drivers/media, drivers/mtd and drivers/staging could probably be single patches.

Hi Joe:

I found there is a bug in spatch, it could not deal with

Review

Quoted 2 lines
(quoted diff) +1 -1
-       memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+       !ether_addr_equal_unaligned(e1, e2)

Not an spatch bug but a defect in the ordering of transforms in the ether_addr_equal_unaligned.cocci file

This should be better:

$ cat ether_addr_equal_unaligned.cocci
@@
expression e1;
expression e2;
@@
(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) == 0
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\)) != 0
+	!ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

(quoted diff) +1 -1
-	!memcmp(e1, e2, \(6\|ETH_ALEN\))
+	ether_addr_equal_unaligned(e1, e2)

@@
expression e1;
expression e2;
@@

Review

Quoted 2 lines
(quoted diff) +1 -1
-	memcmp(e1, e2, \(6\|ETH_ALEN\))
+	!ether_addr_equal_unaligned(e1, e2)

Yes, it could works well, thanks a lot.

Regards
Ding

Quoted 6 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