| Message ID | 20220824111712.5999-1-sunshouxin@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] bonding: Remove unnecessary check | expand |
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 > >
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 >> >> >
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; }
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(-)