Messages in this thread |  | | Date | Wed, 10 May 2023 22:35:25 +0800 | | Subject | Re: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure() | | From | Abel Wu <> |
| |
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
|  |