Re: [PATCH v2 1/2] memcg: Don't generate low/min events if either low/min or elow/emin is 0

[Johannes Weiner]

On Fri, Apr 04, 2025 at 02:55:35PM -0400, Waiman Long wrote:
> On 4/4/25 2:13 PM, Johannes Weiner wrote:
> > * Waiman points out that the weirdness is seeing low events without
> >    having a low configured. Eh, this isn't really true with recursive
> >    propagation; you may or may not have an elow depending on parental
> >    configuration and sibling behavior.
> >
> Do you mind if we just don't update the low event count if low isn't 
> set, but leave the rest the same like

What's the motivation for doing anything beyond the skip-on-!usage?

> @@ -659,21 +659,25 @@ static inline bool mem_cgroup_unprotected(struct 
> mem_cgro>
>   static inline bool mem_cgroup_below_low(struct mem_cgroup *target,
>                                          struct mem_cgroup *memcg)
>   {
> +       unsigned long elow;
> +
>          if (mem_cgroup_unprotected(target, memcg))
>                  return false;
> 
> -       return READ_ONCE(memcg->memory.elow) >=
> -               page_counter_read(&memcg->memory);
> +       elow = READ_ONCE(memcg->memory.elow);
> +       return elow && (page_counter_read(&memcg->memory) <= elow);
>   }
> 
>   static inline bool mem_cgroup_below_min(struct mem_cgroup *target,
>                                          struct mem_cgroup *memcg)
>   {
> +       unsigned long emin;
> +
>          if (mem_cgroup_unprotected(target, memcg))
>                  return false;
> 
> -       return READ_ONCE(memcg->memory.emin) >=
> -               page_counter_read(&memcg->memory);
> +       emin = READ_ONCE(memcg->memory.emin);
> +       return emin && (page_counter_read(&memcg->memory) <= emin);
>   }

This still redefines the empty case to mean excess. That's a quirk I
would have liked to avoid. I don't see why you would need it?

> @@ -5919,7 +5923,8 @@ static void shrink_node_memcgs(pg_data_t *pgdat, 
> struct s>
>                                  sc->memcg_low_skipped = 1;
>                                  continue;
>                          }
> -                       memcg_memory_event(memcg, MEMCG_LOW);
> +                       if (memcg->memory.low)
> +                               memcg_memory_event(memcg, MEMCG_LOW);

That's not right. In setups where protection comes from the parent, no
breaches would ever be counted.