Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <20220824111712.5999-1-sunshouxin@chinatelecom.cn>
References: <20220824111712.5999-1-sunshouxin@chinatelecom.cn>
From:   Jay Vosburgh <j.vosburgh@gmail.com>
Date:   Wed, 24 Aug 2022 11:07:13 -0700
Message-ID: <CAAoacNmKa5oM10J6DTLJ6PANmdS8k80Lcxygv_vXd_0DduXM4A@mail.gmail.com>
Subject: Re: [PATCH net-next v2] bonding: Remove unnecessary check
To:     Sun Shouxin <sunshouxin@chinatelecom.cn>
Cc:     vfalico@gmail.com, andy@greyhouse.net, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org,
        john.fastabend@gmail.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        huyd12@chinatelecom.cn
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>
>