Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230602081135.75424-1-wuyun.abel@bytedance.com>
 <20230602081135.75424-3-wuyun.abel@bytedance.com> <20230602204159.vo7fmuvh3y2pdfi5@google.com>
In-Reply-To: <20230602204159.vo7fmuvh3y2pdfi5@google.com>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Sun, 4 Jun 2023 12:36:25 +0200
Message-ID: <CAF=yD-LFQRreWq1RMkvLw9Nj3NQpJwbDSCfECUhh-aVchR-jsg@mail.gmail.com>
Subject: Re: [PATCH net-next v5 2/3] sock: Always take memcg pressure into consideration
To:     Shakeel Butt <shakeelb@google.com>
Cc:     Abel Wu <wuyun.abel@bytedance.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        Muchun Song <muchun.song@linux.dev>,
        Simon Horman <simon.horman@corigine.com>,
        netdev@vger.kernel.org, linux-mm@kvack.org,
        cgroups@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 2, 2023 at 10:42=E2=80=AFPM Shakeel Butt <shakeelb@google.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@bytedance.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
> >