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

[Waiman Long]

On 4/4/25 3:38 PM, Johannes Weiner wrote:
> 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?
It is to avoid making further change. I am fine with modifying the test 
to allow low event even when low isn't set.
> > @@ -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?
OK, I will drop that.
> > @@ -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.

OK. Will post a v3 to incorporate your suggestion.

Thanks,
Longman