cgroups: enhance task_cgroup()
PATCH 1/2 cgroups: enhance task_cgroup()
task_cgroup() calls cgroup_subsys_state().
and we must use rcu_read_lock() to protect cgroup_subsys_state(). so we must use rcu_read_lock() to protect task_cgroup().
but it'll not so friendly to caller: the callers of task_cgroup() have held cgroup_lock(). it means that struct cgroup will not be freed.
So this patch add rcu_read_lock() in task_cgroup() to enhance task_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, and cgroup_lock() can protect task_cgroup().
---
include/linux/cgroup.h +5 -1
@@ -359,6 +360,10 @@static inline struct cgroup *task_cgroup (struct task_struct *task,int subsys_id) { -return task_subsys_state (task, subsys_id)->cgroup; +struct cgroup *ret; +rcu_read_lock (); + ret =task_subsys_state (task, subsys_id)->cgroup; +rcu_read_unlock (); +return ret; }
int cgroup_clone(struct task_struct *tsk, struct cgroup_subsys *ss,
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().
No, it calls task_subsys_state()
Quoted 9 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().
Is there a reason to add an implicit rcu_read_lock() in task_cgroup() and not directly in task_subsys_state() ?
Paul
Quoted 23 lines
Signed-off-by Lai Jiangshan <[email protected]>---
include/linux/cgroup.h +5
-1@@ -359,6 +360,10 @@static inline struct cgroup *task_cgroup (struct task_struct *task,int subsys_id) { -return task_subsys_state (task, subsys_id)->cgroup; +struct cgroup *ret; +rcu_read_lock (); + ret =task_subsys_state (task, subsys_id)->cgroup; +rcu_read_unlock (); +return ret; }int cgroup_clone(struct
task_struct*tsk, structcgroup_subsys*ss,
Quoted 2 lines · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().
No, it calls task_subsys_state()
Quoted 9 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().
Is there a reason to add an implicit rcu_read_lock() in task_cgroup() and not directly in task_subsys_state() ?
Paul
Quoted 23 lines
Signed-off-by Lai Jiangshan <[email protected]>---
include/linux/cgroup.h +5
-1@@ -359,6 +360,10 @@static inline struct cgroup *task_cgroup (struct task_struct *task,int subsys_id) { -return task_subsys_state (task, subsys_id)->cgroup; +struct cgroup *ret; +rcu_read_lock (); + ret =task_subsys_state (task, subsys_id)->cgroup; +rcu_read_unlock (); +return ret; }int cgroup_clone(struct
task_struct*tsk, structcgroup_subsys*ss,
A message that is not in the archive (sent off-list or not yet received): 6599ad830811211058q3fe12e5coa62436e777b60f1a-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 11 lines · Paul Menage
Quoted 1 line · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().No, it calls
task_subsys_state()Quoted 6 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.
Quoted 7 lines
Quoted 3 lines
So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().Is there a reason to add an implicit
rcu_read_lock()intask_cgroup()and not directly intask_subsys_state()?
I don't think it's a good idea to add rcu_read_lock() in task_cgroup() or task_subsys_state(). The returned pointer of the 2 functions should be protected by rcu_read_lock or cgroup_lock, so the caller should still hold the lock by itself.
An example of this is:
static int mem_cgroup_charge_common(...)
{
...
rcu_read_lock();
mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
if (unlikely(!mem)) {
rcu_read_unlock();
return 0;
}
/*
* For every charge from the cgroup, increment reference count
*/
css_get(&mem->css);
rcu_read_unlock();
...
}Quoted 24 lines
Paul
Quoted 22 lines
Signed-off-by Lai Jiangshan <[email protected]>---
include/linux/cgroup.h +5
-1@@ -359,6 +360,10 @@static inline struct cgroup *task_cgroup (struct task_struct *task,int subsys_id) { -return task_subsys_state (task, subsys_id)->cgroup; +struct cgroup *ret; +rcu_read_lock (); + ret =task_subsys_state (task, subsys_id)->cgroup; +rcu_read_unlock (); +return ret; }int cgroup_clone(struct
task_struct*tsk, structcgroup_subsys*ss,
Quoted 11 lines · Paul Menage
Quoted 1 line · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().No, it calls
task_subsys_state()Quoted 6 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.
Quoted 6 lines
Quoted 3 lines
So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().Is there a reason to add an implicit
rcu_read_lock()intask_cgroup()and not directly intask_subsys_state()?
Yes.
The caller have held the cgroup_lock() when it calls task_cgroup(). After we add an implicit rcu_read_lock() in task_cgroup(), we don't need rcu_read_lock()/task_lock() for using task_cgroup().
For cgroup_exit() will change tsk->cgroups, if we don't add an implicit rcu_read_lock() in task_cgroup(), we have to fix 7 places which using task_cgroup().
task_subsys_state() is different, it is used in fast path, If we add an implicit rcu_read_lock() in task_subsys_state(), we still need rcu_read_lock()/task_lock() for using it,
so it's redundant rcu_read_lock(), and slower the fast path a little.
Lai.
Quoted 3 lines
Paul
Quoted 11 lines · Paul Menage
Quoted 1 line · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().No, it calls
task_subsys_state()Quoted 6 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.
Quoted 7 lines
Quoted 3 lines
So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().Is there a reason to add an implicit
rcu_read_lock()intask_cgroup()and not directly intask_subsys_state()?
I don't think it's a good idea to add rcu_read_lock() in task_cgroup() or task_subsys_state(). The returned pointer of the 2 functions should be protected by rcu_read_lock or cgroup_lock, so the caller should still hold the lock by itself.
An example of this is:
static int mem_cgroup_charge_common(...)
{
...
rcu_read_lock();
mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
if (unlikely(!mem)) {
rcu_read_unlock();
return 0;
}
/*
* For every charge from the cgroup, increment reference count
*/
css_get(&mem->css);
rcu_read_unlock();
...
}Quoted 24 lines
Paul
Quoted 22 lines
Signed-off-by Lai Jiangshan <[email protected]>---
include/linux/cgroup.h +5
-1@@ -359,6 +360,10 @@static inline struct cgroup *task_cgroup (struct task_struct *task,int subsys_id) { -return task_subsys_state (task, subsys_id)->cgroup; +struct cgroup *ret; +rcu_read_lock (); + ret =task_subsys_state (task, subsys_id)->cgroup; +rcu_read_unlock (); +return ret; }int cgroup_clone(struct
task_struct*tsk, structcgroup_subsys*ss,
Quoted 11 lines · Paul Menage
Quoted 1 line · Lai Jiangshan
task_cgroup()callscgroup_subsys_state().No, it calls
task_subsys_state()Quoted 6 lines
and we must use
rcu_read_lock()to protectcgroup_subsys_state(). so we must usercu_read_lock()to protecttask_cgroup().but it'll not so friendly to caller: the callers of
task_cgroup()have heldcgroup_lock(). it means that struct cgroup will not be freed.
Quoted 6 lines
Quoted 3 lines
So this patch add
rcu_read_lock()intask_cgroup()to enhancetask_cgroup(). And we do NOT NEED FIX task_cgroup()'s callers, andcgroup_lock()can protecttask_cgroup().Is there a reason to add an implicit
rcu_read_lock()intask_cgroup()and not directly intask_subsys_state()?
Yes.
The caller have held the cgroup_lock() when it calls task_cgroup(). After we add an implicit rcu_read_lock() in task_cgroup(), we don't need rcu_read_lock()/task_lock() for using task_cgroup().
For cgroup_exit() will change tsk->cgroups, if we don't add an implicit rcu_read_lock() in task_cgroup(), we have to fix 7 places which using task_cgroup().
task_subsys_state() is different, it is used in fast path, If we add an implicit rcu_read_lock() in task_subsys_state(), we still need rcu_read_lock()/task_lock() for using it,
so it's redundant rcu_read_lock(), and slower the fast path a little.
Lai.
Quoted 3 lines
Paul