Re: [PATCH 1/2] mm: oom: add memcg to oom_control

[Michal Hocko <mhocko@kernel.org>]

On Fri 27-05-16 17:17:41, Vladimir Davydov wrote:
> It's a part of oom context just like allocation order and nodemask, so
> let's move it to oom_control instead of passing it in the argument list.

Don't remember why we haven't done that when the structure was adde.

> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  drivers/tty/sysrq.c |  1 +
>  include/linux/oom.h |  8 +++++---
>  mm/memcontrol.c     |  5 +++--
>  mm/oom_kill.c       | 32 +++++++++++++++-----------------
>  mm/page_alloc.c     |  1 +
>  5 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index e5139402e7f8..52bbd27e93ae 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -363,6 +363,7 @@ static void moom_callback(struct work_struct *ignored)
>  	struct oom_control oc = {
>  		.zonelist = node_zonelist(first_memory_node, gfp_mask),
>  		.nodemask = NULL,
> +		.memcg = NULL,
>  		.gfp_mask = gfp_mask,
>  		.order = -1,
>  	};
> diff --git a/include/linux/oom.h b/include/linux/oom.h
> index 83469522690a..cbc24a5fe28d 100644
> --- a/include/linux/oom.h
> +++ b/include/linux/oom.h
> @@ -23,6 +23,9 @@ struct oom_control {
>  	/* Used to determine mempolicy */
>  	nodemask_t *nodemask;
>  
> +	/* Memory cgroup in which oom is invoked, or NULL for global oom */
> +	struct mem_cgroup *memcg;
> +
>  	/* Used to determine cpuset and node locality requirement */
>  	const gfp_t gfp_mask;
>  
> @@ -83,11 +86,10 @@ extern unsigned long oom_badness(struct task_struct *p,
>  
>  extern void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  			     unsigned int points, unsigned long totalpages,
> -			     struct mem_cgroup *memcg, const char *message);
> +			     const char *message);
>  
>  extern void check_panic_on_oom(struct oom_control *oc,
> -			       enum oom_constraint constraint,
> -			       struct mem_cgroup *memcg);
> +			       enum oom_constraint constraint);
>  
>  extern enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
>  		struct task_struct *task, unsigned long totalpages);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 37ba604984c9..eeb3b14de01a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1259,6 +1259,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	struct oom_control oc = {
>  		.zonelist = NULL,
>  		.nodemask = NULL,
> +		.memcg = memcg,
>  		.gfp_mask = gfp_mask,
>  		.order = order,
>  	};
> @@ -1281,7 +1282,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  		goto unlock;
>  	}
>  
> -	check_panic_on_oom(&oc, CONSTRAINT_MEMCG, memcg);
> +	check_panic_on_oom(&oc, CONSTRAINT_MEMCG);
>  	totalpages = mem_cgroup_get_limit(memcg) ? : 1;
>  	for_each_mem_cgroup_tree(iter, memcg) {
>  		struct css_task_iter it;
> @@ -1329,7 +1330,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  
>  	if (chosen) {
>  		points = chosen_points * 1000 / totalpages;
> -		oom_kill_process(&oc, chosen, points, totalpages, memcg,
> +		oom_kill_process(&oc, chosen, points, totalpages,
>  				 "Memory cgroup out of memory");
>  	}
>  unlock:
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index b95c4c101b35..b3424199069b 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -383,8 +383,7 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask)
>  	rcu_read_unlock();
>  }
>  
> -static void dump_header(struct oom_control *oc, struct task_struct *p,
> -			struct mem_cgroup *memcg)
> +static void dump_header(struct oom_control *oc, struct task_struct *p)
>  {
>  	pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), order=%d, oom_score_adj=%hd\n",
>  		current->comm, oc->gfp_mask, &oc->gfp_mask, oc->order,
> @@ -392,12 +391,12 @@ static void dump_header(struct oom_control *oc, struct task_struct *p,
>  
>  	cpuset_print_current_mems_allowed();
>  	dump_stack();
> -	if (memcg)
> -		mem_cgroup_print_oom_info(memcg, p);
> +	if (oc->memcg)
> +		mem_cgroup_print_oom_info(oc->memcg, p);
>  	else
>  		show_mem(SHOW_MEM_FILTER_NODES);
>  	if (sysctl_oom_dump_tasks)
> -		dump_tasks(memcg, oc->nodemask);
> +		dump_tasks(oc->memcg, oc->nodemask);
>  }
>  
>  /*
> @@ -748,7 +747,7 @@ void oom_killer_enable(void)
>   */
>  void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  		      unsigned int points, unsigned long totalpages,
> -		      struct mem_cgroup *memcg, const char *message)
> +		      const char *message)
>  {
>  	struct task_struct *victim = p;
>  	struct task_struct *child;
> @@ -774,7 +773,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	task_unlock(p);
>  
>  	if (__ratelimit(&oom_rs))
> -		dump_header(oc, p, memcg);
> +		dump_header(oc, p);
>  
>  	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
>  		message, task_pid_nr(p), p->comm, points);
> @@ -795,8 +794,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  			/*
>  			 * oom_badness() returns 0 if the thread is unkillable
>  			 */
> -			child_points = oom_badness(child, memcg, oc->nodemask,
> -								totalpages);
> +			child_points = oom_badness(child,
> +					oc->memcg, oc->nodemask, totalpages);
>  			if (child_points > victim_points) {
>  				put_task_struct(victim);
>  				victim = child;
> @@ -874,8 +873,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  /*
>   * Determines whether the kernel must panic because of the panic_on_oom sysctl.
>   */
> -void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
> -			struct mem_cgroup *memcg)
> +void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint)
>  {
>  	if (likely(!sysctl_panic_on_oom))
>  		return;
> @@ -891,7 +889,7 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
>  	/* Do not panic for oom kills triggered by sysrq */
>  	if (is_sysrq_oom(oc))
>  		return;
> -	dump_header(oc, NULL, memcg);
> +	dump_header(oc, NULL);
>  	panic("Out of memory: %s panic_on_oom is enabled\n",
>  		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
>  }
> @@ -966,13 +964,13 @@ bool out_of_memory(struct oom_control *oc)
>  	constraint = constrained_alloc(oc, &totalpages);
>  	if (constraint != CONSTRAINT_MEMORY_POLICY)
>  		oc->nodemask = NULL;
> -	check_panic_on_oom(oc, constraint, NULL);
> +	check_panic_on_oom(oc, constraint);
>  
>  	if (sysctl_oom_kill_allocating_task && current->mm &&
>  	    !oom_unkillable_task(current, NULL, oc->nodemask) &&
>  	    current->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) {
>  		get_task_struct(current);
> -		oom_kill_process(oc, current, 0, totalpages, NULL,
> +		oom_kill_process(oc, current, 0, totalpages,
>  				 "Out of memory (oom_kill_allocating_task)");
>  		return true;
>  	}
> @@ -980,12 +978,11 @@ bool out_of_memory(struct oom_control *oc)
>  	p = select_bad_process(oc, &points, totalpages);
>  	/* Found nothing?!?! Either we hang forever, or we panic. */
>  	if (!p && !is_sysrq_oom(oc)) {
> -		dump_header(oc, NULL, NULL);
> +		dump_header(oc, NULL);
>  		panic("Out of memory and no killable processes...\n");
>  	}
>  	if (p && p != (void *)-1UL) {
> -		oom_kill_process(oc, p, points, totalpages, NULL,
> -				 "Out of memory");
> +		oom_kill_process(oc, p, points, totalpages, "Out of memory");
>  		/*
>  		 * Give the killed process a good chance to exit before trying
>  		 * to allocate memory again.
> @@ -1005,6 +1002,7 @@ void pagefault_out_of_memory(void)
>  	struct oom_control oc = {
>  		.zonelist = NULL,
>  		.nodemask = NULL,
> +		.memcg = NULL,
>  		.gfp_mask = 0,
>  		.order = 0,
>  	};
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f21f56f88c8a..7da8310b86e9 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3101,6 +3101,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	struct oom_control oc = {
>  		.zonelist = ac->zonelist,
>  		.nodemask = ac->nodemask,
> +		.memcg = NULL,
>  		.gfp_mask = gfp_mask,
>  		.order = order,
>  	};
> -- 
> 2.1.4

-- 
Michal Hocko
SUSE Labs