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

[Dmitry Safonov]

On 12/19/2016 06:54 PM, Andrey Ryabinin wrote:
> From: David Rientjes <rientjes at 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 at google.com>
> Suggested-by: Oleg Nesterov <oleg at redhat.com>
> Cc: Michal Hocko <mhocko at kernel.org>
> Cc: Vladimir Davydov <vdavydov at parallels.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work at gmail.com>
> Acked-by: Johannes Weiner <hannes at cmpxchg.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds at 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 at virtuozzo.com>

Thanks,
Reviewed-by: Dmitry Safonov <dsafonov at 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);
>  		}
>


-- 
              Dmitry