On Tue 26-05-15 19:38:22, Oleg Nesterov wrote:
> On 05/26, Michal Hocko wrote:
> >
> > On Tue 26-05-15 18:36:46, Oleg Nesterov wrote:
> > >
> > > > +static struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
> > > > +{
> > > > + if (!p->mm)
> > > > + return NULL;
> > > > + return rcu_dereference(p->mm->memcg);
> > > > +}
> > >
> > > Probably I missed something, but it seems that the callers do not
> > > expect it can return NULL.
> >
> > This hasn't changed by this patch. mem_cgroup_from_task was allowed to
> > return NULL even before. I've just made it static because it doesn't
> > have any external users anymore.
>
> I see, but it could only return NULL if mem_cgroup_from_css() returns
> NULL. Now it returns NULL for sure if the caller is task_in_mem_cgroup(),
>
> // called when task->mm == NULL
>
> task_memcg = mem_cgroup_from_task(task);
> css_get(&task_memcg->css);
>
> and this css_get() doesn't look nice if task_memcg == NULL ;)
You are right of course. mem_cgroup_from_task is indeed weird. I will
add the diff below to the original patch and try to get rid of this
weird interface in a follow up patch.
> > I will double check
>
> Yes, please. Perhaps I missed something.
>
> > > And in fact I can't understand what mem_cgroup_from_task() actually
> > > means, with or without these changes.
> >
> > It performs task_struct->mem_cgroup mapping. We cannot use cgroup
> > mapping here because the charges are bound to mm_struct rather than
> > task.
>
> Sure, this is what I can understand. I meant... OK, lets ignore
> "without these changes", because without these changes there are
> much more oddities ;) With these changes only ->mm == NULL case
> looks unclear.
>
> And btw,
>
> if (!p->mm)
> return NULL;
> return rcu_dereference(p->mm->memcg);
>
> perhaps this needs a comment. It is not clear what protects ->mm.
> But. After this series "p" is always current (if ->mm != NULL), so
> this is fine.
>
> Nevermind. Please forget. I feel this needs a bit of cleanup, but
> we can always do this later.
Yes I will rather do that in a separate patch. Thanks!
This will go into to patch because I have indeed change the semantic of
this function and I haven't realized the subtle difference.
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index aa85d5dfbe0e..ab00b6ae84e2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -471,9 +471,14 @@ static inline struct mem_cgroup
*mem_cgroup_from_id(unsigned short id)
static struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
{
- if (!p->mm)
- return NULL;
- return rcu_dereference(p->mm->memcg);
+ if (p->mm)
+ return rcu_dereference(p->mm->memcg);
+
+ /*
+ * If the process doesn't have mm struct anymore we have to fallback
+ * to the task_css.
+ */
+ return mem_cgroup_from_css(task_css(p, memory_cgrp_id));
}
void mm_set_memcg(struct mm_struct *mm, struct mem_cgroup *memcg)
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
