Re: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()

[Abel Wu <wuyun.abel@bytedance.com>]

Hi Eric, thanks very much for comments!

On 5/9/23 4:55 PM, Eric Dumazet wrote:
> On Sat, May 6, 2023 at 10:59 AM Abel Wu <wuyun.abel@bytedance.com> 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. 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) {
> 
> I do not understand this patch.
> 
> Changelog is evasive, I do not see what practical problem you want to solve.
> 
> sk_has_memory_pressure() is not about memcg, simply the fact that a
> proto has a non NULL memory_pressure pointer.

Sorry for failed to provide a reasonable explanation... Would you please
check my reply to Paolo?

Thanks,
	Abel