From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta1.migadu.com (out-85.mta1.migadu.com [95.215.58.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6DCE547989B for ; Fri, 21 Aug 2026 10:42:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.85 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787308989; cv=none; b=B0d4zsT4DAwW4BIXqcOUYTIsRKa0i64Hf/MnR2SKICj4VRmM5B2FHo7XywcBzw10q2Pi/f4K0WR05TNcp2ghJDj/WaiJoBMcieKw65lQdqoW0N32LnmRbubJJr1Ck8T8HXdJ+M7QUH3rqA+BE0FxypTkHzITCFV9vPg1HSyVKho= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787308989; c=relaxed/simple; bh=HIJtjC+DGHEiaopHeYxaIrtkuNdtQ2BjdMUGtvCpRkE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=uUYjeGutwylwaAKiqBX6gOf67OKJynKPzzAyHO2M/7pfLCdjuzmfhRg6BzfejvqYot2ydoiMvuMCv1ekBatwI8n0lc1W/JMWuCiqEwKUTr46iiPOn1JmNLX+1Tzee3/Zq0rS25QaLWx//4jXpCsxyQSpKg6bsNNO2BEZ5w6L/XM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=BWmaf2r8; arc=none smtp.client-ip=95.215.58.85 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="BWmaf2r8" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=HIJtjC+DGHEiaopHeYxaIrtkuNdtQ2BjdMUGtvCpRkE=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1787308975; v=1; x=1787913775; b=BWmaf2r8iZPXV9yfijHZXTEc34THQyinpXxG5Bp09nipF0gcyksgvWBBIq6uzVinzqjoMeG6 MPw/vfvK3vmsGESUfnvAKHvq4avZyxg9Fpt2a3x4k52okLNCvXePv35yjDVJNLWAC2o/sAOA5C8 bZyBFVEXiHV2lnFxuFNqJ35k= X-Envelope-To: linux-kernel@vger.kernel.org Received: from fedora (203.175.12.241) by smtp.migadu.com with ESMTPS id 2a6fe22ca90daeed; Fri, 21 Aug 2026 10:42:45 +0000 X-Mizu-Trace-ID: 2a6fe22ca90daeed X-Migadu-Flow: FLOW_OUT Date: Fri, 21 Aug 2026 18:42:38 +0800 From: Hangbin Liu To: Nikolay Aleksandrov Cc: Jay Vosburgh , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Hangbin Liu Subject: Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap() Message-ID: References: <20260820-bond_overflow-v4-0-805ba0d3efb6@kylinos.cn> <20260820-bond_overflow-v4-2-805ba0d3efb6@kylinos.cn> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Hi Nikolay, On Fri, Aug 21, 2026 at 01:16:20PM +0300, Nikolay Aleksandrov wrote: > > -static long long compute_gap(struct slave *slave) > > +static u64 compute_gap(struct slave *slave) > > { > > - return (s64) (slave->speed << 20) - /* Convert to Megabit per sec */ > > - (s64) (SLAVE_TLB_INFO(slave).load << 3); /* Bytes to bits */ > > + u64 slave_load = SLAVE_TLB_INFO(slave).load << 3; /* Bytes to bits */ > > + u32 raw_speed = READ_ONCE(slave->speed); > > + u64 speed = (u64)raw_speed << 20; /* Convert to bits per sec */ > > + > > + /* It's meaningless to compare gap on unknown speed NIC */ > > + if (raw_speed == (u32)SPEED_UNKNOWN) > > + return 0; > > + > > + /* Skip slave which is over loaded */ > > + if (speed <= slave_load) > > + return 0; > > + > > + return speed - slave_load; > > } > > static struct slave *tlb_get_least_loaded_slave(struct bonding *bond) > > { > > struct slave *slave, *least_loaded; > > struct list_head *iter; > > - long long max_gap; > > + u64 max_gap = 0; > > least_loaded = NULL; > > - max_gap = LLONG_MIN; > > /* Find the slave with the largest gap */ > > bond_for_each_slave_rcu(bond, slave, iter) { > > if (bond_slave_can_tx(slave)) { > > - long long gap = compute_gap(slave); > > + u64 gap = compute_gap(slave); > > - if (max_gap < gap) { > > + /* Make sure we have one available slave */ > > + if (max_gap <= gap) { > > least_loaded = slave; > > max_gap = gap; > > I think Sashiko's review has a point here: > "Does clamping the gap to 0 completely break load balancing when all interfaces > are overloaded? > When all slaves are overloaded, compute_gap() returns 0 for all of them. Since > max_gap is initialized to 0, max_gap <= gap will evaluate to 0 <= 0, which is > true. > This means tlb_get_least_loaded_slave() will continually update least_loaded to > the current slave, ultimately routing all traffic to the last slave in the list > instead of distributing it across the least overloaded interfaces." Yes, I have thought about this question. Previous code set max_gap LLONG_MIN, so there always has a slave assigned. Now we use u64. If we use (max_gap < gap), there may return NULL pointer. > > That is, compute_gap makes multiple different scenarios look the same: > if speed is unknown = 0 > if exactly equal capacity = 0 > if overloaded by *any* amount = 0 Yes, if there is are 2 NICs with 1 Gbps and 10 Gbps, but both shows as unknown. There is no meaning to compare the gaps. Because they use the same speed value (u32)-1 << 20. If two NICs both overloaded or equal capacity. There is also no mean to select any devices. > > So Sashiko's comment seems correct, it doesn't matter if a slave is overloaded > with 1 gbps or 100, they will look the same. I've thought like: if (speed over load) return 1; if (speed unknown) return 2; That sounds reasonable: we could select an unknown‑speed NIC that is not overloaded. However, this does not work. When performing the `speed <= slave_load` check, the speed value has already been shifted. Unknown speed is converted to a very large value, so the calculation will never report an overload condition — even though the actual hardware may already be overloaded. This is why I end up returning 0 for all such cases. Hope my explanation is clear. Thanks Hangbin