Re: [RFC PATCH] mm, oom_reaper: skip mm structs with mmu notifiers

[Michal Hocko]

On Wed 30-08-17 19:49:04, Andrea Arcangeli wrote:
> Hello Michal,
> 
> On Wed, Aug 30, 2017 at 10:46:00AM +0200, Michal Hocko wrote:
> > +	 * TODO: we really want to get rid of this ugly hack and make sure that
> > +	 * notifiers cannot block for unbounded amount of time and add
> > +	 * mmu_notifier_invalidate_range_{start,end} around unmap_page_range
> 
> KVM already should be ok in that respect. However the major reason to
> prefer mmu_notifier_invalidate_range_start/end is those can block and
> schedule waiting for stuff happening behind the PCI bus easily. So I'm
> not sure if the TODO is good idea to keep.

Long term, I was thinking about a flag to reflect that all registered
notifiers are oom safe (aka they do not depend on memory allocations
or any locks which depend on an allocation) and then we can call into
notifiers. So the check would end up
	if (!mm_has_safe_notifiers(mm))
		...
 
> > +	 */
> > +	if (mm_has_notifiers(mm)) {
> > +		schedule_timeout_idle(HZ);
> 
> Why the schedule_timeout? What's the difference with the OOM
> reaper going to sleep again in the main loop instead?

Well, this is what I had initially - basically to return false here
and rely on oom_reap_task to retry. But my current understanding is that
mm_has_notifiers is likely to be a semi-permanent state (once set it
won't likely go away) so I figured it would be better to simply wait
here and fail right away. If my assumption is not correct then I will
simply return false here.

> 
> > +		goto unlock_oom;
> > +	}
> 
> mm_has_notifiers stops changing after obtaining the mmap_sem for
> reading. See the do_mmu_notifier_register. So it's better to put the
> mm_has_notifiers check immediately after the below:
> 
> >  	if (!down_read_trylock(&mm->mmap_sem)) {
> >  		ret = false;
> >  		trace_skip_task_reaping(tsk->pid);
> 
> If we succeed taking the mmap_sem for reading then we read a stable
> value out of mm_has_notifiers and be sure it won't be set from under
> us.

OK, I will move it.

Thanks!
-- 
Michal Hocko
SUSE Labs