Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()

[Abel Wu]

Gentle ping :)

On 5/10/23 10:35 PM, Abel Wu wrote:
> Hi Paolo, thanks very much for comment!
>
> On 5/9/23 3:52 PM, Paolo Abeni wrote:
> > On Sat, 2023-05-06 at 16:59 +0800, Abel Wu wrote:
> > > The commit 180d8cd942ce ("foundations of per-cgroup memory pressure
> > > controlling") wrapped proto::memory_pressure status into an accessor
> > > named sk_under_memory_pressure(), and in the next commit e1aab161e013
> > > ("socket: initial cgroup code") added the consideration of net-memcg
> > > pressure into this accessor.
> > >
> > > But with the former patch applied, not all of the call sites of
> > > sk_under_memory_pressure() are interested in net-memcg's pressure.
> > > The __sk_mem_{raise,reduce}_allocated() only focus on proto/netns
> > > pressure rather than net-memcg's.
> >
> > Why do you state the above? The current behavior is established since
> > ~12y, arguably we can state quite the opposite.
> >
> > I think this patch should at least target net-next, and I think we need
> > a more detailed reasoning to introduce such behavior change.
>
> Sorry for failed to provide a reasonable explanation... When @allocated
> is no more than tcp_mem[0], the global tcp_mem pressure is gone even if
> the socket's memcg is under pressure.
>
> This reveals that prot::memory_pressure only considers the global tcp
> memory pressure, and is irrelevant to the memcg's. IOW if we're updating
> prot::memory_pressure or making desicions upon prot::memory_pressure,
> the memcg stat should not be considered and sk_under_memory_pressure()
> should not be called since it considers both.
>
> > > IOW this accessor are generally
> > > used for deciding whether should reclaim or not.
> > >
> > > Fixes: e1aab161e013 ("socket: initial cgroup code")
> > > Signed-off-by: Abel Wu <wuyun.abel@xxxxxxxxxxxxx>
> > > ---
> > >   include/net/sock.h |  5 -----
> > >   net/core/sock.c    | 17 +++++++++--------
> > >   2 files changed, 9 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > index 8b7ed7167243..752d51030c5a 100644
> > > --- a/include/net/sock.h
> > > +++ b/include/net/sock.h
> > > @@ -1404,11 +1404,6 @@ static inline int 
> > > sk_under_cgroup_hierarchy(struct sock *sk,
> > >   #endif
> > >   }
> > > -static inline bool sk_has_memory_pressure(const struct sock *sk)
> > > -{
> > > -    return sk->sk_prot->memory_pressure != NULL;
> > > -}
> > > -
> > >   static inline bool sk_under_memory_pressure(const struct sock *sk)
> > >   {
> > >       if (!sk->sk_prot->memory_pressure)
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index 5440e67bcfe3..8d215f821ea6 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> > > @@ -3017,13 +3017,14 @@ int __sk_mem_raise_allocated(struct sock *sk, 
> > > int size, int amt, int kind)
> > >           }
> > >       }
> > > -    if (sk_has_memory_pressure(sk)) {
> > > -        u64 alloc;
> > > -
> > > -        if (!sk_under_memory_pressure(sk))
> > > -            return 1;
> > > -        alloc = sk_sockets_allocated_read_positive(sk);
> > > -        if (sk_prot_mem_limits(sk, 2) > alloc *
> > > +    if (prot->memory_pressure) {
> > > +        /*
> > > +         * If under global pressure, allow the sockets that are below
> > > +         * average memory usage to raise, trying to be fair between all
> > > +         * the sockets under global constrains.
> > > +         */
> > > +        if (!*prot->memory_pressure ||
> > > +            sk_prot_mem_limits(sk, 2) > 
> > > sk_sockets_allocated_read_positive(sk) *
> >
> > The above introduces unrelated changes that makes the code IMHO less
> > readable - I don't see a good reason to drop the 'alloc' variable.
> Besides drop the @alloc variable, this change also removes the condition
> of memcg's pressure from sk_under_memory_pressure() due to the reason
> aforementioned. I can re-introduce @alloc in the next version if you
> think it makes code more readable.
>
> Thanks & Best,
>      Abel