Re: [PATCH 4/6] mm,oom_reaper: Make OOM reaper use list of mm_struct.

[Michal Hocko <mhocko@suse.cz>]

On Tue 12-07-16 15:46:57, Michal Hocko wrote:
> On Tue 12-07-16 22:38:42, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > >  #define MAX_OOM_REAP_RETRIES 10
> > > > -static void oom_reap_task(struct task_struct *tsk)
> > > > +static void oom_reap_task(struct task_struct *tsk, struct mm_struct *mm)
> > > >  {
> > > >  	int attempts = 0;
> > > > -	struct mm_struct *mm = NULL;
> > > > -	struct task_struct *p = find_lock_task_mm(tsk);
> > > >  
> > > >  	/*
> > > > -	 * Make sure we find the associated mm_struct even when the particular
> > > > -	 * thread has already terminated and cleared its mm.
> > > > -	 * We might have race with exit path so consider our work done if there
> > > > -	 * is no mm.
> > > > +	 * Check MMF_OOM_REAPED in case oom_kill_process() found this mm
> > > > +	 * pinned.
> > > >  	 */
> > > > -	if (!p)
> > > > -		goto done;
> > > > -	mm = p->mm;
> > > > -	atomic_inc(&mm->mm_count);
> > > > -	task_unlock(p);
> > > > +	if (test_bit(MMF_OOM_REAPED, &mm->flags))
> > > > +		return;
> > > >  
> > > >  	/* Retry the down_read_trylock(mmap_sem) a few times */
> > > >  	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk, mm))
> > > >  		schedule_timeout_idle(HZ/10);
> > > >  
> > > >  	if (attempts <= MAX_OOM_REAP_RETRIES)
> > > > -		goto done;
> > > > +		return;
> > > >  
> > > >  	/* Ignore this mm because somebody can't call up_write(mmap_sem). */
> > > >  	set_bit(MMF_OOM_REAPED, &mm->flags);
> > > 
> > > This seems unnecessary when oom_reaper always calls exit_oom_mm. The
> > > same applies to __oom_reap_task. Which then means that the flag is
> > > turning into a misnomer. MMF_SKIP_OOM would fit better its current
> > > meaning.
> > 
> > Large oom_score_adj value or being a child process of highest OOM score
> > might cause the same mm being selected again. I think these set_bit() are
> > necessary in order to avoid the same mm being selected again.
> 
> I do not understand. Child will have a different mm struct from the
> parent and I do not see how oom_score_adj is relevant here. Could you
> elaborate, please?

OK, I guess I got your point. You mean we can select the same child/task
again after it has passed its exit_oom_mm. Trying to oom_reap such a
task would be obviously pointless. Then it would be better to stich that
set_bit into exit_oom_mm. Renaming it would be also better in that
context.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>