Re: [PATCH net-next] bonding: correct rr balancing during link failure

From: Lars Everbrand <lars.everbrand@protonmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: linux-kernel@vger.kernel.org, Jay Vosburgh <j.vosburgh@gmail.com>,
	Veaceslav Falico <vfalico@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	"David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next] bonding: correct rr balancing during link failure
Date: Tue, 15 Dec 2020 21:32:50 +0000	[thread overview]
Message-ID: <X9krelP/8MwGP0V5@black-debian> (raw)
In-Reply-To: <20201205114513.4886d15e@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>

On Sat, Dec 05, 2020 at 11:45:13AM -0800, Jakub Kicinski wrote:
> Thanks for the patch!
Kind words for my first attempt at this. Sorry for answering a bit late,
proton-bridge is not my best friend lately.
> 
> Looking at the code in question it feels a little like we're breaking
> abstractions if we bump the counter directly in get_slave_by_id.
My intention was to avoid a big change, and this was the easiest way. I
trust your opinion here.
> 
> For one thing when the function is called for IGMP packets the counter
> should not be incremented at all. But also if packets_per_slave is not
> 1 we'd still be hitting the same leg multiple times (packets_per_slave
> / 2). So it seems like we should round the counter up somehow?
I did not consider this case, I only test =1 and random. Yeah, it breaks
if the counter is updated per packet in any >1 case. 
> 
> For IGMP maybe we don't have to call bond_get_slave_by_id() at all,
> IMHO, just find first leg that can TX. Then we can restructure
> bond_get_slave_by_id() appropriately for the non-IGMP case.
I can have another look but my I am not confident that I am skilled
enough in this area to produce a larger overhaul...