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: <d2abfe0c-0152-860c-60f7-2787973c95d0@bytedance.com>
Date:   Wed, 10 May 2023 22:35:25 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.1
Subject: Re: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()
To:     Paolo Abeni <pabeni@redhat.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
References: <20230506085903.96133-1-wuyun.abel@bytedance.com>
 <588689343dcd6c904e7fc142a001043015e5b14e.camel@redhat.com>
Content-Language: en-US
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <588689343dcd6c904e7fc142a001043015e5b14e.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Paolo, thanks very much for comment!

On 5/9/23 3:52 PM, Paolo Abeni wrote:
> 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.
>>
>> 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.
> 
> 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.

Sorry for failed to provide a reasonable explanation... When @allocated
is no more than tcp_mem[0], the global tcp_mem pressure is gone even if
the socket's memcg is under pressure.

This reveals that prot::memory_pressure only considers the global tcp
memory pressure, and is irrelevant to the memcg's. IOW if we're updating
prot::memory_pressure or making desicions upon prot::memory_pressure,
the memcg stat should not be considered and sk_under_memory_pressure()
should not be called since it considers both.

> 
>> IOW this accessor are generally
>> used for deciding whether should reclaim or not.
>>
>> 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(-)
>>
>> 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
>>   }
>>   
>> -static inline bool sk_has_memory_pressure(const struct sock *sk)
>> -{
>> -	return sk->sk_prot->memory_pressure != 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)
>>   		}
>>   	}
>>   
>> -	if (sk_has_memory_pressure(sk)) {
>> -		u64 alloc;
>> -
>> -		if (!sk_under_memory_pressure(sk))
>> -			return 1;
>> -		alloc = 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.
Besides drop the @alloc variable, this change also removes the condition
of memcg's pressure from sk_under_memory_pressure() due to the reason
aforementioned. I can re-introduce @alloc in the next version if you
think it makes code more readable.

Thanks & Best,
	Abel