Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Jay Vosburgh <jay.vosburgh@canonical.com>
To:     David Ahern <dsahern@kernel.org>
cc:     Sun Shouxin <sunshouxin@chinatelecom.cn>, vfalico@gmail.com,
        andy@greyhouse.net, davem@davemloft.net, kuba@kernel.org,
        yoshfuji@linux-ipv6.org, oliver@neukum.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, huyd12@chinatelecom.cn
Subject: Re: [PATCH v4] net:bonding:Add support for IPV6 RLB to balance-alb mode
In-reply-to: <eff0021c-5a9b-5c44-3fb7-24387cf13e16@kernel.org>
References: <20220317061521.23985-1-sunshouxin@chinatelecom.cn> <eff0021c-5a9b-5c44-3fb7-24387cf13e16@kernel.org>
Comments: In-reply-to David Ahern <dsahern@kernel.org>
   message dated "Thu, 17 Mar 2022 12:49:54 -0600."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <24596.1647547831.1@famine>
Date:   Thu, 17 Mar 2022 13:10:31 -0700
Message-ID: <24597.1647547831@famine>
Precedence: bulk

David Ahern <dsahern@kernel.org> wrote:

>On 3/17/22 12:15 AM, Sun Shouxin wrote:
>> This patch is implementing IPV6 RLB for balance-alb mode.
>> 
>> Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
>> Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
>> ---
>> changelog:
>> v1-->v2:
>> -Remove ndisc_bond_send_na and refactor ndisc_send_na.
>> -In rlb_nd_xmit, if the lladdr is not local, return curr_active_slave.
>> -Don't send neighbor advertisement message when receiving
>>  neighbor advertisement message in rlb6_update_entry_from_na.
>> 
>> v2-->v3:
>> -Don't export ndisc_send_na.
>> -Use ipv6_stub->ndisc_send_na to replace ndisc_send_na
>>  in rlb6_update_client.
>> 
>> v3-->v4:
>> -Submit all code at a whole patch.
>
>you misunderstood Jakub's comment. The code should evolve with small,
>focused patches and each patch needs to compile and function correctly
>(ie., no breakage).

	Agreed; the split of the patches was not at issue, it was that
each patch in a series must compile and the built kernel must function
rationally.

>You need to respond to Jiri's question about why this feature is needed.

	I'm not entirely sold on adding IPv6 RLB for balance-alb, but
the IPv4 version of it does see moderate levels of use, even now.  It's
less common than LACP by far, though.  I'd like to know why someone
would choose IPv6 RLB over LACP.  I wonder if this is a checklist item
somewhere that something must have "complete support for IPv6" or words
to that effect, versus an actual functional need.

>After that:
>
>1. patch 1 adds void *data to ndisc_send_na stub function and
>ndisc_send_na direct function. Update all places that use both
>ndisc_send_na to pass NULL as the data parameter.
>
>2. patch 2 refactors ndisc_send_na to handle the new data argument
>
>3. patch 3 exports any IPv6 functions. explain why each needs to be
>exported.
>
>4. patch 4 .... bonding changes. (bonding folks can respond on how to
>introduce that change).

	Looking at the previous patch for bonding, my two initial
requests are:

	1) A more detailed commit message.  The only way to understand
how any of this actually works is reading the code, there is no higher
level description.

	2) How does this interact with the IPv4 RLB logic?  Is it
possible for a given bond interface MAC to be "assigned" to two
different peers (one IPv4, one IPv6), and if so, does that behave in an
expected manner?  I.e., two peers on the network could receive
contradictory information via ARP and ND for the MAC address of a given
peer.  This is already possible with the IPv4 RLB, but with an
additional IPv6 RLB, a single peer could see two different MACs for a
given host (one via IPv4, one via IPv6), and another peer could see the
opposite, or even disjoint information across several peers.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com