[Devel,rh7] ms/mm, oom: remove task_lock protecting comm printing

Submitted by Andrey Ryabinin on Dec. 19, 2016, 3:54 p.m.

Details

Message ID 20161219155423.25850-1-aryabinin@virtuozzo.com
State New
Series "ms/mm, oom: remove task_lock protecting comm printing"
Headers show

Commit Message

Andrey Ryabinin Dec. 19, 2016, 3:54 p.m.
From: David Rientjes <rientjes@google.com>

ML: da39da3a54fed88e29024f2f1f6cd7357cd03a44

The oom killer takes task_lock() in a couple of places solely to protect
printing the task's comm.

A process's comm, including current's comm, may change due to
/proc/pid/comm or PR_SET_NAME.

The comm will always be NULL-terminated, so the worst race scenario would
only be during update.  We can tolerate a comm being printed that is in
the middle of an update to avoid taking the lock.

Other locations in the kernel have already dropped task_lock() when
printing comm, so this is consistent.

Signed-off-by: David Rientjes <rientjes@google.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov@parallels.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

https://jira.sw.ru/browse/PSBM-56178

[Partial backport - this patch doesn't contain changes in kernel/cpuset.c
from the upstream patch. Our kernel still needs task_lock() task lock there
to protect task_cs(). Upstream switched to RCU protection thus it doesn't
need task_lock() to protect task_cs()]

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 mm/oom_kill.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f9a8e62..59b109e 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -570,10 +570,8 @@  bool oom_trylock(struct mem_cgroup *memcg)
 			struct task_struct *p = ctx->victim;
 
 			if (p && ctx->marked) {
-				task_lock(p);
 				pr_err("OOM kill timeout: %d (%s)\n",
 				       task_pid_nr(p), p->comm);
-				task_unlock(p);
 				show_stack(p, NULL);
 			}
 
@@ -766,12 +764,9 @@  static void oom_berserker(unsigned long points, unsigned long overdraft,
 		    points * 100 / totalpages)
 			continue;
 
-		if (__ratelimit(&berserker_rs)) {
-			task_lock(p);
+		if (__ratelimit(&berserker_rs))
 			pr_err("Rage kill process %d (%s)\n",
 			       task_pid_nr(p), p->comm);
-			task_unlock(p);
-		}
 
 		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 		mem_cgroup_note_oom_kill(memcg, p);
@@ -818,10 +813,8 @@  void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	if (__ratelimit(&oom_rs))
 		dump_header(p, gfp_mask, order, memcg, nodemask);
 
-	task_lock(p);
 	pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
 		message, task_pid_nr(p), p->comm, points * 1000 / totalpages);
-	task_unlock(p);
 
 	/*
 	 * If any of p's children has a different mm and is eligible for kill,
@@ -891,10 +884,8 @@  void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 				continue;
 
-			task_lock(p);	/* Protect ->comm from prctl() */
 			pr_err("Kill process %d (%s) in VE \"%s\" sharing same memory\n",
 				task_pid_nr(p), p->comm, task_ve_name(p));
-			task_unlock(p);
 			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 			mem_cgroup_note_oom_kill(memcg, p);
 		}

Comments

Dmitry Safonov Dec. 20, 2016, 10:44 a.m.
On 12/19/2016 06:54 PM, Andrey Ryabinin wrote:
> From: David Rientjes <rientjes@google.com>
>
> ML: da39da3a54fed88e29024f2f1f6cd7357cd03a44
>
> The oom killer takes task_lock() in a couple of places solely to protect
> printing the task's comm.
>
> A process's comm, including current's comm, may change due to
> /proc/pid/comm or PR_SET_NAME.
>
> The comm will always be NULL-terminated, so the worst race scenario would
> only be during update.  We can tolerate a comm being printed that is in
> the middle of an update to avoid taking the lock.
>
> Other locations in the kernel have already dropped task_lock() when
> printing comm, so this is consistent.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>
> https://jira.sw.ru/browse/PSBM-56178
>
> [Partial backport - this patch doesn't contain changes in kernel/cpuset.c
> from the upstream patch. Our kernel still needs task_lock() task lock there
> to protect task_cs(). Upstream switched to RCU protection thus it doesn't
> need task_lock() to protect task_cs()]
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Thanks,
Reviewed-by: Dmitry Safonov <dsafonov@virtuozzo.com>

> ---
>  mm/oom_kill.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index f9a8e62..59b109e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -570,10 +570,8 @@ bool oom_trylock(struct mem_cgroup *memcg)
>  			struct task_struct *p = ctx->victim;
>
>  			if (p && ctx->marked) {
> -				task_lock(p);
>  				pr_err("OOM kill timeout: %d (%s)\n",
>  				       task_pid_nr(p), p->comm);
> -				task_unlock(p);
>  				show_stack(p, NULL);
>  			}
>
> @@ -766,12 +764,9 @@ static void oom_berserker(unsigned long points, unsigned long overdraft,
>  		    points * 100 / totalpages)
>  			continue;
>
> -		if (__ratelimit(&berserker_rs)) {
> -			task_lock(p);
> +		if (__ratelimit(&berserker_rs))
>  			pr_err("Rage kill process %d (%s)\n",
>  			       task_pid_nr(p), p->comm);
> -			task_unlock(p);
> -		}
>
>  		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>  		mem_cgroup_note_oom_kill(memcg, p);
> @@ -818,10 +813,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  	if (__ratelimit(&oom_rs))
>  		dump_header(p, gfp_mask, order, memcg, nodemask);
>
> -	task_lock(p);
>  	pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
>  		message, task_pid_nr(p), p->comm, points * 1000 / totalpages);
> -	task_unlock(p);
>
>  	/*
>  	 * If any of p's children has a different mm and is eligible for kill,
> @@ -891,10 +884,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
>  				continue;
>
> -			task_lock(p);	/* Protect ->comm from prctl() */
>  			pr_err("Kill process %d (%s) in VE \"%s\" sharing same memory\n",
>  				task_pid_nr(p), p->comm, task_ve_name(p));
> -			task_unlock(p);
>  			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>  			mem_cgroup_note_oom_kill(memcg, p);
>  		}
>