Re: [PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[David Rientjes]

On Wed, 4 Dec 2013, Oleg Nesterov wrote:

> find_lock_task_mm() expects it is called under rcu or tasklist lock,
> but it seems that at least oom_unkillable_task()->task_in_mem_cgroup()
> and mem_cgroup_out_of_memory()->oom_badness() can call it lockless.
> 
> Perhaps we could fix the callers, but this patch simply adds rcu lock
> into find_lock_task_mm(). This also allows to simplify a bit one of its
> callers, oom_kill_process().
> 
> Signed-off-by: Oleg Nesterov 

Acked-by: David Rientjes 
--
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/

Re: [PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[Michal Hocko]

On Wed 04-12-13 14:04:20, Oleg Nesterov wrote:
> find_lock_task_mm() expects it is called under rcu or tasklist lock,
> but it seems that at least oom_unkillable_task()->task_in_mem_cgroup()
> and mem_cgroup_out_of_memory()->oom_badness() can call it lockless.
> 
> Perhaps we could fix the callers, but this patch simply adds rcu lock
> into find_lock_task_mm(). This also allows to simplify a bit one of its
> callers, oom_kill_process().
> 
> Signed-off-by: Oleg Nesterov 

Reviewed-by: Michal Hocko 

Thanks!
> ---
>  mm/oom_kill.c |   12 
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 0d8ad1e..054ff47 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -102,14 +102,19 @@ struct task_struct *find_lock_task_mm(struct 
> task_struct *p)
>  {
>   struct task_struct *t;
>  
> + rcu_read_lock();
> +
>   for_each_thread(p, t) {
>   task_lock(t);
>   if (likely(t->mm))
> - return t;
> + goto found;
>   task_unlock(t);
>   }
> + t = NULL;
> +found:
> + rcu_read_unlock();
>  
> - return NULL;
> + return t;
>  }
>  
>  /* return true if the task is not adequate as candidate victim task. */
> @@ -461,10 +466,8 @@ void oom_kill_process(struct task_struct *p, gfp_t 
> gfp_mask, int order,
>   }
>   read_unlock(_lock);
>  
> - rcu_read_lock();
>   p = find_lock_task_mm(victim);
>   if (!p) {
> - rcu_read_unlock();
>   put_task_struct(victim);
>   return;
>   } else if (victim != p) {
> @@ -490,6 +493,7 @@ void oom_kill_process(struct task_struct *p, gfp_t 
> gfp_mask, int order,
>* That thread will now get access to memory reserves since it has a
>* pending fatal signal.
>*/
> + rcu_read_lock();
>   for_each_process(p)
>   if (p->mm == mm && !same_thread_group(p, victim) &&
>   !(p->flags & PF_KTHREAD)) {
> -- 
> 1.5.5.1
> 

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

[PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[Oleg Nesterov]

find_lock_task_mm() expects it is called under rcu or tasklist lock,
but it seems that at least oom_unkillable_task()->task_in_mem_cgroup()
and mem_cgroup_out_of_memory()->oom_badness() can call it lockless.

Perhaps we could fix the callers, but this patch simply adds rcu lock
into find_lock_task_mm(). This also allows to simplify a bit one of its
callers, oom_kill_process().

Signed-off-by: Oleg Nesterov 
---
 mm/oom_kill.c |   12 
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0d8ad1e..054ff47 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -102,14 +102,19 @@ struct task_struct *find_lock_task_mm(struct task_struct 
*p)
 {
struct task_struct *t;
 
+   rcu_read_lock();
+
for_each_thread(p, t) {
task_lock(t);
if (likely(t->mm))
-   return t;
+   goto found;
task_unlock(t);
}
+   t = NULL;
+found:
+   rcu_read_unlock();
 
-   return NULL;
+   return t;
 }
 
 /* return true if the task is not adequate as candidate victim task. */
@@ -461,10 +466,8 @@ void oom_kill_process(struct task_struct *p, gfp_t 
gfp_mask, int order,
}
read_unlock(_lock);
 
-   rcu_read_lock();
p = find_lock_task_mm(victim);
if (!p) {
-   rcu_read_unlock();
put_task_struct(victim);
return;
} else if (victim != p) {
@@ -490,6 +493,7 @@ void oom_kill_process(struct task_struct *p, gfp_t 
gfp_mask, int order,
 * That thread will now get access to memory reserves since it has a
 * pending fatal signal.
 */
+   rcu_read_lock();
for_each_process(p)
if (p->mm == mm && !same_thread_group(p, victim) &&
!(p->flags & PF_KTHREAD)) {
-- 
1.5.5.1

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

[PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[Oleg Nesterov]

find_lock_task_mm() expects it is called under rcu or tasklist lock,
but it seems that at least oom_unkillable_task()-task_in_mem_cgroup()
and mem_cgroup_out_of_memory()-oom_badness() can call it lockless.

Perhaps we could fix the callers, but this patch simply adds rcu lock
into find_lock_task_mm(). This also allows to simplify a bit one of its
callers, oom_kill_process().

Signed-off-by: Oleg Nesterov o...@redhat.com
---
 mm/oom_kill.c |   12 
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0d8ad1e..054ff47 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -102,14 +102,19 @@ struct task_struct *find_lock_task_mm(struct task_struct 
*p)
 {
struct task_struct *t;
 
+   rcu_read_lock();
+
for_each_thread(p, t) {
task_lock(t);
if (likely(t-mm))
-   return t;
+   goto found;
task_unlock(t);
}
+   t = NULL;
+found:
+   rcu_read_unlock();
 
-   return NULL;
+   return t;
 }
 
 /* return true if the task is not adequate as candidate victim task. */
@@ -461,10 +466,8 @@ void oom_kill_process(struct task_struct *p, gfp_t 
gfp_mask, int order,
}
read_unlock(tasklist_lock);
 
-   rcu_read_lock();
p = find_lock_task_mm(victim);
if (!p) {
-   rcu_read_unlock();
put_task_struct(victim);
return;
} else if (victim != p) {
@@ -490,6 +493,7 @@ void oom_kill_process(struct task_struct *p, gfp_t 
gfp_mask, int order,
 * That thread will now get access to memory reserves since it has a
 * pending fatal signal.
 */
+   rcu_read_lock();
for_each_process(p)
if (p-mm == mm  !same_thread_group(p, victim) 
!(p-flags  PF_KTHREAD)) {
-- 
1.5.5.1

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

Re: [PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[Michal Hocko]

On Wed 04-12-13 14:04:20, Oleg Nesterov wrote:
 find_lock_task_mm() expects it is called under rcu or tasklist lock,
 but it seems that at least oom_unkillable_task()-task_in_mem_cgroup()
 and mem_cgroup_out_of_memory()-oom_badness() can call it lockless.
 
 Perhaps we could fix the callers, but this patch simply adds rcu lock
 into find_lock_task_mm(). This also allows to simplify a bit one of its
 callers, oom_kill_process().
 
 Signed-off-by: Oleg Nesterov o...@redhat.com

Reviewed-by: Michal Hocko mho...@suse.cz

Thanks!
 ---
  mm/oom_kill.c |   12 
  1 files changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/mm/oom_kill.c b/mm/oom_kill.c
 index 0d8ad1e..054ff47 100644
 --- a/mm/oom_kill.c
 +++ b/mm/oom_kill.c
 @@ -102,14 +102,19 @@ struct task_struct *find_lock_task_mm(struct 
 task_struct *p)
  {
   struct task_struct *t;
  
 + rcu_read_lock();
 +
   for_each_thread(p, t) {
   task_lock(t);
   if (likely(t-mm))
 - return t;
 + goto found;
   task_unlock(t);
   }
 + t = NULL;
 +found:
 + rcu_read_unlock();
  
 - return NULL;
 + return t;
  }
  
  /* return true if the task is not adequate as candidate victim task. */
 @@ -461,10 +466,8 @@ void oom_kill_process(struct task_struct *p, gfp_t 
 gfp_mask, int order,
   }
   read_unlock(tasklist_lock);
  
 - rcu_read_lock();
   p = find_lock_task_mm(victim);
   if (!p) {
 - rcu_read_unlock();
   put_task_struct(victim);
   return;
   } else if (victim != p) {
 @@ -490,6 +493,7 @@ void oom_kill_process(struct task_struct *p, gfp_t 
 gfp_mask, int order,
* That thread will now get access to memory reserves since it has a
* pending fatal signal.
*/
 + rcu_read_lock();
   for_each_process(p)
   if (p-mm == mm  !same_thread_group(p, victim) 
   !(p-flags  PF_KTHREAD)) {
 -- 
 1.5.5.1
 

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

Re: [PATCH v2 4/4] oom_kill: add rcu_read_lock() into find_lock_task_mm()

[David Rientjes]

On Wed, 4 Dec 2013, Oleg Nesterov wrote:

 find_lock_task_mm() expects it is called under rcu or tasklist lock,
 but it seems that at least oom_unkillable_task()-task_in_mem_cgroup()
 and mem_cgroup_out_of_memory()-oom_badness() can call it lockless.
 
 Perhaps we could fix the callers, but this patch simply adds rcu lock
 into find_lock_task_mm(). This also allows to simplify a bit one of its
 callers, oom_kill_process().
 
 Signed-off-by: Oleg Nesterov o...@redhat.com

Acked-by: David Rientjes rient...@google.com
--
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/