Re: [PATCH 2/3] oom, oom_reaper: Try to reap tasks which skip regular OOM killer path

[Michal Hocko]

On Thu 07-04-16 20:38:43, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > @@ -563,6 +582,53 @@ static void wake_oom_reaper(struct task_struct *tsk)
> >  	wake_up(&oom_reaper_wait);
> >  }
> >  
> > +/* Check if we can reap the given task. This has to be called with stable
> > + * tsk->mm
> > + */
> > +static void try_oom_reaper(struct task_struct *tsk)
> > +{
> > +	struct mm_struct *mm = tsk->mm;
> > +	struct task_struct *p;
> > +
> > +	if (!mm)
> > +		return;
> > +
> > +	/*
> > +	 * There might be other threads/processes which are either not
> > +	 * dying or even not killable.
> > +	 */
> > +	if (atomic_read(&mm->mm_users) > 1) {
> > +		rcu_read_lock();
> > +		for_each_process(p) {
> > +			bool exiting;
> > +
> > +			if (!process_shares_mm(p, mm))
> > +				continue;
> > +			if (same_thread_group(p, tsk))
> > +				continue;
> > +			if (fatal_signal_pending(p))
> > +				continue;
> > +
> > +			/*
> > +			 * If the task is exiting make sure the whole thread group
> > +			 * is exiting and cannot acces mm anymore.
> > +			 */
> > +			spin_lock_irq(&p->sighand->siglock);
> > +			exiting = signal_group_exit(p->signal);
> > +			spin_unlock_irq(&p->sighand->siglock);
> > +			if (exiting)
> > +				continue;
> > +
> > +			/* Give up */
> > +			rcu_read_unlock();
> > +			return;
> > +		}
> > +		rcu_read_unlock();
> > +	}
> > +
> > +	wake_oom_reaper(tsk);
> > +}
> > +
> 
> I think you want to change "try_oom_reaper() without wake_oom_reaper()"
> as mm_is_reapable() and use it from oom_kill_process() in order to skip
> p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN test which needlessly makes
> can_oom_reap false.

Not sure I understand the OOM_SCORE_ADJ_MIN part. We cannot reap the
task if somebody sharing the mm is OOM_SCORE_ADJ_MIN. We have to check
this in oom_kill_process because we are sending SIGKILL but we do not
have to check for this explicitly in try_oom_reaper because we only care
about exiting/killed tasks.

[...]

> > @@ -873,6 +926,7 @@ bool out_of_memory(struct oom_control *oc)
> >  	if (current->mm &&
> >  	    (fatal_signal_pending(current) || task_will_free_mem(current))) {
> >  		mark_oom_victim(current);
> > +		try_oom_reaper(current);
> >  		return true;
> >  	}
> >  
> 
> Why don't you call try_oom_reaper() from the shortcuts in
> mem_cgroup_out_of_memory() as well?

I have focused on the global case and the correctness for now. But I
agree we can safely squash mem_cgroup_out_of_memory part into the patch
as well. Thanks for pointing this out.

> Why don't you embed try_oom_reaper() into mark_oom_victim() like I did at
> http://lkml.kernel.org/r/201602052014.HBG52666.HFMOQVLFOSFJtO@xxxxxxxxxxxxxxxxxxx ?

it didn't fit in the current flow of oom_kill_process where we do:
do_send_sig_info(victim)
mark_oom_victim(victim)
kill_sharing_tasks

so in the case of shared mm we wouldn't schedule the task for the reaper
most likely because we have to kill them first.
-- 
Michal Hocko
SUSE Labs