lkml.org 
[lkml]   [2023]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()
From
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

\
 
 \ /
  Last update: 2023-05-10 16:38    [from the cache]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog