[PATCH net-next v2] bonding: Remove unnecessary check

[PATCH net-next v2] bonding: Remove unnecessary check

[Sun Shouxin]

This code is intended to support bond alb interface added to
Linux bridge by modifying MAC, however, it doesn't work for
one bond alb interface with vlan added to bridge.
Since commit d5410ac7b0ba("net:bonding:support balance-alb
interface with vlan to bridge"), new logic is adapted to handle
bond alb with or without vlan id, and then the code is deprecated.

Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
---
 drivers/net/bonding/bond_main.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 50e60843020c..6b0f0ce9b9a1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1578,19 +1578,6 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 
 	skb->dev = bond->dev;
 
-	if (BOND_MODE(bond) == BOND_MODE_ALB &&
-	    netif_is_bridge_port(bond->dev) &&
-	    skb->pkt_type == PACKET_HOST) {
-
-		if (unlikely(skb_cow_head(skb,
-					  skb->data - skb_mac_header(skb)))) {
-			kfree_skb(skb);
-			return RX_HANDLER_CONSUMED;
-		}
-		bond_hw_addr_copy(eth_hdr(skb)->h_dest, bond->dev->dev_addr,
-				  bond->dev->addr_len);
-	}
-
 	return ret;
 }
 
-- 
2.27.0

Re: [PATCH net-next v2] bonding: Remove unnecessary check

[Jay Vosburgh]

On 8/24/22, Sun Shouxin <sunshouxin@chinatelecom.cn> wrote:
> This code is intended to support bond alb interface added to
> Linux bridge by modifying MAC, however, it doesn't work for
> one bond alb interface with vlan added to bridge.
> Since commit d5410ac7b0ba("net:bonding:support balance-alb
> interface with vlan to bridge"), new logic is adapted to handle
> bond alb with or without vlan id, and then the code is deprecated.

I think this could still be clearer; the actual changes relate to the stack of
interfaces (e.g., eth0 -> bond0 -> vlan123 -> bridge0), not what VLAN tags
incoming traffic contains.

The code being removed here is specifically for the case of
eth0 -> bond0 -> bridge0, without an intermediate VLAN interface
in the stack (because, if memory serves, netif_is_bridge_port doesn't
transfer through to the bond if there's a VLAN interface in between).

Also, this code is for incoming traffic, assigning the bond's MAC to
traffic arriving on interfaces other than the active interface (which bears
the bond's MAC in alb mode; the other interfaces have different MACs).
Commit d5410ac7b0ba affects the balance assignments for outgoing ARP
traffic.  I'm not sure that d5410 is an exact replacement for the code this
patch removes.

       -J

>
> Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
> Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
> ---
>  drivers/net/bonding/bond_main.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index 50e60843020c..6b0f0ce9b9a1 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1578,19 +1578,6 @@ static rx_handler_result_t bond_handle_frame(struct
> sk_buff **pskb)
>
>  	skb->dev = bond->dev;
>
> -	if (BOND_MODE(bond) == BOND_MODE_ALB &&
> -	    netif_is_bridge_port(bond->dev) &&
> -	    skb->pkt_type == PACKET_HOST) {
> -
> -		if (unlikely(skb_cow_head(skb,
> -					  skb->data - skb_mac_header(skb)))) {
> -			kfree_skb(skb);
> -			return RX_HANDLER_CONSUMED;
> -		}
> -		bond_hw_addr_copy(eth_hdr(skb)->h_dest, bond->dev->dev_addr,
> -				  bond->dev->addr_len);
> -	}
> -
>  	return ret;
>  }
>
> --
> 2.27.0
>
>

Re: [PATCH net-next v2] bonding: Remove unnecessary check

[Jonathan Toppins]

On 8/24/22 14:07, Jay Vosburgh wrote:
> On 8/24/22, Sun Shouxin <sunshouxin@chinatelecom.cn> wrote:
>> This code is intended to support bond alb interface added to
>> Linux bridge by modifying MAC, however, it doesn't work for
>> one bond alb interface with vlan added to bridge.
>> Since commit d5410ac7b0ba("net:bonding:support balance-alb
>> interface with vlan to bridge"), new logic is adapted to handle
>> bond alb with or without vlan id, and then the code is deprecated.
> 
> I think this could still be clearer; the actual changes relate to the stack of
> interfaces (e.g., eth0 -> bond0 -> vlan123 -> bridge0), not what VLAN tags
> incoming traffic contains.
> 
> The code being removed here is specifically for the case of
> eth0 -> bond0 -> bridge0, without an intermediate VLAN interface
> in the stack (because, if memory serves, netif_is_bridge_port doesn't
> transfer through to the bond if there's a VLAN interface in between).
> 
> Also, this code is for incoming traffic, assigning the bond's MAC to
> traffic arriving on interfaces other than the active interface (which bears
> the bond's MAC in alb mode; the other interfaces have different MACs).
> Commit d5410ac7b0ba affects the balance assignments for outgoing ARP
> traffic.  I'm not sure that d5410 is an exact replacement for the code this
> patch removes.

I would be more comfortable with a change like this if it can be 
demonstrated that an example test case functions as expected before and 
after the change. Could a selftests test be written with veths to 
demonstrate this code is indeed redundant?

-Jon

> 
>>
>> Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
>> Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
>> ---
>>   drivers/net/bonding/bond_main.c | 13 -------------
>>   1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index 50e60843020c..6b0f0ce9b9a1 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1578,19 +1578,6 @@ static rx_handler_result_t bond_handle_frame(struct
>> sk_buff **pskb)
>>
>>   	skb->dev = bond->dev;
>>
>> -	if (BOND_MODE(bond) == BOND_MODE_ALB &&
>> -	    netif_is_bridge_port(bond->dev) &&
>> -	    skb->pkt_type == PACKET_HOST) {
>> -
>> -		if (unlikely(skb_cow_head(skb,
>> -					  skb->data - skb_mac_header(skb)))) {
>> -			kfree_skb(skb);
>> -			return RX_HANDLER_CONSUMED;
>> -		}
>> -		bond_hw_addr_copy(eth_hdr(skb)->h_dest, bond->dev->dev_addr,
>> -				  bond->dev->addr_len);
>> -	}
>> -
>>   	return ret;
>>   }
>>
>> --
>> 2.27.0
>>
>>
>