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

[Johannes Weiner]

On Fri, May 02, 2014 at 04:15:15PM +0200, Michal Hocko wrote:
> On Fri 02-05-14 09:01:18, Johannes Weiner wrote:
> > 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.
> 
> OK fair enough, what about the following?

Thanks, that's much better IMO.

> @@ -2215,8 +2215,18 @@ static inline bool should_continue_reclaim(struct zone *zone,
>  	}
>  }
>  
> +/**
> + * __shrink_zone - shrinks a given zone
> + *
> + * @zone: zone to shrink
> + * @sc: scan control with additional reclaim parameters
> + * @force_memcg_guarantee: do not reclaim memcgs which are within their memory
> + * guarantee
> + *
> + * Returns the number of reclaimed memcgs.
> + */
>  static unsigned __shrink_zone(struct zone *zone, struct scan_control *sc,
> -		bool follow_low_limit)
> +		bool force_memcg_guarantee)
>  {
>  	unsigned long nr_reclaimed, nr_scanned;
>  	unsigned nr_scanned_groups = 0;
> @@ -2236,12 +2246,9 @@ static unsigned __shrink_zone(struct zone *zone, struct scan_control *sc,
>  		do {
>  			struct lruvec *lruvec;
>  
> -			/*
> -			 * Memcg might be under its low limit so we have to
> -			 * skip it during the first reclaim round
> -			 */
> -			if (follow_low_limit &&
> -					!mem_cgroup_reclaim_eligible(memcg, root)) {
> +			/* Memcg might be protected from the reclaim */
> +			if (force_memcg_guarantee &&

respect_?  consider_?

force sounds like something the second round would do -- force reclaim
despite guarantees...  But then again, I'm still for removing that 2nd
force cycle, so I don't care too strongly about that name (yet) :-)
--
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/