Re: [PATCH 3/5] Memory controller soft limit organize cgroups (v7)

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-03-25 14:39:53]:

> On Wed, 25 Mar 2009 10:59:47 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-03-25 13:59:00]:
> 
> > > > @@ -75,7 +85,8 @@ void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
> > > >  	counter->usage -= val;
> > > >  }
> > > >  
> > > > -void res_counter_uncharge(struct res_counter *counter, unsigned long val)
> > > > +void res_counter_uncharge(struct res_counter *counter, unsigned long val,
> > > > +				bool *was_soft_limit_excess)
> > > >  {
> > > >  	unsigned long flags;
> > > >  	struct res_counter *c;
> > > > @@ -83,6 +94,9 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
> > > >  	local_irq_save(flags);
> > > >  	for (c = counter; c != NULL; c = c->parent) {
> > > >  		spin_lock(&c->lock);
> > > > +		if (c == counter && was_soft_limit_excess)
> > > > +			*was_soft_limit_excess =
> > > > +				!res_counter_soft_limit_check_locked(c);
> > > >  		res_counter_uncharge_locked(c, val);
> > > >  		spin_unlock(&c->lock);
> > > >  	}
> > > Does this work as intended ?
> > > Assume following hierarchy
> > > 
> > >    A/  softlimit=1G usage=300M
> > >      B/ softlimit=200M usage=300M.
> > >      C/ softlimit=800M usage=0M
> > > 
> > > *was_soft_limit_excess will be false and no tree update, forever.
> > >
> > 
> > No.. was_soft_limit_excess checks the soft limit before uncharge to
> > see if we were over soft limit, when a page gets uncharged from B,
> > since B is over soft limit and on tree, we will update the tree. Why
> > do you say that was_soft_limit_excess will return false? 
> > 
> my eyes tend to be buggy. ok, change the question.
>

No problem, we've all been there :)
 
> ==
> +void res_counter_uncharge(struct res_counter *counter, unsigned long val,
> +				bool *was_soft_limit_excess)
>  {
>  	unsigned long flags;
>  	struct res_counter *c;
> @@ -83,6 +94,9 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
>  	local_irq_save(flags);
>  	for (c = counter; c != NULL; c = c->parent) {
>  		spin_lock(&c->lock);
> +		if (c == counter && was_soft_limit_excess)
> +			*was_soft_limit_excess =
> +				!res_counter_soft_limit_check_locked(c);
>  		res_counter_uncharge_locked(c, val);
>  		spin_unlock(&c->lock);
>  	}
> ==
> Why just check "c == coutner" case is enough ?
> 

This is a very good question, I think this check might not be
necessary and can also be potentially buggy.

> Thanks,
> -Kame
> 
> 
> 
> 
> 

-- 
	Balbir

--
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>