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

KAMEZAWA Hiroyuki
2009-06-18T03:20:51Z

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