Re: [RFC][BUGFIX] memcg: rmdir doesn't return

["KAMEZAWA Hiroyuki" <kamezawa.hiroyu@jp.fujitsu.com>]

Daisuke Nishimura wrote:
> On Tue, 16 Jun 2009 17:44:36 +0900, KAMEZAWA Hiroyuki
>> +	/*
>> +	 * css_put/get is provided for subsys to grab refcnt to css. In
>> typical
>> +	 * case, subsystem has no reference after pre_destroy(). But, under
>> +	 * hierarchy management, some *temporal* refcnt can be hold.
>> +	 * To avoid returning -EBUSY to a user, waitqueue is used. If subsys
>> +	 * is really busy, it should return -EBUSY at pre_destroy(). wake_up
>> +	 * is called when css_put() is called and refcnt goes down to 0.
>> +	 *
>> +	 * Subsys can check CGRP_WAIT_ON_RMDIR bit by itself to know
>> +	 * it's under ongoing rmdir() or not. Because css_tryget() returns
>> false
>> +	 * only after css->refcnt returns 0, checking this bit is useful when
>> +	 * css' refcnt seems to be not temporal.
>> +	 */
>> +	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
>> +	prepare_to_wait(&cgroup_rmdir_waitq, &wait, TASK_INTERRUPTIBLE);
>> +
> I'm sorry if I misunderstand something.
>
> Preparing waitq here means force_empty would be called with
> TASK_INTERRUPTIBLE,
> so current can sleep with TASK_INTRRUPTIBLE by cond_resched().
>
Ah...you're right.

> Can we ensure that it can be waken up, especially in case we are not under
> memory pressure ?
>
Hmm. I'll modify here.

lag between
 pre_destroy-> check css's ref -> sleep
 css_tryget -> charge to res_counter
is an enemy anyway. Adding "retry_rmdir()" as previous one is a choice..
(I wonder we should stop css_get/put against page_cgroup ...
 but that change will be too large for bugfix)

>>  	mutex_lock(&cgroup_mutex);
>> -	if (atomic_read(&cgrp->count) != 0) {
>> -		mutex_unlock(&cgroup_mutex);
>> -		return -EBUSY;
>> -	}
>> -	if (!list_empty(&cgrp->children)) {
>> +	if (atomic_read(&cgrp->count) != 0 || !list_empty(&cgrp->children)) {
>>  		mutex_unlock(&cgroup_mutex);
>> +		finish_wait(&cgroup_rmdir_waitq, &wait);
>> +		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
>>  		return -EBUSY;
>>  	}
>>  	mutex_unlock(&cgroup_mutex);
>> @@ -2683,25 +2696,20 @@ again:
>>  	 * that rmdir() request comes.
>>  	 */
>>  	ret = cgroup_call_pre_destroy(cgrp);
>> -	if (ret)
>> +	if (ret) {
>> +		finish_wait(&cgroup_rmdir_waitq, &wait);
>> +		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
>>  		return ret;
>> +	}
>>
>>  	mutex_lock(&cgroup_mutex);
>>  	parent = cgrp->parent;
>>  	if (atomic_read(&cgrp->count) || !list_empty(&cgrp->children)) {
>>  		mutex_unlock(&cgroup_mutex);
>> +		finish_wait(&cgroup_rmdir_waitq, &wait);
>> +		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
>>  		return -EBUSY;
>>  	}
>> -	/*
>> -	 * css_put/get is provided for subsys to grab refcnt to css. In
>> typical
>> -	 * case, subsystem has no reference after pre_destroy(). But, under
>> -	 * hierarchy management, some *temporal* refcnt can be hold.
>> -	 * To avoid returning -EBUSY to a user, waitqueue is used. If subsys
>> -	 * is really busy, it should return -EBUSY at pre_destroy(). wake_up
>> -	 * is called when css_put() is called and refcnt goes down to 0.
>> -	 */
>> -	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
>> -	prepare_to_wait(&cgroup_rmdir_waitq, &wait, TASK_INTERRUPTIBLE);
>>
>>  	if (!cgroup_clear_css_refs(cgrp)) {
>>  		mutex_unlock(&cgroup_mutex);
>> Index: linux-2.6.30.org/mm/memcontrol.c
>> ===================================================================
>> --- linux-2.6.30.org.orig/mm/memcontrol.c
>> +++ linux-2.6.30.org/mm/memcontrol.c
>> @@ -1338,6 +1338,7 @@ __mem_cgroup_commit_charge_swapin(struct
>>  		return;
>>  	if (!ptr)
>>  		return;
>> +	css_get(&ptr->css);
> What's the purpose of this css_get ?
> Can you add a comment ?
>
memcg's css->refcnt can be go down to 0 while commit. So, access to
memcg->css.cgroup can be invalid.


>>  	pc = lookup_page_cgroup(page);
>>  	mem_cgroup_lru_del_before_commit_swapcache(page);
>>  	__mem_cgroup_commit_charge(ptr, pc, ctype);
>> @@ -1367,8 +1368,14 @@ __mem_cgroup_commit_charge_swapin(struct
>>  		}
>>  		rcu_read_unlock();
>>  	}
>> -	/* add this page(page_cgroup) to the LRU we want. */
>> -
>> +	/*
>> +	 * Because we charged against a cgroup which is obtained by record
>> +	 * in swap_cgroup, not by task, there is a possibility that someone is
>> +	 * waiting for rmdir. This happens when a swap entry is shared
>> +	 * among cgroups. After wakeup, pre_destroy() will be called again.
>> +	 */
>> +	cgroup_wakeup_rmdir_waiters(&ptr->css.cgroup);
> '&' must be removed here.
>
maybe reflesh miss, sorry.

Thanks,
-Kame

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