Re: [PATCH nf] ipvs: make destination flags atomic

[Julian Anastasov <ja@ssi.bg>]

	Hello,

On Wed, 8 Jul 2026, Yizhou Zhao wrote:

> > On Jul 8, 2026, at 03:18, Julian Anastasov <ja@ssi.bg> wrote:
> > 
> > On Tue, 7 Jul 2026, Yizhou Zhao wrote:
> > 
> 
> We have posted a v2 patch at:
> https://lore.kernel.org/netfilter-devel/20260708060454.20534-1-zhaoyz24@mails.tsinghua.edu.cn/
> 
> The v2 patch updates the commit message with more conservative
> wording, and fixes the checkpatch logical-continuation warnings.

	After looking again at the code, I think we can
do it in different way:

- IP_VS_DEST_F_AVAILABLE and IP_VS_DEST_F_OVERLOAD are defined
in include/uapi/linux/ip_vs.h but we never export them to user
space. So, we are free to change them. We can move them to 
include/net/ip_vs.h, see below...

- IP_VS_DEST_F_AVAILABLE is changed only under service_mutex,
so we can keep its usage

- IP_VS_DEST_F_OVERLOAD needs different access methods.
We can add 'unsigned long flags2;', may be after l_threshold.
And to switch to such usage (F_OVERLOAD -> FL_OVERLOAD):

	- test_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2)
	- set_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2)

		Sometimes if (test_bit()) clear_bit() can avoid
		full memory barrier in ip_vs_dest_update_overload()

	- clear_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2)
		test_bit() guard can help here too

	As there are other races involved, something like
this can be a starting point for such change. It tries harder
to update the overload flag on dest edit/add but it does not
include the proposed bitops:

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 49297fec448a..b34631270e24 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -1906,6 +1906,8 @@ static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
 		kfree(dest);
 }
 
+void ip_vs_dest_update_overload(struct ip_vs_dest *dest);
+
 /* IPVS sync daemon data and function prototypes
  * (from ip_vs_sync.c)
  */
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index d19caf66afeb..3fd221996e6e 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -1087,6 +1087,26 @@ static inline int ip_vs_dest_totalconns(struct ip_vs_dest *dest)
 		+ atomic_read(&dest->inactconns);
 }
 
+__always_inline void ip_vs_dest_update_overload(struct ip_vs_dest *dest)
+{
+	int conns, l, u;
+
+	u = READ_ONCE(dest->u_threshold);
+	if (!u)
+		goto unset;
+	conns = ip_vs_dest_totalconns(dest);
+	if (conns >= u) {
+		dest->flags |= IP_VS_DEST_F_OVERLOAD;
+		return;
+	}
+	l = READ_ONCE(dest->l_threshold) ? : (u * 3 / 4);
+	if (conns >= l && l)
+		return;
+
+unset:
+	dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
+}
+
 /*
  *	Bind a connection entry with a virtual service destination
  *	Called just after a new connection entry is created.
@@ -1161,9 +1181,7 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_vs_dest *dest)
 		atomic_inc(&dest->persistconns);
 	}
 
-	if (dest->u_threshold != 0 &&
-	    ip_vs_dest_totalconns(dest) >= dest->u_threshold)
-		dest->flags |= IP_VS_DEST_F_OVERLOAD;
+	ip_vs_dest_update_overload(dest);
 }
 
 
@@ -1257,16 +1275,8 @@ static inline void ip_vs_unbind_dest(struct ip_vs_conn *cp)
 		atomic_dec(&dest->persistconns);
 	}
 
-	if (dest->l_threshold != 0) {
-		if (ip_vs_dest_totalconns(dest) < dest->l_threshold)
-			dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
-	} else if (dest->u_threshold != 0) {
-		if (ip_vs_dest_totalconns(dest) * 4 < dest->u_threshold * 3)
-			dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
-	} else {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
-			dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
-	}
+	if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		ip_vs_dest_update_overload(dest);
 
 	ip_vs_dest_put(dest);
 }
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index bcf40b8c41cf..2871116e46ec 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -1315,6 +1315,7 @@ __ip_vs_update_dest(struct ip_vs_service *svc, struct ip_vs_dest *dest,
 	struct ip_vs_service *old_svc;
 	struct ip_vs_scheduler *sched;
 	int conn_flags;
+	bool upd_thresh;
 
 	/* We cannot modify an address and change the address family */
 	BUG_ON(!add && udest->af != dest->af);
@@ -1370,10 +1371,12 @@ __ip_vs_update_dest(struct ip_vs_service *svc, struct ip_vs_dest *dest,
 	/* set the dest status flags */
 	dest->flags |= IP_VS_DEST_F_AVAILABLE;
 
-	if (udest->u_threshold == 0 || udest->u_threshold > dest->u_threshold)
-		dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
-	dest->u_threshold = udest->u_threshold;
-	dest->l_threshold = udest->l_threshold;
+	upd_thresh = READ_ONCE(dest->u_threshold) != udest->u_threshold ||
+		     READ_ONCE(dest->l_threshold) != udest->l_threshold;
+	WRITE_ONCE(dest->u_threshold, udest->u_threshold);
+	WRITE_ONCE(dest->l_threshold, udest->l_threshold);
+	if (upd_thresh)
+		ip_vs_dest_update_overload(dest);
 
 	dest->af = udest->af;
 
@@ -3667,8 +3670,8 @@ __ip_vs_get_dest_entries(struct netns_ipvs *ipvs, const struct ip_vs_get_dests *
 			entry.port = dest->port;
 			entry.conn_flags = atomic_read(&dest->conn_flags);
 			entry.weight = atomic_read(&dest->weight);
-			entry.u_threshold = dest->u_threshold;
-			entry.l_threshold = dest->l_threshold;
+			entry.u_threshold = READ_ONCE(dest->u_threshold);
+			entry.l_threshold = READ_ONCE(dest->l_threshold);
 			entry.activeconns = atomic_read(&dest->activeconns);
 			entry.inactconns = atomic_read(&dest->inactconns);
 			entry.persistconns = atomic_read(&dest->persistconns);
@@ -4277,8 +4280,10 @@ static int ip_vs_genl_fill_dest(struct sk_buff *skb, struct ip_vs_dest *dest)
 			 dest->tun_port) ||
 	    nla_put_u16(skb, IPVS_DEST_ATTR_TUN_FLAGS,
 			dest->tun_flags) ||
-	    nla_put_u32(skb, IPVS_DEST_ATTR_U_THRESH, dest->u_threshold) ||
-	    nla_put_u32(skb, IPVS_DEST_ATTR_L_THRESH, dest->l_threshold) ||
+	    nla_put_u32(skb, IPVS_DEST_ATTR_U_THRESH,
+			READ_ONCE(dest->u_threshold)) ||
+	    nla_put_u32(skb, IPVS_DEST_ATTR_L_THRESH,
+			READ_ONCE(dest->l_threshold)) ||
 	    nla_put_u32(skb, IPVS_DEST_ATTR_ACTIVE_CONNS,
 			atomic_read(&dest->activeconns)) ||
 	    nla_put_u32(skb, IPVS_DEST_ATTR_INACT_CONNS,

Regards

--
Julian Anastasov <ja@ssi.bg>