Re: [PATCH RESEND net-next 1/2] net-memcg: Scopify the indicators of sockmem pressure

[Roman Gushchin]

On Tue, Jul 11, 2023 at 08:41:43PM +0800, Abel Wu wrote:
> Now there are two indicators of socket memory pressure sit inside
> struct mem_cgroup, socket_pressure and tcpmem_pressure.

Hi Abel!

> When in legacy mode aka. cgroupv1, the socket memory is charged
> into a separate counter memcg->tcpmem rather than ->memory, so
> the reclaim pressure of the memcg has nothing to do with socket's
> pressure at all.

But we still might set memcg->socket_pressure and propagate the pressure,
right?
If you're changing this, you need to provide a bit more data on why it's
a good idea. I'm not saying the current status is perfect, but I think we need
a bit more justification for this change.

> While for default mode, the ->tcpmem is simply
> not used.
> 
> So {socket,tcpmem}_pressure are only used in default/legacy mode
> respectively. This patch fixes the pieces of code that make mixed
> use of both.
> 
> Signed-off-by: Abel Wu <wuyun.abel@xxxxxxxxxxxxx>
> ---
>  include/linux/memcontrol.h | 4 ++--
>  mm/vmpressure.c            | 8 ++++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 5818af8eca5a..5860c7f316b9 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1727,8 +1727,8 @@ void mem_cgroup_sk_alloc(struct sock *sk);
>  void mem_cgroup_sk_free(struct sock *sk);
>  static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
>  {
> -	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_pressure)
> -		return true;
> +	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> +		return !!memcg->tcpmem_pressure;

So here you can have something like
   if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
        do {
            if (time_before(jiffies, READ_ONCE(memcg->socket_pressure)))
                  return true;
        } while ((memcg = parent_mem_cgroup(memcg)));
   } else {
	return !!READ_ONCE(memcg->socket_pressure);
   }

And, please, add a bold comment here or nearby the socket_pressure definition
that it has a different semantics in the legacy and default modes.

Overall I think it's a good idea to clean these things up and thank you
for working on this. But I wonder if we can make the next step and leave only
one mechanism for both cgroup v1 and v2 instead of having this weird setup
where memcg->socket_pressure is set differently from different paths on cgroup
v1 and v2.

Thanks!