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

[Abel Wu]

Hi Eric, thanks very much for comments!

On 5/9/23 4:55 PM, Eric Dumazet wrote:
> On Sat, May 6, 2023 at 10:59 AM Abel Wu <wuyun.abel@xxxxxxxxxxxxx> 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. 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) {
>
> I do not understand this patch.
>
> Changelog is evasive, I do not see what practical problem you want to solve.
>
> sk_has_memory_pressure() is not about memcg, simply the fact that a
> proto has a non NULL memory_pressure pointer.

Sorry for failed to provide a reasonable explanation... Would you please
check my reply to Paolo?

Thanks,
	Abel