From: Abel Wu <wuyun.abel@bytedance.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
Shakeel Butt <shakeelb@google.com>,
Paolo Abeni <pabeni@redhat.com>
Cc: "David S . Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
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
Subject: Re: Re: [PATCH net-next v5 2/3] sock: Always take memcg pressure into consideration
Date: Mon, 5 Jun 2023 11:44:55 +0800 [thread overview]
Message-ID: <6f67c3ca-5e73-d7ac-f32a-42a21d3ea576@bytedance.com> (raw)
In-Reply-To: <CAF=yD-LFQRreWq1RMkvLw9Nj3NQpJwbDSCfECUhh-aVchR-jsg@mail.gmail.com>
On 6/4/23 6:36 PM, Willem de Bruijn wrote:
> On Fri, Jun 2, 2023 at 10:42 PM 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.
I intended to make __sk_mem_raise_allocated() be aware of net-memcg
pressure instead of just this bit [1][2].
[1]
https://lore.kernel.org/lkml/20230523094652.49411-5-wuyun.abel@bytedance.com/
[2]
https://lore.kernel.org/lkml/20230523094652.49411-6-wuyun.abel@bytedance.com/
And TBH I am wondering why considering memcg's pressure here, as the
main part in this if statement is to allow the sockets that are below
average memory usage to raise from a *global* memory view, which seems
nothing to do with memcg.
> 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?
Be more conservative on sockmem alloc if under memcg pressure, to
avoid worse memstall/latency.
>
>
>>> 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
>>>
next prev parent reply other threads:[~2023-06-05 3:45 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-02 8:11 [PATCH net-next v5 0/3] sock: Improve condition on sockmem pressure Abel Wu
2023-06-02 8:11 ` [PATCH net-next v5 1/3] net-memcg: Fold dependency into memcg pressure cond Abel Wu
2023-06-02 20:25 ` Shakeel Butt
2023-06-05 11:52 ` Abel Wu
2023-06-02 8:11 ` [PATCH net-next v5 2/3] sock: Always take memcg pressure into consideration Abel Wu
2023-06-02 20:41 ` Shakeel Butt
2023-06-04 10:36 ` Willem de Bruijn
2023-06-05 3:44 ` Abel Wu [this message]
2023-06-05 8:27 ` Paolo Abeni
2023-06-05 9:57 ` Abel Wu
2023-06-02 8:11 ` [PATCH net-next v5 3/3] sock: Fix misuse of sk_under_memory_pressure() Abel Wu
2023-06-02 20:53 ` Shakeel Butt
2023-06-05 8:34 ` Abel Wu
2023-06-06 8:39 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f67c3ca-5e73-d7ac-f32a-42a21d3ea576@bytedance.com \
--to=wuyun.abel@bytedance.com \
--cc=cgroups@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hannes@cmpxchg.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=muchun.song@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=shakeelb@google.com \
--cc=simon.horman@corigine.com \
--cc=vdavydov.dev@gmail.com \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).