Re: [PATCH net-next 2/4] bonding: use common mac addr checks
Banerjee, Debabrata wrote:
>> From: Jay Vosburgh [mailto:jay.vosbu...@canonical.com]
>> Debabrata Banerjee wrote:
>
>> >- if
>> (!ether_addr_equal_64bits(rx_hash_table[index].mac_dst,
>> >-mac_bcast) &&
>> >-
>> !is_zero_ether_addr(rx_hash_table[index].mac_dst)) {
>> >+ if
>> (is_valid_ether_addr(rx_hash_table[index].mac_dst)) {
>>
>> This change and the similar ones below will now fail non-broadcast
>> multicast Ethernet addresses, where the prior code would not. Is this an
>> intentional change?
>
>Yes I don't see how it makes sense to use multicast addresses at all, but I
>may be missing something. It's also illegal according to rfc1812 3.3.2, but
>obviously this balancing mode is trying to be very clever. We probably
>shouldn't violate the rfc anyway.
Fair enough, but I think it would be good to call this out in
the change log just in case it does somehow cause a regression.
-J
---
-Jay Vosburgh, jay.vosbu...@canonical.com
RE: [PATCH net-next 2/4] bonding: use common mac addr checks
> From: Jay Vosburgh [mailto:jay.vosbu...@canonical.com]
> Debabrata Banerjee wrote:
> >-if
> (!ether_addr_equal_64bits(rx_hash_table[index].mac_dst,
> >- mac_bcast) &&
> >-
> !is_zero_ether_addr(rx_hash_table[index].mac_dst)) {
> >+if
> (is_valid_ether_addr(rx_hash_table[index].mac_dst)) {
>
> This change and the similar ones below will now fail non-broadcast
> multicast Ethernet addresses, where the prior code would not. Is this an
> intentional change?
Yes I don't see how it makes sense to use multicast addresses at all, but I may
be missing something. It's also illegal according to rfc1812 3.3.2, but
obviously this balancing mode is trying to be very clever. We probably
shouldn't violate the rfc anyway.
Re: [PATCH net-next 2/4] bonding: use common mac addr checks
Debabrata Banerjee wrote:
>Replace homegrown mac addr checks with faster defs from etherdevice.h
>
>Signed-off-by: Debabrata Banerjee
>---
> drivers/net/bonding/bond_alb.c | 28 +---
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index c2f6c58e4e6a..180e50f7806f 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -40,11 +40,6 @@
> #include
> #include
>
>-
>-
>-static const u8 mac_bcast[ETH_ALEN + 2] __long_aligned = {
>- 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>-};
> static const u8 mac_v6_allmcast[ETH_ALEN + 2] __long_aligned = {
> 0x33, 0x33, 0x00, 0x00, 0x00, 0x01
> };
>@@ -420,9 +415,7 @@ static void rlb_clear_slave(struct bonding *bond, struct
>slave *slave)
>
> if (assigned_slave) {
> rx_hash_table[index].slave = assigned_slave;
>- if
>(!ether_addr_equal_64bits(rx_hash_table[index].mac_dst,
>- mac_bcast) &&
>-
>!is_zero_ether_addr(rx_hash_table[index].mac_dst)) {
>+ if
>(is_valid_ether_addr(rx_hash_table[index].mac_dst)) {
This change and the similar ones below will now fail
non-broadcast multicast Ethernet addresses, where the prior code would
not. Is this an intentional change?
-J
> bond_info->rx_hashtbl[index].ntt = 1;
> bond_info->rx_ntt = 1;
> /* A slave has been removed from the
>@@ -525,8 +518,7 @@ static void rlb_req_update_slave_clients(struct bonding
>*bond, struct slave *sla
> client_info = &(bond_info->rx_hashtbl[hash_index]);
>
> if ((client_info->slave == slave) &&
>- !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>- !is_zero_ether_addr(client_info->mac_dst)) {
>+ is_valid_ether_addr(client_info->mac_dst)) {
> client_info->ntt = 1;
> ntt = 1;
> }
>@@ -567,8 +559,7 @@ static void rlb_req_update_subnet_clients(struct bonding
>*bond, __be32 src_ip)
> if ((client_info->ip_src == src_ip) &&
> !ether_addr_equal_64bits(client_info->slave->dev->dev_addr,
>bond->dev->dev_addr) &&
>- !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>- !is_zero_ether_addr(client_info->mac_dst)) {
>+ is_valid_ether_addr(client_info->mac_dst)) {
> client_info->ntt = 1;
> bond_info->rx_ntt = 1;
> }
>@@ -596,7 +587,7 @@ static struct slave *rlb_choose_channel(struct sk_buff
>*skb, struct bonding *bon
> if ((client_info->ip_src == arp->ip_src) &&
> (client_info->ip_dst == arp->ip_dst)) {
> /* the entry is already assigned to this client */
>- if (!ether_addr_equal_64bits(arp->mac_dst, mac_bcast)) {
>+ if (!is_broadcast_ether_addr(arp->mac_dst)) {
> /* update mac address from arp */
> ether_addr_copy(client_info->mac_dst,
> arp->mac_dst);
> }
>@@ -644,8 +635,7 @@ static struct slave *rlb_choose_channel(struct sk_buff
>*skb, struct bonding *bon
> ether_addr_copy(client_info->mac_src, arp->mac_src);
> client_info->slave = assigned_slave;
>
>- if (!ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>- !is_zero_ether_addr(client_info->mac_dst)) {
>+ if (is_valid_ether_addr(client_info->mac_dst)) {
> client_info->ntt = 1;
> bond->alb_info.rx_ntt = 1;
> } else {
>@@ -1418,9 +1408,9 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device
>*bond_dev)
> case ETH_P_IP: {
> const struct iphdr *iph = ip_hdr(skb);
>
>- if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast) ||
>- (iph->daddr == ip_bcast) ||
>- (iph->protocol == IPPROTO_IGMP)) {
>+ if (is_broadcast_ether_addr(eth_data->h_dest) ||
>+ iph->daddr == ip_bcast ||
>+ iph->protocol == IPPROTO_IGMP) {
> do_tx_balance = false;
> break;
> }
>@@ -1432,7 +1422,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device
>*bond_dev)
> /* IPv6 doesn't really use broadcast mac address, but leave
>* that here just in case.
>*/
>- if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast)) {
>+
[PATCH net-next 2/4] bonding: use common mac addr checks
Replace homegrown mac addr checks with faster defs from etherdevice.h
Signed-off-by: Debabrata Banerjee
---
drivers/net/bonding/bond_alb.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c2f6c58e4e6a..180e50f7806f 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -40,11 +40,6 @@
#include
#include
-
-
-static const u8 mac_bcast[ETH_ALEN + 2] __long_aligned = {
- 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
-};
static const u8 mac_v6_allmcast[ETH_ALEN + 2] __long_aligned = {
0x33, 0x33, 0x00, 0x00, 0x00, 0x01
};
@@ -420,9 +415,7 @@ static void rlb_clear_slave(struct bonding *bond, struct
slave *slave)
if (assigned_slave) {
rx_hash_table[index].slave = assigned_slave;
- if
(!ether_addr_equal_64bits(rx_hash_table[index].mac_dst,
-mac_bcast) &&
-
!is_zero_ether_addr(rx_hash_table[index].mac_dst)) {
+ if
(is_valid_ether_addr(rx_hash_table[index].mac_dst)) {
bond_info->rx_hashtbl[index].ntt = 1;
bond_info->rx_ntt = 1;
/* A slave has been removed from the
@@ -525,8 +518,7 @@ static void rlb_req_update_slave_clients(struct bonding
*bond, struct slave *sla
client_info = &(bond_info->rx_hashtbl[hash_index]);
if ((client_info->slave == slave) &&
- !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
- !is_zero_ether_addr(client_info->mac_dst)) {
+ is_valid_ether_addr(client_info->mac_dst)) {
client_info->ntt = 1;
ntt = 1;
}
@@ -567,8 +559,7 @@ static void rlb_req_update_subnet_clients(struct bonding
*bond, __be32 src_ip)
if ((client_info->ip_src == src_ip) &&
!ether_addr_equal_64bits(client_info->slave->dev->dev_addr,
bond->dev->dev_addr) &&
- !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
- !is_zero_ether_addr(client_info->mac_dst)) {
+ is_valid_ether_addr(client_info->mac_dst)) {
client_info->ntt = 1;
bond_info->rx_ntt = 1;
}
@@ -596,7 +587,7 @@ static struct slave *rlb_choose_channel(struct sk_buff
*skb, struct bonding *bon
if ((client_info->ip_src == arp->ip_src) &&
(client_info->ip_dst == arp->ip_dst)) {
/* the entry is already assigned to this client */
- if (!ether_addr_equal_64bits(arp->mac_dst, mac_bcast)) {
+ if (!is_broadcast_ether_addr(arp->mac_dst)) {
/* update mac address from arp */
ether_addr_copy(client_info->mac_dst,
arp->mac_dst);
}
@@ -644,8 +635,7 @@ static struct slave *rlb_choose_channel(struct sk_buff
*skb, struct bonding *bon
ether_addr_copy(client_info->mac_src, arp->mac_src);
client_info->slave = assigned_slave;
- if (!ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
- !is_zero_ether_addr(client_info->mac_dst)) {
+ if (is_valid_ether_addr(client_info->mac_dst)) {
client_info->ntt = 1;
bond->alb_info.rx_ntt = 1;
} else {
@@ -1418,9 +1408,9 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device
*bond_dev)
case ETH_P_IP: {
const struct iphdr *iph = ip_hdr(skb);
- if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast) ||
- (iph->daddr == ip_bcast) ||
- (iph->protocol == IPPROTO_IGMP)) {
+ if (is_broadcast_ether_addr(eth_data->h_dest) ||
+ iph->daddr == ip_bcast ||
+ iph->protocol == IPPROTO_IGMP) {
do_tx_balance = false;
break;
}
@@ -1432,7 +1422,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device
*bond_dev)
/* IPv6 doesn't really use broadcast mac address, but leave
* that here just in case.
*/
- if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast)) {
+ if (is_broadcast_ether_addr(eth_data->h_dest)) {
do_tx_balance = false;
break;
}
--
2.17.0
