Re: [PATCH -mmotm 2/4] cgroups: remove events before destroying subsystem state objects

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Sat, Feb 20, 2010 at 6:22 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Sat, 20 Feb 2010 00:28:17 +0200
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
>
>> Events should be removed after rmdir of cgroup directory, but before
>> destroying subsystem state objects. Let's take reference to cgroup
>> directory dentry to do that.
>>
>> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
>
> Okay, I welcome this.
>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hioryu@jp.fujitsu.com>
>
> Just a quesion...After this change, if cgroup has remaining event,
> cgroup is removed by workqueue of event->remove() -> d_put(), finally. Right ?

Yes

> Do you have a test set for checking this behavior ?

Run ./cgroup_event_listener to listen any event in the cgroup. Remove directory
of the cgroup (cgroup_event_listener will detect it). And check 'num_cgroups'
column in /proc/cgroups.

>
> Thanks,
> -Kame
>
>
>
>> ---
>>  include/linux/cgroup.h |    3 ---
>>  kernel/cgroup.c        |    8 ++++++++
>>  mm/memcontrol.c        |    9 ---------
>>  3 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 64cebfe..1719c75 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -395,9 +395,6 @@ struct cftype {
>>        * closes the eventfd or on cgroup removing.
>>        * This callback must be implemented, if you want provide
>>        * notification functionality.
>> -      *
>> -      * Be careful. It can be called after destroy(), so you have
>> -      * to keep all nesessary data, until all events are removed.
>>        */
>>       int (*unregister_event)(struct cgroup *cgrp, struct cftype *cft,
>>                       struct eventfd_ctx *eventfd);
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 46903cb..d142524 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -2979,6 +2979,7 @@ static void cgroup_event_remove(struct work_struct *work)
>>
>>       eventfd_ctx_put(event->eventfd);
>>       kfree(event);
>> +     dput(cgrp->dentry);
>>  }
>>
>>  /*
>> @@ -3099,6 +3100,13 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
>>               goto fail;
>>       }
>>
>> +     /*
>> +      * Events should be removed after rmdir of cgroup directory, but before
>> +      * destroying subsystem state objects. Let's take reference to cgroup
>> +      * directory dentry to do that.
>> +      */
>> +     dget(cgrp->dentry);
>> +
>>       spin_lock(&cgrp->event_list_lock);
>>       list_add(&event->list, &cgrp->event_list);
>>       spin_unlock(&cgrp->event_list_lock);
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index a443c30..8fe6e7f 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3358,12 +3358,6 @@ static int mem_cgroup_register_event(struct cgroup *cgrp, struct cftype *cft,
>>               }
>>       }
>>
>> -     /*
>> -      * We need to increment refcnt to be sure that all thresholds
>> -      * will be unregistered before calling __mem_cgroup_free()
>> -      */
>> -     mem_cgroup_get(memcg);
>> -
>>       if (type == _MEM)
>>               rcu_assign_pointer(memcg->thresholds, thresholds_new);
>>       else
>> @@ -3457,9 +3451,6 @@ assign:
>>       /* To be sure that nobody uses thresholds before freeing it */
>>       synchronize_rcu();
>>
>> -     for (i = 0; i < thresholds->size - size; i++)
>> -             mem_cgroup_put(memcg);
>> -
>>       kfree(thresholds);
>>  unlock:
>>       mutex_unlock(&memcg->thresholds_lock);
>> --
>> 1.6.6.2
>>
>>
>
>

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