From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) (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 2957548F826 for ; Fri, 21 Aug 2026 13:12:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.47 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787317969; cv=none; b=KdNh1PNtBcS495k0NObUjfvu3/dvYP2ddyEHEKkKmKHO26QENHJbmQkxvWJB55oRTotkh3qAkYgqqJCfRQwgcjG6h4GdN1DieCZ+xypkTdU6YLGU9m+2qL0/7E2tIpWyHMTIOkY9AQknzrxqtFlx3B3Gyr2s8ec78w3DCGscfRg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787317969; c=relaxed/simple; bh=xIawzfQXdKP0Li2QjPtdvuJ9BFkJyYj3YzYRq2dpdx4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Pwh4byHpPG00USXeIKuRaaOY+/x4N50UkpdRa+3Gz1TZZVuyqzb7TBbslQabE2iMMVrUnGvKHREs1Sk9l8aMOQQYTjDVZFi41CaZqBHnF5sbkWl97Y++zpG3coPbndA0ZTG9mxSwtDBF8XZg5SaUprP+P6hq0YvYJxsLrGcAyBs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=blackwall.org; spf=none smtp.mailfrom=blackwall.org; dkim=pass (2048-bit key) header.d=blackwall.org header.i=@blackwall.org header.b=LJzqUWSG; arc=none smtp.client-ip=209.85.208.47 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=blackwall.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=blackwall.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=blackwall.org header.i=@blackwall.org header.b="LJzqUWSG" Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-69f7fa1c548so2030925a12.2 for ; Fri, 21 Aug 2026 06:12:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=blackwall.org; s=google; t=1787317954; x=1787922754; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to:content-type; bh=JSVhaEczIAITOiUBAk3m0NB6xGaNx6EDtdLJJZuRCEo=; b=LJzqUWSGeql7ttoe3JTTDfWeKIrFSVtAa845PTvWE1xw2wTyMwfX4PRqCr+pW499FR VTK4KPE/1dbGVh0WWxHo4pxvxLRc14IEsaGrllLUnFpHrDyWPhZ2J/zKvdue4E3z8p90 3ZZRumtwuor7TksPB+Gp5mAw85gOFYBxj7R/Pcmrqg9SXKM4eGoSaBTpYXU2FTTJa84z ZkLjCgorDPWIq1UXUlofK2HjybWulJPzxFZUSdC82yJdkjUQxQWeiuBn9b2vrxgB8jkN ggsf/EYtYeIKgeq8D0x5Hk/d4EYVTmPvwdPx1Nwvnn2nv0J8CAjFCPVs607J9DDW07SF V5IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787317954; x=1787922754; h=content-transfer-encoding:content-type:in-reply-to:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=JSVhaEczIAITOiUBAk3m0NB6xGaNx6EDtdLJJZuRCEo=; b=pMmeEZsvBDmWBxDHmTpYIvP56fqGG94yoIYSK9QPthOQ1rpC1TuRRw8KSng1BTeXHN 67/JmO2s+kYX78Ixk6hnQWQdCZEtRpOYWUMeIH73r5tMbLMdUErp+nu82rDD6LZYGjpc cy8gHiDUh5TprJjK2AoEW9fXSN7AQEgkRfp6sAVidlq+X0dhaS28yL7y+87th0ttjFyk lZBboiGkG9Mh//uoUtRGC0m5ak7oT3JuIkk7WRevXmVKLTYlAAUdfltagT1iLwNPVA2j JFXe43kT31K8K3KxFZTSxhiuJ7d4pU7EqyixxCbVYOfljrrqBD1EFPAX7KRyn2dE28UJ gy+Q== X-Forwarded-Encrypted: i=1; AHgh+Rq02CO6wCIOPytHHMccsuw+OpnuwsNw8Jfqsls8rEqYaJoT4bjMG2YJcioVgJ5UFRGVPpBA8typAtNRnDk=@vger.kernel.org X-Gm-Message-State: AFuF++lc26spd780aRhV2jJo0TdnAz/qMvIxP9aJD7aIZKcDA3Ft0t5h xjXXlc9G182XxceU/TU0ZrB8/1zJDzbyJ++MvBrefnAvF/EX/scnIzc0WUayRvjb9cU= X-Gm-Gg: AR+sD135eZMm4QZhNcL2QAd1DZXqJDWGNuqP7LvyPKc8cF+TploYNuuYB8Bot0A9/+l gQ7NOtSLvhfh8dhpbEwLzi3tYeKfr/sJtp+SgtfxQOI478YXWhE5n8eZk8PsOp4OLkVD51QeVtD zWBbizbfcXxIoKEHJ0b/S8zY4TDrjR8qvlBWhs+c3UfwFM0xSA7oxb+iiWaqU7PAdWHDvop5cT6 H0B+P21SXLUdZGNh2LqdXnzRI/899WhZYIUCoEHY0SfX8azkEd/iSu0Uwyj8leoNgsFxX4IsnHy 9Fnj2PbgMv/M7pE5aj5sNO7jvHWnbEqzD+0lfUboIhGSfBMOXVzW+I0K0L7WfsWqF3d23B9XOPW tjdKzsDZBEHbldTgB6wBSkXqL2qi0mHLDFViQHtY6Lwi3eT/fwNDo/5nJmO4gtWoR7XGEGnPX3e 6puSCLxrzPNuarq3hDM8XsWk39EiVZxIn6rrvwuTpaE9tocjuv2uwVwDTUooyvxC+yo9TTo/wZm +Zm6GMm99dvo5tb4Lk= X-Received: by 2002:a05:6402:428c:b0:6a0:8885:e6f0 with SMTP id 4fb4d7f45d1cf-6a42f162cdbmr7788392a12.3.1787317953467; Fri, 21 Aug 2026 06:12:33 -0700 (PDT) Received: from [192.168.0.161] (78-154-15-182.ip.btc-net.bg. [78.154.15.182]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-6a3feec9e2esm6416459a12.5.2026.08.21.06.12.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 Aug 2026 06:12:32 -0700 (PDT) Message-ID: Date: Fri, 21 Aug 2026 16:12:31 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap() Content-Language: en-US, bg To: Hangbin Liu 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 References: <20260820-bond_overflow-v4-0-805ba0d3efb6@kylinos.cn> <20260820-bond_overflow-v4-2-805ba0d3efb6@kylinos.cn> From: Nikolay Aleksandrov In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 21/08/2026 15:58, Hangbin Liu wrote: > On Fri, Aug 21, 2026 at 02:33:39PM +0300, Nikolay Aleksandrov wrote: >>>> 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. >> >> Well that is debatable, to be correct you'd like to choose the NIC that is >> least overloaded, one could be at capacity and the other could be 10Gbps above >> capacity and you can still choose the second with this. >> >> If you make it a signed comparison then you can choose the least loaded, you'd > > Oh, do you want to fallback to use s64 (long long) in compute_gap? Then > all the counters need to using s64. The same with unbalanced_load, and we > can't using the "delta" anymore. Do we need to change back to using spin_lock > to protect the unbalanced_load writing. > why? see more below >> have to mark unknown speed with S64_MIN but it will compute the correct numbers > > Here do you mean > if (raw_speed == (u32)SPEED_UNKNOWN) > s64 speed = S64_MIN > > ? Then the 's64 gap = speed - load' will overflow, which means a 1Gbps NIC > (shown as unknown) will have more gaps then 10Gbps NIC (correctly shown speed) > oh that is easily fixed, it should not be a problem >> and you can choose the least overloaded NIC, which the current code actually >> does correctly. >> >> And most importantly - you definitely want to differentiate between unknown speed >> and overload, these should not be the same. > > If we use s64 and all slaves are overloaded, we can compute the difference. > But once there is an unknown speed NIC, we lose visibility into the real difference. > Such a NIC could be 1G, 10G, or 100G, yet we set its speed to `(u32)-1`. no, we use signed and set it at S64_MIN, it is never chosen. > > That is why I believe comparing gaps for NICs with unknown speed is meaningless. > Right and they shouldn't be considered or rather should be last. > Regarding overload scenarios: do you think this is a common‑case situation? > Because in practice, we rarely hit the theoretical maximum link speed. > For example, a 10Gbps NIC typically peaks at around ~950 Mbps. > > Thanks > Hangbin Completely untested, but something like: -static u64 compute_gap(struct slave *slave) +static s64 compute_gap(struct slave *slave) { u64 slave_load = SLAVE_TLB_INFO(slave).load << 3; u32 raw_speed = READ_ONCE(slave->speed); u64 speed = (u64)raw_speed << 20; if (raw_speed == (u32)SPEED_UNKNOWN) - return 0; - - if (speed <= slave_load) - return 0; + return S64_MIN; - return speed - slave_load; + return (s64)speed - (s64)slave_load; } Then change the selection variables and comparison: - u64 max_gap = 0; + s64 max_gap = S64_MIN; ... - u64 gap = compute_gap(slave); + s64 gap = compute_gap(slave); - if (max_gap <= gap) { + if (!least_loaded || max_gap < gap) { This should choose the slave with smallest gap and put the unknown speed behind all slaves with known speeds.