Re: [patch] mm, oom: remove task_lock protecting comm printing

[Michal Hocko]

On Tue 22-09-15 16:30:13, David Rientjes wrote:
> 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.

cpuset_print_task_mems_allowed seems unrelated and it would probably
deserve a separate pach.

> Suggested-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>

For the task_lock part
Acked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
>  include/linux/cpuset.h |  4 ++--
>  kernel/cpuset.c        | 14 +++++++-------
>  mm/oom_kill.c          |  8 +-------
>  3 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -93,7 +93,7 @@ extern int current_cpuset_is_being_rebound(void);
>  
>  extern void rebuild_sched_domains(void);
>  
> -extern void cpuset_print_task_mems_allowed(struct task_struct *p);
> +extern void cpuset_print_current_mems_allowed(void);
>  
>  /*
>   * read_mems_allowed_begin is required when making decisions involving
> @@ -219,7 +219,7 @@ static inline void rebuild_sched_domains(void)
>  	partition_sched_domains(1, NULL, NULL);
>  }
>  
> -static inline void cpuset_print_task_mems_allowed(struct task_struct *p)
> +static inline void cpuset_print_current_mems_allowed(void)
>  {
>  }
>  
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -2599,22 +2599,22 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
>  }
>  
>  /**
> - * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
> - * @tsk: pointer to task_struct of some task.
> + * cpuset_print_current_mems_allowed - prints current's cpuset and mems_allowed
>   *
> - * Description: Prints @task's name, cpuset name, and cached copy of its
> + * Description: Prints current's name, cpuset name, and cached copy of its
>   * mems_allowed to the kernel log.
>   */
> -void cpuset_print_task_mems_allowed(struct task_struct *tsk)
> +void cpuset_print_current_mems_allowed(void)
>  {
>  	struct cgroup *cgrp;
>  
>  	rcu_read_lock();
>  
> -	cgrp = task_cs(tsk)->css.cgroup;
> -	pr_info("%s cpuset=", tsk->comm);
> +	cgrp = task_cs(current)->css.cgroup;
> +	pr_info("%s cpuset=", current->comm);
>  	pr_cont_cgroup_name(cgrp);
> -	pr_cont(" mems_allowed=%*pbl\n", nodemask_pr_args(&tsk->mems_allowed));
> +	pr_cont(" mems_allowed=%*pbl\n",
> +		nodemask_pr_args(&current->mems_allowed));
>  
>  	rcu_read_unlock();
>  }
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -386,13 +386,11 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask)
>  static void dump_header(struct oom_control *oc, struct task_struct *p,
>  			struct mem_cgroup *memcg)
>  {
> -	task_lock(current);
>  	pr_warning("%s invoked oom-killer: gfp_mask=0x%x, order=%d, "
>  		"oom_score_adj=%hd\n",
>  		current->comm, oc->gfp_mask, oc->order,
>  		current->signal->oom_score_adj);
> -	cpuset_print_task_mems_allowed(current);
> -	task_unlock(current);
> +	cpuset_print_current_mems_allowed();
>  	dump_stack();
>  	if (memcg)
>  		mem_cgroup_print_oom_info(memcg, p);
> @@ -518,10 +516,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	if (__ratelimit(&oom_rs))
>  		dump_header(oc, p, memcg);
>  
> -	task_lock(p);
>  	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
>  		message, task_pid_nr(p), p->comm, points);
> -	task_unlock(p);
>  
>  	/*
>  	 * If any of p's children has a different mm and is eligible for kill,
> @@ -586,10 +582,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
>  				continue;
>  
> -			task_lock(p);	/* Protect ->comm from prctl() */
>  			pr_err("Kill process %d (%s) sharing same memory\n",
>  				task_pid_nr(p), p->comm);
> -			task_unlock(p);
>  			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>  		}
>  	rcu_read_unlock();

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/