Re: [RFC 2/3] memcg: Ignore soft limit until it is explicitly specified

[Michal Hocko <mhocko@suse.cz>]

On Tue 09-04-13 09:24:06, Johannes Weiner wrote:
> On Tue, Apr 09, 2013 at 02:13:14PM +0200, Michal Hocko wrote:
[...]
> > @@ -2062,14 +2066,15 @@ static bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
> >  
> >  /*
> >   * A group is eligible for the soft limit reclaim if it is
> > - * 	a) is over its soft limit
> > - * 	b) any parent up the hierarchy is over its soft limit
> > + * 	a) doesn't have any soft limit set
> > + * 	b) is over its soft limit
> > + * 	c) any parent up the hierarchy is over its soft limit
> >   */
> >  bool mem_cgroup_soft_reclaim_eligible(struct mem_cgroup *memcg)
> >  {
> >  	struct mem_cgroup *parent = memcg;
> >  
> > -	if (res_counter_soft_limit_excess(&memcg->res))
> > +	if (!memcg->soft_limited || res_counter_soft_limit_excess(&memcg->res))
> >  		return true;
> 
> With the very similar condition in the hierarchy walk down there, this
> was more confusing than I would have expected it to be.
> 
> Would you mind splitting this check and putting the comments directly
> over the individual checks?
> 
> 	/* No specific soft limit set, eligible for soft reclaim */
> 	if (!memcg->soft_limited)
> 		return true;
> 
> 	/* Soft limit exceeded, eligible for soft reclaim */
> 	if (res_counter_soft_limit_excess(&memcg->res))
> 		return true;
> 
> 	/* Parental limit exceeded, eligible for... soft reclaim! */

Sure thing.

> 	...
> 
> > @@ -2077,7 +2082,8 @@ bool mem_cgroup_soft_reclaim_eligible(struct mem_cgroup *memcg)
> >  	 * have to obey and reclaim from this group as well.
> >  	 */
> >  	while((parent = parent_mem_cgroup(parent))) {
> > -		if (res_counter_soft_limit_excess(&parent->res))
> > +		if (memcg->soft_limited &&
> > +				res_counter_soft_limit_excess(&parent->res))
> >  			return true;
> 
> Should this be parent->soft_limited instead of memcg->softlimited?

Yes. I haven't tested with deeper hierarchies yet... Thanks for catching
this.

> 
> > @@ -5237,6 +5243,14 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
> >  			ret = res_counter_set_soft_limit(&memcg->res, val);
> >  		else
> >  			ret = -EINVAL;
> > +
> > +		/*
> > +		 * We could disable soft_limited when we get RESOURCE_MAX but
> > +		 * then we have a little problem to distinguish the default
> > +		 * unlimited and limitted but never soft reclaimed groups.
> > +		 */
> > +		if (!ret)
> > +			memcg->soft_limited = true;
> 
> It's neither reversible nor distinguishable from userspace, so it
> would be good to either find a value or just make the soft_limited
> knob explicit and accessible from userspace.

I can export the knob but I would like to prevent from that if possible.
So far it seems it would be hard to keep backward compatibility. I hoped
somebody would come up with something clever ;)

One possible way would be returning -1 if soft_limited == false. Users
who use u64 would see the same value in the end so they shouldn't break
and those that are _really_ interested can check the string value as
well. What do you think?

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>