netdev - Re: [PATCH net-next v5 2/3] sock: Always take memcg pressure into consideration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-LFQRreWq1RMkvLw9Nj3NQpJwbDSCfECUhh-aVchR-jsg@mail.gmail.com>
Date: Sun, 4 Jun 2023 12:36:25 +0200
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Shakeel Butt <shakeelb@...gle.com>
Cc: Abel Wu <wuyun.abel@...edance.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Johannes Weiner <hannes@...xchg.org>, Michal Hocko <mhocko@...nel.org>, 
	Vladimir Davydov <vdavydov.dev@...il.com>, Muchun Song <muchun.song@...ux.dev>, 
	Simon Horman <simon.horman@...igine.com>, netdev@...r.kernel.org, linux-mm@...ck.org, 
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/3] sock: Always take memcg pressure into consideration

On Fri, Jun 2, 2023 at 10:42 PM Shakeel Butt <shakeelb@...gle.com> wrote:
>
> On Fri, Jun 02, 2023 at 04:11:34PM +0800, Abel Wu wrote:
> > The sk_under_memory_pressure() is called to check whether there is
> > memory pressure related to this socket. But now it ignores the net-
> > memcg's pressure if the proto of the socket doesn't care about the
> > global pressure, which may put burden on its memcg compaction or
> > reclaim path (also remember that socket memory is un-reclaimable).
> >
> > So always check the memcg's vm status to alleviate memstalls when
> > it's in pressure.
> >
>
> This is interesting. UDP is the only protocol which supports memory
> accounting (i.e. udp_memory_allocated) but it does not define
> memory_pressure. In addition, it does have sysctl_udp_mem. So
> effectively UDP supports a hard limit and ignores memcg pressure at the
> moment. This patch will change its behavior to consider memcg pressure
> as well. I don't have any objection but let's get opinion of UDP
> maintainer.

Others have more experience with memory pressure on UDP, for the
record. Paolo worked on UDP memory pressure in
https://lore.kernel.org/netdev/cover.1579281705.git.pabeni@redhat.com/

It does seem odd to me to modify sk_under_memory_pressure only. See
for instance its use in __sk_mem_raise_allocated:

        if (sk_has_memory_pressure(sk)) {
                u64 alloc;

                if (!sk_under_memory_pressure(sk))
                        return 1;

This is not even reached as sk_has_memory_pressure is false for UDP.
So this commit only affects the only other protocol-independent
caller, __sk_mem_reduce_allocated, to possibly call
sk_leave_memory_pressure if now under the global limit.

What is the expected behavioral change in practice of this commit?


> > Signed-off-by: Abel Wu <wuyun.abel@...edance.com>
> > ---
> >  include/net/sock.h | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 3f63253ee092..ad1895ffbc4a 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1411,13 +1411,11 @@ static inline bool sk_has_memory_pressure(const struct sock *sk)
> >
> >  static inline bool sk_under_memory_pressure(const struct sock *sk)
> >  {
> > -     if (!sk->sk_prot->memory_pressure)
> > -             return false;
> > -
> >       if (mem_cgroup_under_socket_pressure(sk->sk_memcg))
> >               return true;
> >
> > -     return !!*sk->sk_prot->memory_pressure;
> > +     return sk->sk_prot->memory_pressure &&
> > +             *sk->sk_prot->memory_pressure;
> >  }
> >
> >  static inline long
> > --
> > 2.37.3
> >