Re: [PATCH 2/2] mm: oom: deduplicate victim selection code for memcg and global oom

[Tetsuo Handa]

Tetsuo Handa wrote:
> Michal Hocko wrote:
> > The victim selection code can be reduced because it is basically
> > shared between the two, only the iterator differs. But I guess that
> > can be eliminated by a simple helper.
> 
> Thank you for CC: me. I like this clean up.
> 
> > ---
> >  include/linux/oom.h |  5 +++++
> >  mm/memcontrol.c     | 47 ++++++-----------------------------------
> >  mm/oom_kill.c       | 60 ++++++++++++++++++++++++++++-------------------------
> >  3 files changed, 43 insertions(+), 69 deletions(-)
> 
> I think we can apply your version with below changes folded into your version.
> (I think totalpages argument can be passed via oom_control as well. Also, according to
> http://lkml.kernel.org/r/201602192336.EJF90671.HMFLFSVOFJOtOQ@xxxxxxxxxxxxxxxxxxx ,
> we can safely replace oc->memcg in oom_badness() in oom_evaluate_task() with NULL. )
> 
>  include/linux/oom.h |   10 ----------
>  mm/memcontrol.c     |    7 +++++--
>  mm/oom_kill.c       |   14 ++++++++++++--
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/oom.h b/include/linux/oom.h
> index 7b3eb25..77e98a0 100644
> --- a/include/linux/oom.h
> +++ b/include/linux/oom.h
> @@ -49,13 +49,6 @@ enum oom_constraint {
>  	CONSTRAINT_MEMCG,
>  };
>  
> -enum oom_scan_t {
> -	OOM_SCAN_OK,		/* scan thread and find its badness */
> -	OOM_SCAN_CONTINUE,	/* do not consider thread for oom kill */
> -	OOM_SCAN_ABORT,		/* abort the iteration and return */
> -	OOM_SCAN_SELECT,	/* always select this thread first */
> -};
> -
>  extern struct mutex oom_lock;
>  
>  static inline void set_current_oom_origin(void)
> @@ -96,9 +89,6 @@ extern void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  extern void check_panic_on_oom(struct oom_control *oc,
>  			       enum oom_constraint constraint);
>  
> -extern enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> -					       struct task_struct *task);
> -
>  extern bool out_of_memory(struct oom_control *oc);
>  
>  extern void exit_oom_victim(struct task_struct *tsk);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9c51b4d..f3482a2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1288,12 +1288,15 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  
>  		css_task_iter_start(&iter->css, &it);
>  		while ((task = css_task_iter_next(&it)))
> -			if (!oom_evaluate_task(&oc, task, totalpages))
> +			if (!oom_evaluate_task(&oc, task, totalpages)) {
> +				css_task_iter_end(&it);

Oops. Duplicated css_task_iter_end() calls. If it is safe to reverse ordering of
css_task_iter_end(&it) and mem_cgroup_iter_break(memcg, iter), removing this
css_task_iter_end(&it) line is the simplest fix.

> +				mem_cgroup_iter_break(memcg, iter);
>  				break;
> +			}
>  		css_task_iter_end(&it);
>  	}
>  
> -	if (oc.chosen) {
> +	if (oc.chosen && oc.chosen != (void *) -1UL) {
>  		points = oc.chosen_points * 1000 / totalpages;
>  		oom_kill_process(&oc, oc.chosen, points, totalpages,
>  				 "Memory cgroup out of memory");
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index bce3ea2..f634bca 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -273,8 +273,15 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc,
>  }
>  #endif
>  
> -enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> -					struct task_struct *task)
> +enum oom_scan_t {
> +	OOM_SCAN_OK,		/* scan thread and find its badness */
> +	OOM_SCAN_CONTINUE,	/* do not consider thread for oom kill */
> +	OOM_SCAN_ABORT,		/* abort the iteration and return */
> +	OOM_SCAN_SELECT,	/* always select this thread first */
> +};
> +
> +static enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> +					       struct task_struct *task)
>  {
>  	if (oom_unkillable_task(task, NULL, oc->nodemask))
>  		return OOM_SCAN_CONTINUE;
> @@ -307,6 +314,9 @@ int oom_evaluate_task(struct oom_control *oc, struct task_struct *p, unsigned lo
>  	case OOM_SCAN_CONTINUE:
>  		return 1;
>  	case OOM_SCAN_ABORT:
> +		if (oc->chosen)
> +			put_task_struct(oc->chosen);
> +		oc->chosen = (void *) -1UL;
>  		return 0;
>  	case OOM_SCAN_OK:
>  		break;
>