Paul Menage wrote:
quoted text
> On Mon, Mar 24, 2008 at 7:01 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>>  --- linux-2.6.25-rc5/include/linux/mm_types.h~memory-controller-add-mm-owner    2008-03-20 13:35:09.000000000 +0530
>>  +++ linux-2.6.25-rc5-balbir/include/linux/mm_types.h    2008-03-20 15:11:05.000000000 +0530
>>  @@ -228,7 +228,10 @@ struct mm_struct {
>>         rwlock_t                ioctx_list_lock;
>>         struct kioctx           *ioctx_list;
>>   #ifdef CONFIG_CGROUP_MEM_RES_CTLR
>>  -       struct mem_cgroup *mem_cgroup;
>>  +       struct task_struct *owner;      /* The thread group leader that */
>>  +                                       /* owns the mm_struct. This     */
>>  +                                       /* might be useful even outside */
>>  +                                       /* of the config option         */
>>   #endif
> 
> This should probably be controlled by something like a CONFIG_MM_OWNER
> that's selected by any Kconfig option (mem cgroup, etc) that needs
> mm->owner to be maintained.
> 

OK, will do

quoted text
>>  @@ -248,12 +248,40 @@ void mm_init_cgroup(struct mm_struct *mm
>>
>>         mem = mem_cgroup_from_task(p);
>>         css_get(&mem->css);
>>  -       mm->mem_cgroup = mem;
>>  +       mm->owner = p;
>>  +}
>>  +
>>  +void mem_cgroup_fork_init(struct task_struct *p)
>>  +{
>>  +       struct mm_struct *mm = get_task_mm(p);
>>  +       struct mem_cgroup *mem, *oldmem;
>>  +       if (!mm)
>>  +               return;
>>  +
>>  +       /*
>>  +        * Initial owner at mm_init_cgroup() time is the task itself.
>>  +        * The thread group leader had not been setup then
>>  +        */
>>  +       oldmem = mem_cgroup_from_task(mm->owner);
>>  +       /*
>>  +        * Override the mm->owner after we know the thread group later
>>  +        */
>>  +       mm->owner = p->group_leader;
>>  +       mem = mem_cgroup_from_task(mm->owner);
>>  +       css_get(&mem->css);
>>  +       css_put(&oldmem->css);
>>  +       mmput(mm);
>>   }
>>
>>   void mm_free_cgroup(struct mm_struct *mm)
>>   {
>>  -       css_put(&mm->mem_cgroup->css);
>>  +       struct mem_cgroup *mem;
>>  +
>>  +       /*
>>  +        * TODO: Should we assign mm->owner to NULL here?
>>  +        */
>>  +       mem = mem_cgroup_from_task(mm->owner);
>>  +       css_put(&mem->css);
>>   }
> 
> It seems to me that the code to setup/maintain mm->owner should be
> independent of the control groups, but should be part of the generic
> fork/exit code.
> 

Hmm.. Yes, we will need to do that if we decide to go with the MM_OWNER approach.

quoted text
> Also, if mm->owner exits but mm is still alive (unlikely, but could
> happen with weird custom threading libraries?) then we need to
> reassign mm->owner to one of the other users of the mm (by looking
> first in the thread group, then among the parents/siblings/children,
> and then among all processes as a last resort?)
> 

The comment in __exit_signal states that

"The group leader stays around as a zombie as long
 as there are other threads.  When it gets reaped,
 the exit.c code will add its counts into these totals."

Given that the thread group leader stays around, do we need to reassign
mm->owner? Do you do anything special in cgroups like cleanup the
task_struct->css->subsys_state on exit?

quoted text
>>  -       rcu_read_lock();
>>  -       mem = rcu_dereference(mm->mem_cgroup);
>>  +       mem = mem_cgroup_from_task(mm->owner);
> 
> I think we still need the rcu_read_lock(), since mm->owner can move
> cgroups any time.
> 

OK, so cgroup task movement is protected by RCU, right? I'll check for all
mm->owner uses.

quoted text
>>  @@ -1069,7 +1096,6 @@ static void mem_cgroup_move_task(struct
>>                 goto out;
>>
>>         css_get(&mem->css);
>>  -       rcu_assign_pointer(mm->mem_cgroup, mem);
>>         css_put(&old_mem->css);
>>
> 
> We shouldn't need reference counting on this pointer, since the
> cgroups framework won't allow a subsystem to be freed while it has any
> tasks in it.
> 

This reference earlier indicated that there were active mm->mem_cgroup users of
the cgroup. With mm->owner changes, we might not require this. Let me double
confirm that.

Thanks for the review.

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/