| Message ID | 20230523094652.49411-5-wuyun.abel@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sock: Improve condition on sockmem pressure | expand |
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
On 5/23/23 6:26 PM, Paolo Abeni wrote: > 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)) Yes, this condition stands means the global socket memory is absolutely under pressure, and the status is sustained until @allocated falls down to sk_prot_mem_limits(sk, 0). I see some places in the source tree call it 'soft pressure' if usage between index [0] and [1]. The idea behind this patch is to allow the socket memory to raise if there is no pressure neither in global nor net-memcg. With the condition @allocated > sk_prot_mem_limits(sk, 0) we can't be sure whether there is pressure or not in global. And this also aligns with the original logic if net-memcg is not used. I am thinking changing the name of this variable to @might_pressured or something to better illustrate the status of memory pressure. What do you think? > > 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. IIUC the failure can only be due to its memcg under vmpressure. In this case allowing the allocation would burden the mm subsys with increased fragmented unmovable/unreclaimable memory. Thanks & Best, Abel
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; + + /* No pressure observed in global/memcg. */ + if (!pressured) return 1; - } /* Under pressure. */ if (allocated > sk_prot_mem_limits(sk, 1))
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(-)