Re: [PATCH net 10/15] ipvs: honor the overload state and do not enter it forever

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's reason for moving ip_vs_dest_set_min() after
  the set_max/erase cleanup in ip_vs_lblcr_schedule()…

--

ipvs: honor the overload state and do not enter it forever

This commit adds an smp_mb() and a re-read of totalconns in
__ip_vs_dest_update_overload(), so a dest cannot stay in the overload
state forever when it races with ip_vs_unbind_dest(). It also drops the
overload parameter from ip_vs_dest_update_overload() and makes LBLC and
LBLCR honor the overload state for cached dests. In LBLCR it moves the
ip_vs_dest_set_min() call to after the set_max/erase cleanup.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019150249.1879516-2-pablo%40netfilter.org

> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
> index dfb5bf83750db..4046608922e7c 100644
> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c

[ ... ]

> @@ -658,9 +659,6 @@ ip_vs_lblcr_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
>  	if (en) {
>  		en->lastuse = jiffies;
>  
> -		/* Get the least loaded destination */
> -		dest = ip_vs_dest_set_min(&en->set);
> -
>  		/* More than one destination + enough time passed by, cleanup */
>  		if (atomic_read(&en->set.size) > 1 &&
>  		    time_after(jiffies, en->set.lastmod +
> @@ -676,6 +674,9 @@ ip_vs_lblcr_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
>  			spin_unlock_bh(&svc->sched_lock);
>  		}
>  
> +		/* Get the least loaded destination */
> +		dest = ip_vs_dest_set_min(&en->set);
> +
>  		/* If the destination is not overloaded, use it */
>  		if (dest && !is_overloaded(dest, svc))
>  			goto out;

[Severity: Low]
This isn't a bug, and the code change looks right. The commit message
explains the move this way:

  "Also, move the ip_vs_dest_set_min() call in LBLCR after the
  ip_vs_dest_set_erase() call that removes the most-loaded
  server because due to races we may work with dest that is
  in process of removing, i.e. ip_vs_dest_set_min() and
  ip_vs_dest_set_max() may return same dest."

Can the two helpers also return the same dest without any race?

ip_vs_dest_set_min() only replaces its candidate when another member is
strictly less loaded:

    if (((__s64)loh * atomic_read(&dest->weight) >
         (__s64)doh * atomic_read(&least->weight)) &&
        (dest->cflags & IP_VS_DEST_CF_AVAILABLE)) {

ip_vs_dest_set_max() only replaces its candidate when another member is
strictly more loaded:

    if (((__s64)moh * atomic_read(&dest->weight) <
         (__s64)doh * atomic_read(&most->weight))
        && (atomic_read(&dest->weight) > 0)) {

Take two weight-1 members with the same ip_vs_dest_conn_overhead(), for
example both idle. Both helpers then return the first list element.

Before this patch, once the set had more than one member and was older
than sysctl_lblcr_expiration, the sequence was:

ip_vs_lblcr_schedule()
    ip_vs_dest_set_min()     -> first member (tie)
    ip_vs_dest_set_max()     -> same first member (tie)
    ip_vs_dest_set_erase()   -> removes it from en->set
    goto out                 -> returns the erased dest

So on load ties the old code returned the dest it had just removed from
the cache set, with no race involved.

Could the commit message say that this also happens on ties, not only
because of races?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org