Re: [PATCH 1/4] memcg, mm: introduce lowlimit reclaim

[Johannes Weiner]

On Fri, May 02, 2014 at 02:07:15PM +0200, Michal Hocko wrote:
> On Fri 02-05-14 11:36:28, Michal Hocko wrote:
> > On Wed 30-04-14 18:55:50, Johannes Weiner wrote:
> > > On Mon, Apr 28, 2014 at 02:26:42PM +0200, Michal Hocko wrote:
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index 19d620b3d69c..40e517630138 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -2808,6 +2808,29 @@ static struct mem_cgroup *mem_cgroup_lookup(unsigned short id)
> > > >  	return mem_cgroup_from_id(id);
> > > >  }
> > > >  
> > > > +/**
> > > > + * mem_cgroup_reclaim_eligible - checks whether given memcg is eligible for the
> > > > + * reclaim
> > > > + * @memcg: target memcg for the reclaim
> > > > + * @root: root of the reclaim hierarchy (null for the global reclaim)
> > > > + *
> > > > + * The given group is reclaimable if it is above its low limit and the same
> > > > + * applies for all parents up the hierarchy until root (including).
> > > > + */
> > > > +bool mem_cgroup_reclaim_eligible(struct mem_cgroup *memcg,
> > > > +		struct mem_cgroup *root)
> > > 
> > > Could you please rename this to something that is more descriptive in
> > > the reclaim callsite?  How about mem_cgroup_within_low_limit()?
> > 
> > I have intentionally used somethig that is not low_limit specific. The
> > generic reclaim code does't have to care about the reason why a memcg is
> > not reclaimable. I agree that having follow_low_limit paramter explicit
> > and mem_cgroup_reclaim_eligible not is messy. So something should be
> > renamed. I would probably go with s@follow_low_limit@check_reclaim_eligible@
> > but I do not have a strong preference.
> 
> What about this?

I really don't like it.

Yes, we should be hiding implementation details, but we should stop
treating memcg like an alien in this code.  The VM code obviously
doesn't have to know HOW the guarantees are exactly implemented, but
it's a perfectly fine *concept* that can be known outside of memcg:

shrink_zone:
for each memcg in system:
  if mem_cgroup_within_guarantee(memcg):
    continue
  reclaim(memcg-zone)

is perfectly understandable and makes it easier to reason about the
behavior of the reclaim code.  If I just see !mem_cgroup_eligible(), I
don't know if this affects the scenario I'm thinking about at all.

It's obscuring useful information for absolutely no benefit.  If you
burden the reclaim code with a callback, you better explain what you
are doing.  You owe it to the reader.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/