[PATCH net v4 0/2] bonding: fix TLB load-tracking overflow on high-speed NICs

[PATCH net v4 0/2] bonding: fix TLB load-tracking overflow on high-speed NICs

[Hangbin Liu]

The bonding TLB (Transmit Load Balancing) mode tracks per-slave and
per-client transmit byte counts in u32 fields. At sustained throughput
above ~3.2 Gbit/s over the 10-second rebalance interval these counters
wrap, causing compute_gap() to produce incorrect gap values and
mis-select transmit slaves. Such speeds are routine on modern NICs
under heavy traffic.

This two-patch series fixes the overflow by widening the relevant
fields to u64.

Patch 1 converts the unbalanced_load counter to per-cpu state as a
preparatory step. The counter sits in the transmit hot path, so
converting it to per-cpu before widening avoids introducing cross-CPU
synchronization overhead for a u64. Also use a prev_total_unbalanced
to store the previous total load to avoid reset per-cpu data.

Patch 2 widens tx_bytes, load_history, load, and the per-cpu
unbalanced_load tx_bytes from u32 to u64. It adds u64_stats_sync
protection for the per-cpu counter to prevent tearing on 32-bit
architectures, and reworks compute_gap() to use u64 arithmetic with
READ_ONCE() on slave->speed.

Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
---
Changes in v4:
- move per-cpu allocation to tlb_initialize/tlb_deinitialize (Nikolay Aleksandrov)
- use an extra prev_total_unbalanced to avoid reset per-cpu data (Nikolay Aleksandrov)
- Link to v3: https://lore.kernel.org/r/20260818-bond_overflow-v3-0-e05d4dbc2fd8@kylinos.cn

Changes in v3:
- Add a preparatory patch to convert unbalanced_load to per-cpu first
- widens tlb counters to u64 and add helpers to prevent tearing on 32-bit
- Link to v2: https://lore.kernel.org/r/20260814-bond_overflow-v2-1-d3fe588ad167@kylinos.cn

Changes in v2:
- update comment description, including AI-detected info.
- fix tx_bytes/load type detected by sashiko
- cast SPEED_UNKNOWN to 0 before shift, detected by sashiko
- Link to v1: https://lore.kernel.org/r/20260810-bond_overflow-v1-1-c9ff29d76770@kylinos.cn

---
Hangbin Liu (2):
      bonding: convert unbalanced_load to per-cpu state
      bonding: fix u32 overflow in compute_gap()

 drivers/net/bonding/bond_alb.c | 82 ++++++++++++++++++++++++++++++++++--------
 include/net/bond_alb.h         | 14 +++++---
 2 files changed, 77 insertions(+), 19 deletions(-)
---
base-commit: 564973a259ec76f2dad0853420e7034cc43994c4
change-id: 20260806-bond_overflow-ac6a6a78d6a0

Best regards,
-- 
Hangbin Liu <liuhangbin@kylinos.cn>

[PATCH net v4 1/2] bonding: convert unbalanced_load to per-cpu state

[Hangbin Liu]

From: Hangbin Liu <liuhangbin@kylinos.cn>

A later patch widens the bonding TLB tx counters from u32 to u64. The
unbalanced_load counter sits in the transmit hot path, and cross-CPU
synchronization of a u64 would introduce measurable overhead. Convert
unbalanced_load to a per-cpu counter first so that the subsequent
widening only touches per-cpu data local to each CPU.

Introduce struct unbalanced_load_stats to hold the per-cpu counter,
and move the aggregation into a helper, reset_unbalanced_load(), which
sums all per-cpu instances. Use the delta of current total load vs
variable prev_total_unbalanced to calculate the loading.

Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
---
 drivers/net/bonding/bond_alb.c | 37 ++++++++++++++++++++++++++++++-------
 include/net/bond_alb.h         |  7 ++++++-
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 839f7482dc18..0afed2c39231 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -133,6 +133,10 @@ static int tlb_initialize(struct bonding *bond)
 	if (!new_hashtbl)
 		return -ENOMEM;
 
+	bond_info->unbalanced_load = alloc_percpu(struct unbalanced_load_stats);
+	if (!bond_info->unbalanced_load)
+		goto out;
+
 	spin_lock_bh(&bond->mode_lock);
 
 	bond_info->tx_hashtbl = new_hashtbl;
@@ -143,6 +147,10 @@ static int tlb_initialize(struct bonding *bond)
 	spin_unlock_bh(&bond->mode_lock);
 
 	return 0;
+
+out:
+	kfree(new_hashtbl);
+	return -ENOMEM;
 }
 
 /* Must be called only after all slaves have been released */
@@ -154,6 +162,8 @@ static void tlb_deinitialize(struct bonding *bond)
 
 	kfree(bond_info->tx_hashtbl);
 	bond_info->tx_hashtbl = NULL;
+	free_percpu(bond_info->unbalanced_load);
+	bond_info->prev_total_unbalanced = 0;
 
 	spin_unlock_bh(&bond->mode_lock);
 }
@@ -1345,7 +1355,7 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
 		/* unbalanced or unassigned, send through primary */
 		tx_slave = rcu_dereference(bond->curr_active_slave);
 		if (bond->params.tlb_dynamic_lb)
-			bond_info->unbalanced_load += skb->len;
+			this_cpu_add(bond_info->unbalanced_load->tx_bytes, skb->len);
 	}
 
 	if (tx_slave && bond_slave_can_tx(tx_slave)) {
@@ -1529,6 +1539,23 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 	return bond_do_alb_xmit(skb, bond, tx_slave);
 }
 
+static u32 reset_unbalanced_load(struct alb_bond_info *bond_info)
+{
+	struct unbalanced_load_stats *p;
+	u32 delta, total_bytes = 0;
+	int i;
+
+	for_each_possible_cpu(i) {
+		p = per_cpu_ptr(bond_info->unbalanced_load, i);
+		total_bytes += READ_ONCE(p->tx_bytes);
+	}
+
+	delta = total_bytes - bond_info->prev_total_unbalanced;
+	bond_info->prev_total_unbalanced = total_bytes;
+
+	return delta / BOND_TLB_REBALANCE_INTERVAL;
+}
+
 void bond_alb_monitor(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
@@ -1570,12 +1597,8 @@ void bond_alb_monitor(struct work_struct *work)
 	if (atomic_read(&bond_info->tx_rebalance_counter) >= BOND_TLB_REBALANCE_TICKS) {
 		bond_for_each_slave_rcu(bond, slave, iter) {
 			tlb_clear_slave(bond, slave, 1);
-			if (slave == rcu_access_pointer(bond->curr_active_slave)) {
-				SLAVE_TLB_INFO(slave).load =
-					bond_info->unbalanced_load /
-						BOND_TLB_REBALANCE_INTERVAL;
-				bond_info->unbalanced_load = 0;
-			}
+			if (slave == rcu_access_pointer(bond->curr_active_slave))
+				SLAVE_TLB_INFO(slave).load = reset_unbalanced_load(bond_info);
 		}
 		atomic_set(&bond_info->tx_rebalance_counter, 0);
 	}
diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
index e5945427f38d..6fb09b4fc7e2 100644
--- a/include/net/bond_alb.h
+++ b/include/net/bond_alb.h
@@ -123,9 +123,14 @@ struct tlb_slave_info {
 			 */
 };
 
+struct unbalanced_load_stats {
+	u32			tx_bytes;
+};
+
 struct alb_bond_info {
 	struct tlb_client_info	*tx_hashtbl; /* Dynamically allocated */
-	u32			unbalanced_load;
+	struct unbalanced_load_stats __percpu	*unbalanced_load;
+	u32			prev_total_unbalanced;
 	atomic_t		tx_rebalance_counter;
 	int			lp_counter;
 	/* -------- rlb parameters -------- */

-- 
2.55.0

[PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Hangbin Liu]

From: Hangbin Liu <liuhangbin@kylinos.cn>

The TLB load-tracking fields tx_bytes, load_history, load, and
unbalanced_load are all u32. At sustained throughput above ~3.2 Gbit/s
over the 10-second rebalance interval the byte counters wrap, causing
compute_gap() to produce incorrect gap values and mis-select slaves.
Such speeds are common on modern NICs under heavy traffic.

Widen these fields to u64. Use u64_stats_sync to protect the per-cpu
unbalanced_load_stats against tearing on 32-bit architectures, and
div_u64() for the 64-bit divisions. The tx_bytes and load_history
are protected in spin_lock. Also protect the slave load writing in
bond_alb_monitor() with spin_lock in case of tear on 32-bit.

Rework compute_gap() to use u64 arithmetic throughout. Return 0 when the
speed is unknown or the slave is already overloaded.

Detected by AI code review.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
---
 drivers/net/bonding/bond_alb.c | 65 ++++++++++++++++++++++++++++++------------
 include/net/bond_alb.h         | 11 +++----
 2 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 0afed2c39231..372db54803d3 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -6,6 +6,7 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/pkt_sched.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -74,8 +75,8 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
 static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
 {
 	if (save_load) {
-		entry->load_history = 1 + entry->tx_bytes /
-				      BOND_TLB_REBALANCE_INTERVAL;
+		entry->load_history = 1 + div_u64(entry->tx_bytes,
+				      BOND_TLB_REBALANCE_INTERVAL);
 		entry->tx_bytes = 0;
 	}
 
@@ -133,7 +134,7 @@ static int tlb_initialize(struct bonding *bond)
 	if (!new_hashtbl)
 		return -ENOMEM;
 
-	bond_info->unbalanced_load = alloc_percpu(struct unbalanced_load_stats);
+	bond_info->unbalanced_load = netdev_alloc_pcpu_stats(struct unbalanced_load_stats);
 	if (!bond_info->unbalanced_load)
 		goto out;
 
@@ -168,27 +169,38 @@ static void tlb_deinitialize(struct bonding *bond)
 	spin_unlock_bh(&bond->mode_lock);
 }
 
-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;
 			}
@@ -1354,8 +1366,14 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
 	if (!tx_slave) {
 		/* unbalanced or unassigned, send through primary */
 		tx_slave = rcu_dereference(bond->curr_active_slave);
-		if (bond->params.tlb_dynamic_lb)
-			this_cpu_add(bond_info->unbalanced_load->tx_bytes, skb->len);
+		if (bond->params.tlb_dynamic_lb) {
+			struct unbalanced_load_stats *pcpu_load;
+
+			pcpu_load = this_cpu_ptr(bond_info->unbalanced_load);
+			u64_stats_update_begin(&pcpu_load->syncp);
+			u64_stats_add(&pcpu_load->tx_bytes, skb->len);
+			u64_stats_update_end(&pcpu_load->syncp);
+		}
 	}
 
 	if (tx_slave && bond_slave_can_tx(tx_slave)) {
@@ -1539,21 +1557,27 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 	return bond_do_alb_xmit(skb, bond, tx_slave);
 }
 
-static u32 reset_unbalanced_load(struct alb_bond_info *bond_info)
+static u64 reset_unbalanced_load(struct alb_bond_info *bond_info)
 {
+	u64 delta, tx_bytes, total_bytes = 0;
 	struct unbalanced_load_stats *p;
-	u32 delta, total_bytes = 0;
+	unsigned int start;
 	int i;
 
 	for_each_possible_cpu(i) {
 		p = per_cpu_ptr(bond_info->unbalanced_load, i);
-		total_bytes += READ_ONCE(p->tx_bytes);
+		do {
+			start = u64_stats_fetch_begin(&p->syncp);
+			tx_bytes = u64_stats_read(&p->tx_bytes);
+		} while (u64_stats_fetch_retry(&p->syncp, start));
+
+		total_bytes += tx_bytes;
 	}
 
 	delta = total_bytes - bond_info->prev_total_unbalanced;
 	bond_info->prev_total_unbalanced = total_bytes;
 
-	return delta / BOND_TLB_REBALANCE_INTERVAL;
+	return div_u64(delta, BOND_TLB_REBALANCE_INTERVAL);
 }
 
 void bond_alb_monitor(struct work_struct *work)
@@ -1597,8 +1621,13 @@ void bond_alb_monitor(struct work_struct *work)
 	if (atomic_read(&bond_info->tx_rebalance_counter) >= BOND_TLB_REBALANCE_TICKS) {
 		bond_for_each_slave_rcu(bond, slave, iter) {
 			tlb_clear_slave(bond, slave, 1);
-			if (slave == rcu_access_pointer(bond->curr_active_slave))
-				SLAVE_TLB_INFO(slave).load = reset_unbalanced_load(bond_info);
+			if (slave == rcu_access_pointer(bond->curr_active_slave)) {
+				u64 new_load = reset_unbalanced_load(bond_info);
+
+				spin_lock_bh(&bond->mode_lock);
+				SLAVE_TLB_INFO(slave).load = new_load;
+				spin_unlock_bh(&bond->mode_lock);
+			}
 		}
 		atomic_set(&bond_info->tx_rebalance_counter, 0);
 	}
diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
index 6fb09b4fc7e2..32f1981033e4 100644
--- a/include/net/bond_alb.h
+++ b/include/net/bond_alb.h
@@ -57,12 +57,12 @@ struct tlb_client_info {
 				 * packets to a Client that the Hash function
 				 * gave this entry index.
 				 */
-	u32 tx_bytes;		/* Each Client accumulates the BytesTx that
+	u64 tx_bytes;		/* Each Client accumulates the BytesTx that
 				 * were transmitted to it, and after each
 				 * CallBack the LoadHistory is divided
 				 * by the balance interval
 				 */
-	u32 load_history;	/* This field contains the amount of Bytes
+	u64 load_history;	/* This field contains the amount of Bytes
 				 * that were transmitted to this client by
 				 * the server on the previous balance
 				 * interval in Bps.
@@ -118,19 +118,20 @@ struct tlb_slave_info {
 			 * are the entries that were assigned to use this
 			 * slave for transmit.
 			 */
-	u32 load;	/* Each slave sums the loadHistory of all clients
+	u64 load;	/* Each slave sums the loadHistory of all clients
 			 * assigned to it
 			 */
 };
 
 struct unbalanced_load_stats {
-	u32			tx_bytes;
+	u64_stats_t		tx_bytes;
+	struct u64_stats_sync	syncp;
 };
 
 struct alb_bond_info {
 	struct tlb_client_info	*tx_hashtbl; /* Dynamically allocated */
 	struct unbalanced_load_stats __percpu	*unbalanced_load;
-	u32			prev_total_unbalanced;
+	u64			prev_total_unbalanced;
 	atomic_t		tx_rebalance_counter;
 	int			lp_counter;
 	/* -------- rlb parameters -------- */

-- 
2.55.0

Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Nikolay Aleksandrov]

On 20/08/2026 08:55, Hangbin Liu wrote:
> From: Hangbin Liu <liuhangbin@kylinos.cn>
> 
> The TLB load-tracking fields tx_bytes, load_history, load, and
> unbalanced_load are all u32. At sustained throughput above ~3.2 Gbit/s
> over the 10-second rebalance interval the byte counters wrap, causing
> compute_gap() to produce incorrect gap values and mis-select slaves.
> Such speeds are common on modern NICs under heavy traffic.
> 
> Widen these fields to u64. Use u64_stats_sync to protect the per-cpu
> unbalanced_load_stats against tearing on 32-bit architectures, and
> div_u64() for the 64-bit divisions. The tx_bytes and load_history
> are protected in spin_lock. Also protect the slave load writing in
> bond_alb_monitor() with spin_lock in case of tear on 32-bit.
> 
> Rework compute_gap() to use u64 arithmetic throughout. Return 0 when the
> speed is unknown or the slave is already overloaded.
> 
> Detected by AI code review.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
> ---
>   drivers/net/bonding/bond_alb.c | 65 ++++++++++++++++++++++++++++++------------
>   include/net/bond_alb.h         | 11 +++----
>   2 files changed, 53 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 0afed2c39231..372db54803d3 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -6,6 +6,7 @@
>   #include <linux/skbuff.h>
>   #include <linux/netdevice.h>
>   #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>   #include <linux/pkt_sched.h>
>   #include <linux/spinlock.h>
>   #include <linux/slab.h>
> @@ -74,8 +75,8 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
>   static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>   {
>   	if (save_load) {
> -		entry->load_history = 1 + entry->tx_bytes /
> -				      BOND_TLB_REBALANCE_INTERVAL;
> +		entry->load_history = 1 + div_u64(entry->tx_bytes,
> +				      BOND_TLB_REBALANCE_INTERVAL);
>   		entry->tx_bytes = 0;
>   	}
>   
> @@ -133,7 +134,7 @@ static int tlb_initialize(struct bonding *bond)
>   	if (!new_hashtbl)
>   		return -ENOMEM;
>   
> -	bond_info->unbalanced_load = alloc_percpu(struct unbalanced_load_stats);
> +	bond_info->unbalanced_load = netdev_alloc_pcpu_stats(struct unbalanced_load_stats);
>   	if (!bond_info->unbalanced_load)
>   		goto out;
>   
> @@ -168,27 +169,38 @@ static void tlb_deinitialize(struct bonding *bond)
>   	spin_unlock_bh(&bond->mode_lock);
>   }
>   
> -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."

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

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.

>   			}
> @@ -1354,8 +1366,14 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>   	if (!tx_slave) {
>   		/* unbalanced or unassigned, send through primary */
>   		tx_slave = rcu_dereference(bond->curr_active_slave);
> -		if (bond->params.tlb_dynamic_lb)
> -			this_cpu_add(bond_info->unbalanced_load->tx_bytes, skb->len);
> +		if (bond->params.tlb_dynamic_lb) {
> +			struct unbalanced_load_stats *pcpu_load;
> +
> +			pcpu_load = this_cpu_ptr(bond_info->unbalanced_load);
> +			u64_stats_update_begin(&pcpu_load->syncp);
> +			u64_stats_add(&pcpu_load->tx_bytes, skb->len);
> +			u64_stats_update_end(&pcpu_load->syncp);
> +		}
>   	}
>   
>   	if (tx_slave && bond_slave_can_tx(tx_slave)) {
> @@ -1539,21 +1557,27 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>   	return bond_do_alb_xmit(skb, bond, tx_slave);
>   }
>   
> -static u32 reset_unbalanced_load(struct alb_bond_info *bond_info)
> +static u64 reset_unbalanced_load(struct alb_bond_info *bond_info)
>   {
> +	u64 delta, tx_bytes, total_bytes = 0;
>   	struct unbalanced_load_stats *p;
> -	u32 delta, total_bytes = 0;
> +	unsigned int start;
>   	int i;
>   
>   	for_each_possible_cpu(i) {
>   		p = per_cpu_ptr(bond_info->unbalanced_load, i);
> -		total_bytes += READ_ONCE(p->tx_bytes);
> +		do {
> +			start = u64_stats_fetch_begin(&p->syncp);
> +			tx_bytes = u64_stats_read(&p->tx_bytes);
> +		} while (u64_stats_fetch_retry(&p->syncp, start));
> +
> +		total_bytes += tx_bytes;
>   	}
>   
>   	delta = total_bytes - bond_info->prev_total_unbalanced;
>   	bond_info->prev_total_unbalanced = total_bytes;
>   
> -	return delta / BOND_TLB_REBALANCE_INTERVAL;
> +	return div_u64(delta, BOND_TLB_REBALANCE_INTERVAL);
>   }
>   
>   void bond_alb_monitor(struct work_struct *work)
> @@ -1597,8 +1621,13 @@ void bond_alb_monitor(struct work_struct *work)
>   	if (atomic_read(&bond_info->tx_rebalance_counter) >= BOND_TLB_REBALANCE_TICKS) {
>   		bond_for_each_slave_rcu(bond, slave, iter) {
>   			tlb_clear_slave(bond, slave, 1);
> -			if (slave == rcu_access_pointer(bond->curr_active_slave))
> -				SLAVE_TLB_INFO(slave).load = reset_unbalanced_load(bond_info);
> +			if (slave == rcu_access_pointer(bond->curr_active_slave)) {
> +				u64 new_load = reset_unbalanced_load(bond_info);
> +
> +				spin_lock_bh(&bond->mode_lock);
> +				SLAVE_TLB_INFO(slave).load = new_load;
> +				spin_unlock_bh(&bond->mode_lock);
> +			}
>   		}
>   		atomic_set(&bond_info->tx_rebalance_counter, 0);
>   	}
> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
> index 6fb09b4fc7e2..32f1981033e4 100644
> --- a/include/net/bond_alb.h
> +++ b/include/net/bond_alb.h
> @@ -57,12 +57,12 @@ struct tlb_client_info {
>   				 * packets to a Client that the Hash function
>   				 * gave this entry index.
>   				 */
> -	u32 tx_bytes;		/* Each Client accumulates the BytesTx that
> +	u64 tx_bytes;		/* Each Client accumulates the BytesTx that
>   				 * were transmitted to it, and after each
>   				 * CallBack the LoadHistory is divided
>   				 * by the balance interval
>   				 */
> -	u32 load_history;	/* This field contains the amount of Bytes
> +	u64 load_history;	/* This field contains the amount of Bytes
>   				 * that were transmitted to this client by
>   				 * the server on the previous balance
>   				 * interval in Bps.
> @@ -118,19 +118,20 @@ struct tlb_slave_info {
>   			 * are the entries that were assigned to use this
>   			 * slave for transmit.
>   			 */
> -	u32 load;	/* Each slave sums the loadHistory of all clients
> +	u64 load;	/* Each slave sums the loadHistory of all clients
>   			 * assigned to it
>   			 */
>   };
>   
>   struct unbalanced_load_stats {
> -	u32			tx_bytes;
> +	u64_stats_t		tx_bytes;
> +	struct u64_stats_sync	syncp;
>   };
>   
>   struct alb_bond_info {
>   	struct tlb_client_info	*tx_hashtbl; /* Dynamically allocated */
>   	struct unbalanced_load_stats __percpu	*unbalanced_load;
> -	u32			prev_total_unbalanced;
> +	u64			prev_total_unbalanced;
>   	atomic_t		tx_rebalance_counter;
>   	int			lp_counter;
>   	/* -------- rlb parameters -------- */
> 

Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Hangbin Liu]

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

Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Nikolay Aleksandrov]

On 21/08/2026 13:42, Hangbin Liu wrote:
> 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.

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
have to mark unknown speed with S64_MIN but it will compute the correct numbers
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.

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

It doesn't to me, it still doesn't differentiate between NICs that are
overloaded differently.

> 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

Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Hangbin Liu]

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.

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

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

That is why I believe comparing gaps for NICs with unknown speed is meaningless.

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

Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

[Nikolay Aleksandrov]

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.