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/[email protected]
> ?
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
