Hello Julian, Thank you for the detailed proposal. Yes, I am happy to handle the follow-up conversion of the overload flag to bitops.
> On Jul 8, 2026, at 23:53, Julian Anastasov <[email protected]> wrote: > > > Hello, > > On Wed, 8 Jul 2026, Yizhou Zhao wrote: > >>> On Jul 8, 2026, at 03:18, Julian Anastasov <[email protected]> wrote: >>> >>> On Tue, 7 Jul 2026, Yizhou Zhao wrote: >>> >> >> We have posted a v2 patch at: >> https://lore.kernel.org/netfilter-devel/[email protected]/ >> >> 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; > + I noticed one integer-rounding detail in the proposed helper. The existing default lower-threshold check: ip_vs_dest_totalconns(dest) * 4 < dest->u_threshold * 3 clears OVERLOAD when conns is below ceil(3 * u / 4), assuming the multiplications do not overflow. In the proposed helper: l = u * 3 / 4; if (conns >= l && l) return; the division rounds down, so the boundary is different when u is not a multiple of four. For example, with u == 2 and conns == 1, the existing code clears OVERLOAD while the helper keeps it set. Would using l = u - u / 4; for the default lower threshold preserve the existing behavior while also avoiding the multiplication overflow? > +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 <[email protected]> Please go ahead with the ip_vs_dest_update_overload() patch. I will base the follow-up on the posted version and check the resulting struct ip_vs_dest layout as Vadim suggested. Regards, Yizhou
