Re: [PATCH] i40e: use ether_addr_equal_64bits
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
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, >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, >mac_filter_list, list) {
> - if ((ether_addr_equal(macaddr, f->macaddr)) &&
> + if
Re: [PATCH] i40e: use ether_addr_equal_64bits
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 j...@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_dest1)
{
- 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);
-
