* [PATCH net-next 19/20] net: plip: slight optimization of addr compare @ 2013-12-27 6:49 Ding Tianhong 2013-12-27 15:48 ` Eric Dumazet 0 siblings, 1 reply; 14+ messages in thread From: Ding Tianhong @ 2013-12-27 6:49 UTC (permalink / raw) To: David S. Miller, Netdev, linux-kernel@vger.kernel.org Use possibly more efficient ether_addr_equal to instead of memcmp. Cc: "David S. Miller" <davem@davemloft.net> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 6:49 [PATCH net-next 19/20] net: plip: slight optimization of addr compare Ding Tianhong @ 2013-12-27 15:48 ` Eric Dumazet 2013-12-27 17:06 ` Joe Perches 2013-12-28 5:05 ` Ding Tianhong 0 siblings, 2 replies; 14+ messages in thread From: Eric Dumazet @ 2013-12-27 15:48 UTC (permalink / raw) To: Ding Tianhong; +Cc: David S. Miller, Netdev, linux-kernel@vger.kernel.org On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > Use possibly more efficient ether_addr_equal > to instead of memcmp. > > Cc: "David S. Miller" <davem@davemloft.net> > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > 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; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 15:48 ` Eric Dumazet @ 2013-12-27 17:06 ` Joe Perches 2013-12-27 20:12 ` Julia Lawall 2013-12-28 5:07 ` Ding Tianhong 2013-12-28 5:05 ` Ding Tianhong 1 sibling, 2 replies; 14+ messages in thread From: Joe Perches @ 2013-12-27 17:06 UTC (permalink / raw) To: Eric Dumazet, Julia Lawall Cc: Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > > Use possibly more efficient ether_addr_equal > > to instead of memcmp. [] > > diff --git 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; > } 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 17:06 ` Joe Perches @ 2013-12-27 20:12 ` Julia Lawall 2013-12-27 20:46 ` Joe Perches 2013-12-28 5:07 ` Ding Tianhong 1 sibling, 1 reply; 14+ messages in thread From: Julia Lawall @ 2013-12-27 20:12 UTC (permalink / raw) To: Joe Perches Cc: Eric Dumazet, Julia Lawall, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org On Fri, 27 Dec 2013, Joe Perches wrote: > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > > > Use possibly more efficient ether_addr_equal > > > to instead of memcmp. > [] > > > diff --git 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; > > } > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 20:12 ` Julia Lawall @ 2013-12-27 20:46 ` Joe Perches 2013-12-27 21:56 ` [PATCH] i40e: use ether_addr_equal_64bits Joe Perches ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Joe Perches @ 2013-12-27 20:46 UTC (permalink / raw) To: Julia Lawall Cc: Eric Dumazet, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org On Fri, 2013-12-27 at 21:12 +0100, Julia Lawall wrote: > On Fri, 27 Dec 2013, Joe Perches wrote: > > > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > > > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > > > > Use possibly more efficient ether_addr_equal > > > > to instead of memcmp. > > [] > > > > diff --git 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; > > > } > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] i40e: use ether_addr_equal_64bits 2013-12-27 20:46 ` Joe Perches @ 2013-12-27 21:56 ` Joe Perches 2013-12-27 21:59 ` Jeff Kirsher 2013-12-27 22:47 ` [PATCH net-next 19/20] net: plip: slight optimization of addr compare Julia Lawall 2013-12-29 22:05 ` Julia Lawall 2 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2013-12-27 21:56 UTC (permalink / raw) To: Jeff Kirsher, Jesse Brandeburg Cc: Eric Dumazet, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org, e1000-devel, Julia Lawall 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: > > On Fri, 27 Dec 2013, Joe Perches wrote: > > > > > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > > > > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > > > > > Use possibly more efficient ether_addr_equal > > > > > to instead of memcmp. > > > [] > > > > > diff --git 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; > > > > } > > > > > > 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++; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] i40e: use ether_addr_equal_64bits 2013-12-27 21:56 ` [PATCH] i40e: use ether_addr_equal_64bits Joe Perches @ 2013-12-27 21:59 ` Jeff Kirsher 0 siblings, 0 replies; 14+ messages in thread From: Jeff Kirsher @ 2013-12-27 21:59 UTC (permalink / raw) To: Joe Perches Cc: e1000-devel, Netdev, linux-kernel@vger.kernel.org, Jesse Brandeburg, Julia Lawall, Ding Tianhong, David S. Miller [-- Attachment #1.1: Type: text/plain, Size: 6652 bytes --] 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. > --- > > On Fri, 2013-12-27 at 12:46 -0800, Joe Perches wrote: > > On Fri, 2013-12-27 at 21:12 +0100, Julia Lawall wrote: > > > On Fri, 27 Dec 2013, Joe Perches wrote: > > > > > > > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > > > > > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > > > > > > Use possibly more efficient ether_addr_equal > > > > > > to instead of memcmp. > > > > [] > > > > > > diff --git 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; > > > > > } > > > > > > > > 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++; > } > > > [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 455 bytes --] ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk [-- Attachment #3: Type: text/plain, Size: 257 bytes --] _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 20:46 ` Joe Perches 2013-12-27 21:56 ` [PATCH] i40e: use ether_addr_equal_64bits Joe Perches @ 2013-12-27 22:47 ` Julia Lawall 2013-12-29 22:05 ` Julia Lawall 2 siblings, 0 replies; 14+ messages in thread From: Julia Lawall @ 2013-12-27 22:47 UTC (permalink / raw) To: Joe Perches Cc: Julia Lawall, Eric Dumazet, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org > 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 20:46 ` Joe Perches 2013-12-27 21:56 ` [PATCH] i40e: use ether_addr_equal_64bits Joe Perches 2013-12-27 22:47 ` [PATCH net-next 19/20] net: plip: slight optimization of addr compare Julia Lawall @ 2013-12-29 22:05 ` Julia Lawall 2013-12-29 22:28 ` Joe Perches 2 siblings, 1 reply; 14+ messages in thread From: Julia Lawall @ 2013-12-29 22:05 UTC (permalink / raw) To: Joe Perches Cc: Julia Lawall, Eric Dumazet, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-29 22:05 ` Julia Lawall @ 2013-12-29 22:28 ` Joe Perches 0 siblings, 0 replies; 14+ messages in thread From: Joe Perches @ 2013-12-29 22:28 UTC (permalink / raw) To: Julia Lawall Cc: Eric Dumazet, Ding Tianhong, David S. Miller, Netdev, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 17:06 ` Joe Perches 2013-12-27 20:12 ` Julia Lawall @ 2013-12-28 5:07 ` Ding Tianhong 2013-12-28 5:11 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: Ding Tianhong @ 2013-12-28 5:07 UTC (permalink / raw) To: Joe Perches, Eric Dumazet, Julia Lawall Cc: David S. Miller, Netdev, linux-kernel@vger.kernel.org On 2013/12/28 1:06, Joe Perches wrote: > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: >> On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: >>> Use possibly more efficient ether_addr_equal >>> to instead of memcmp. > [] >>> diff --git 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; >> } > > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-28 5:07 ` Ding Tianhong @ 2013-12-28 5:11 ` Joe Perches 2013-12-28 5:35 ` Ding Tianhong 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2013-12-28 5:11 UTC (permalink / raw) To: Ding Tianhong Cc: Eric Dumazet, Julia Lawall, David S. Miller, Netdev, linux-kernel@vger.kernel.org On Sat, 2013-12-28 at 13:07 +0800, Ding Tianhong wrote: > On 2013/12/28 1:06, Joe Perches wrote: > > On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: > >> On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: > >>> Use possibly more efficient ether_addr_equal > >>> to instead of memcmp. > > [] > >>> diff --git 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; > >> } > > > > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-28 5:11 ` Joe Perches @ 2013-12-28 5:35 ` Ding Tianhong 0 siblings, 0 replies; 14+ messages in thread From: Ding Tianhong @ 2013-12-28 5:35 UTC (permalink / raw) To: Joe Perches Cc: Eric Dumazet, Julia Lawall, David S. Miller, Netdev, linux-kernel@vger.kernel.org On 2013/12/28 13:11, Joe Perches wrote: > On Sat, 2013-12-28 at 13:07 +0800, Ding Tianhong wrote: >> On 2013/12/28 1:06, Joe Perches wrote: >>> On Fri, 2013-12-27 at 07:48 -0800, Eric Dumazet wrote: >>>> On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: >>>>> Use possibly more efficient ether_addr_equal >>>>> to instead of memcmp. >>> [] >>>>> diff --git 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; >>>> } >>> >>> 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 19/20] net: plip: slight optimization of addr compare 2013-12-27 15:48 ` Eric Dumazet 2013-12-27 17:06 ` Joe Perches @ 2013-12-28 5:05 ` Ding Tianhong 1 sibling, 0 replies; 14+ messages in thread From: Ding Tianhong @ 2013-12-28 5:05 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S. Miller, Netdev, linux-kernel@vger.kernel.org On 2013/12/27 23:48, Eric Dumazet wrote: > On Fri, 2013-12-27 at 14:49 +0800, Ding Tianhong wrote: >> Use possibly more efficient ether_addr_equal >> to instead of memcmp. >> >> Cc: "David S. Miller" <davem@davemloft.net> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-12-29 22:28 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-27 6:49 [PATCH net-next 19/20] net: plip: slight optimization of addr compare Ding Tianhong 2013-12-27 15:48 ` Eric Dumazet 2013-12-27 17:06 ` Joe Perches 2013-12-27 20:12 ` Julia Lawall 2013-12-27 20:46 ` Joe Perches 2013-12-27 21:56 ` [PATCH] i40e: use ether_addr_equal_64bits Joe Perches 2013-12-27 21:59 ` Jeff Kirsher 2013-12-27 22:47 ` [PATCH net-next 19/20] net: plip: slight optimization of addr compare Julia Lawall 2013-12-29 22:05 ` Julia Lawall 2013-12-29 22:28 ` Joe Perches 2013-12-28 5:07 ` Ding Tianhong 2013-12-28 5:11 ` Joe Perches 2013-12-28 5:35 ` Ding Tianhong 2013-12-28 5:05 ` Ding Tianhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).