Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <588689343dcd6c904e7fc142a001043015e5b14e.camel@redhat.com>
Subject: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()
From:   Paolo Abeni <pabeni@redhat.com>
To:     Abel Wu <wuyun.abel@bytedance.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 09 May 2023 09:52:28 +0200
In-Reply-To: <20230506085903.96133-1-wuyun.abel@bytedance.com>
References: <20230506085903.96133-1-wuyun.abel@bytedance.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Sat, 2023-05-06 at 16:59 +0800, Abel Wu wrote:
> The commit 180d8cd942ce ("foundations of per-cgroup memory pressure
> controlling") wrapped proto::memory_pressure status into an accessor
> named sk_under_memory_pressure(), and in the next commit e1aab161e013
> ("socket: initial cgroup code") added the consideration of net-memcg
> pressure into this accessor.
>=20
> But with the former patch applied, not all of the call sites of
> sk_under_memory_pressure() are interested in net-memcg's pressure.
> The __sk_mem_{raise,reduce}_allocated() only focus on proto/netns
> pressure rather than net-memcg's.=C2=A0

Why do you state the above? The current behavior is established since
~12y, arguably we can state quite the opposite.

I think this patch should at least target net-next, and I think we need
a more detailed reasoning to introduce such behavior change.

> IOW this accessor are generally
> used for deciding whether should reclaim or not.
>=20
> Fixes: e1aab161e013 ("socket: initial cgroup code")
> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
> ---
>  include/net/sock.h |  5 -----
>  net/core/sock.c    | 17 +++++++++--------
>  2 files changed, 9 insertions(+), 13 deletions(-)
>=20
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 8b7ed7167243..752d51030c5a 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1404,11 +1404,6 @@ static inline int sk_under_cgroup_hierarchy(struct=
 sock *sk,
>  #endif
>  }
> =20
> -static inline bool sk_has_memory_pressure(const struct sock *sk)
> -{
> -	return sk->sk_prot->memory_pressure !=3D NULL;
> -}
> -
>  static inline bool sk_under_memory_pressure(const struct sock *sk)
>  {
>  	if (!sk->sk_prot->memory_pressure)
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 5440e67bcfe3..8d215f821ea6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3017,13 +3017,14 @@ int __sk_mem_raise_allocated(struct sock *sk, int=
 size, int amt, int kind)
>  		}
>  	}
> =20
> -	if (sk_has_memory_pressure(sk)) {
> -		u64 alloc;
> -
> -		if (!sk_under_memory_pressure(sk))
> -			return 1;
> -		alloc =3D sk_sockets_allocated_read_positive(sk);
> -		if (sk_prot_mem_limits(sk, 2) > alloc *
> +	if (prot->memory_pressure) {
> +		/*
> +		 * If under global pressure, allow the sockets that are below
> +		 * average memory usage to raise, trying to be fair between all
> +		 * the sockets under global constrains.
> +		 */
> +		if (!*prot->memory_pressure ||
> +		    sk_prot_mem_limits(sk, 2) > sk_sockets_allocated_read_positive(sk)=
 *

The above introduces unrelated changes that makes the code IMHO less
readable - I don't see a good reason to drop the 'alloc' variable.

Cheers,

Paolo