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

[Yizhou Zhao]

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 <ja@xxxxxx> wrote:
> 
> 
> Hello,
> 
> On Wed, 8 Jul 2026, Yizhou Zhao wrote:
> 
>>> On Jul 8, 2026, at 03:18, Julian Anastasov <ja@xxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx/
>> 
>> 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 <ja@xxxxxx>

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