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

[Michal Hocko]

On Wed 08-06-16 20:18:24, 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. )

yes oom_badness can never see a task from outside of the memcg
hierarchy.

[...]
> +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;

true including the memcg fixup.

>  		return 0;
>  	case OOM_SCAN_OK:
>  		break;

Thanks! I've updated the patch locally but I will wait for Vladimir what
he thinks about this wrt. the original approach.
-- 
Michal Hocko
SUSE Labs