Messages in this thread | | | Subject | Re: [PATCH v3 4/5] sock: Consider memcg pressure when raising sockmem | From | Paolo Abeni <> | Date | Tue, 23 May 2023 12:26:15 +0200 |
| |
On Tue, 2023-05-23 at 17:46 +0800, Abel Wu wrote: > For now __sk_mem_raise_allocated() mainly considers global socket > memory pressure and allows to raise if no global pressure observed, > including the sockets whose memcgs are in pressure, which might > result in longer memcg memstall. > > So take net-memcg's pressure into consideration when allocating > socket memory to alleviate long tail latencies. > > Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> > --- > net/core/sock.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 801df091e37a..b899e0b9feda 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -2976,22 +2976,31 @@ EXPORT_SYMBOL(sk_wait_data); > int __sk_mem_raise_allocated(struct sock *sk, int size, int amt, int kind) > { > bool memcg_charge = mem_cgroup_sockets_enabled && sk->sk_memcg; > + bool charged = true, pressured = false; > struct proto *prot = sk->sk_prot; > - bool charged = true; > long allocated; > > sk_memory_allocated_add(sk, amt); > allocated = sk_memory_allocated(sk); > - if (memcg_charge && > - !(charged = mem_cgroup_charge_skmem(sk->sk_memcg, amt, > - gfp_memcg_charge()))) > - goto suppress_allocation; > + > + if (memcg_charge) { > + charged = mem_cgroup_charge_skmem(sk->sk_memcg, amt, > + gfp_memcg_charge()); > + if (!charged) > + goto suppress_allocation; > + if (mem_cgroup_under_socket_pressure(sk->sk_memcg)) > + pressured = true; > + } > > /* Under limit. */ > - if (allocated <= sk_prot_mem_limits(sk, 0)) { > + if (allocated <= sk_prot_mem_limits(sk, 0)) > sk_leave_memory_pressure(sk); > + else > + pressured = true;
The above looks not correct to me.
allocated > sk_prot_mem_limits(sk, 0)
does not mean the protocol has memory pressure. Such condition is checked later with:
if (allocated > sk_prot_mem_limits(sk, 1))
Here an allocation could fail even if memcg charge is successful and the protocol is not under pressure, which in turn sounds quite (too much?) conservative.
Cheers,
Paolo
| |