Re: [PATCH 1/2] mm, oom: introduce oom reaper

[Michal Hocko]

On Mon 01-02-16 19:02:06, David Rientjes wrote:
> On Thu, 28 Jan 2016, Michal Hocko wrote:
> 
> > [...]
> > > > +static bool __oom_reap_vmas(struct mm_struct *mm)
> > > > +{
> > > > +	struct mmu_gather tlb;
> > > > +	struct vm_area_struct *vma;
> > > > +	struct zap_details details = {.check_swap_entries = true,
> > > > +				      .ignore_dirty = true};
> > > > +	bool ret = true;
> > > > +
> > > > +	/* We might have raced with exit path */
> > > > +	if (!atomic_inc_not_zero(&mm->mm_users))
> > > > +		return true;
> > > > +
> > > > +	if (!down_read_trylock(&mm->mmap_sem)) {
> > > > +		ret = false;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	tlb_gather_mmu(&tlb, mm, 0, -1);
> > > > +	for (vma = mm->mmap ; vma; vma = vma->vm_next) {
> > > > +		if (is_vm_hugetlb_page(vma))
> > > > +			continue;
> > > > +
> > > > +		/*
> > > > +		 * mlocked VMAs require explicit munlocking before unmap.
> > > > +		 * Let's keep it simple here and skip such VMAs.
> > > > +		 */
> > > > +		if (vma->vm_flags & VM_LOCKED)
> > > > +			continue;
> > > 
> > > Shouldn't there be VM_PFNMAP handling here?
> > 
> > What would be the reason to exclude them?
> > 
> 
> Not exclude them, but I would have expected untrack_pfn().

My understanding is that vm_normal_page will do the right thing for
those mappings - especially for CoW VM_PFNMAP which are normal pages
AFAIU. Wrt. to untrack_pfn I was relying that the victim will eventually
enter exit_mmap and do the remaining house keepining. Maybe I am missing
something but untrack_pfn shouldn't lead to releasing a considerable
amount of memory. So is this really necessary or we can wait for
exit_mmap?

> > > I'm wondering why zap_page_range() for vma->vm_start to vma->vm_end wasn't 
> > > used here for simplicity?
> > 
> > I didn't use zap_page_range because I wanted to have a full control over
> > what and how gets torn down. E.g. it is much more easier to skip over
> > hugetlb pages than relying on i_mmap_lock_write which might be blocked
> > and the whole oom_reaper will get stuck.
> > 
> 
> Let me be clear that I think the implementation is fine, minus the missing 
> handling for VM_PFNMAP.  However, I think this implementation is better 
> placed into mm/memory.c to do the iteration, selection criteria, and then 
> unmap_page_range().  I don't think we should be exposing 
> unmap_page_range() globally, but rather add a new function to do the 
> iteration in mm/memory.c with the others.

I do not have any objections to moving the code but I felt this is a
single purpose thingy which doesn't need a wider exposure. The exclusion
criteria is tightly coupled to what oom reaper is allowed to do. In
other words such a function wouldn't be reusable for say MADV_DONTNEED
because it has different criteria. Having all the selection criteria
close to __oom_reap_task on the other hand makes it easier to evaluate
their relevance. So I am not really convinced. I can move it if you feel
strongly about that, though.

> > [...]
> > > > +static void wake_oom_reaper(struct mm_struct *mm)
> > > > +{
> > > > +	struct mm_struct *old_mm;
> > > > +
> > > > +	if (!oom_reaper_th)
> > > > +		return;
> > > > +
> > > > +	/*
> > > > +	 * Pin the given mm. Use mm_count instead of mm_users because
> > > > +	 * we do not want to delay the address space tear down.
> > > > +	 */
> > > > +	atomic_inc(&mm->mm_count);
> > > > +
> > > > +	/*
> > > > +	 * Make sure that only a single mm is ever queued for the reaper
> > > > +	 * because multiple are not necessary and the operation might be
> > > > +	 * disruptive so better reduce it to the bare minimum.
> > > > +	 */
> > > > +	old_mm = cmpxchg(&mm_to_reap, NULL, mm);
> > > > +	if (!old_mm)
> > > > +		wake_up(&oom_reaper_wait);
> > > > +	else
> > > > +		mmdrop(mm);
> > > 
> > > This behavior is probably the only really significant concern I have about 
> > > the patch: we just drop the mm and don't try any reaping if there is 
> > > already reaping in progress.
> > 
> > This is based on the assumption that OOM killer will not select another
> > task to kill until the previous one drops its TIF_MEMDIE. Should this
> > change in the future we will have to come up with a queuing mechanism. I
> > didn't want to do it right away to make the change as simple as
> > possible.
> > 
> 
> The problem is that this is racy and quite easy to trigger: imagine if 
> __oom_reap_vmas() finds mm->mm_users == 0, because the memory of the 
> victim has been freed, and then another system-wide oom condition occurs 
> before the oom reaper's mm_to_reap has been set to NULL.

Yes I realize this is potentially racy. I just didn't consider the race
important enough to justify task queuing in the first submission. Tetsuo
was pushing for this already and I tried to push back for simplicity in
the first submission. But ohh well... I will queue up a patch to do this
on top. I plan to repost the full patchset shortly.

> No synchronization prevents that from happening (not sure what the
> reference to TIF_MEMDIE is about).

Now that I am reading my response again I see how it could be
misleading. I was referring to possibility of choosing multiple oom
victims which was discussed recently. I didn't mean TIF_MEMDIE to exclude
oom reaper vs. exit exclusion.

> In this case, the oom reaper has ignored the next victim and doesn't do 
> anything; the simple race has prevented it from zapping memory and does 
> not reduce the livelock probability.
> 
> This can be solved either by queueing mm's to reap or involving the oom 
> reaper into the oom killer synchronization itself.

as we have already discussed previously oom reaper is really tricky to
be called from the direct OOM context. I will go with queuing. 
 
> > > > +static int __init oom_init(void)
> > > > +{
> > > > +	oom_reaper_th = kthread_run(oom_reaper, NULL, "oom_reaper");
> > > > +	if (IS_ERR(oom_reaper_th)) {
> > > > +		pr_err("Unable to start OOM reaper %ld. Continuing regardless\n",
> > > > +				PTR_ERR(oom_reaper_th));
> > > > +		oom_reaper_th = NULL;
> > > > +	} else {
> > > > +		struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
> > > > +
> > > > +		/*
> > > > +		 * Make sure our oom reaper thread will get scheduled when
> > > > +		 * ASAP and that it won't get preempted by malicious userspace.
> > > > +		 */
> > > > +		sched_setscheduler(oom_reaper_th, SCHED_FIFO, &param);
> > > 
> > > Eeek, do you really show this is necessary?  I would imagine that we would 
> > > want to limit high priority processes system-wide and that we wouldn't 
> > > want to be interferred with by memcg oom conditions that trigger the oom 
> > > reaper, for example.
> > 
> > The idea was that we do not want to allow a high priority userspace to
> > preempt this important operation. I do understand your concern about the
> > memcg oom interference but I find it more important that oom_reaper is
> > runnable when needed. I guess that memcg oom heavy loads can change the
> > priority from userspace if necessary?
> > 
> 
> I'm baffled by any reference to "memcg oom heavy loads", I don't 
> understand this paragraph, sorry.  If a memcg is oom, we shouldn't be
> disrupting the global runqueue by running oom_reaper at a high priority.  
> The disruption itself is not only in first wakeup but also in how long the 
> reaper can run and when it is rescheduled: for a lot of memory this is 
> potentially long.  The reaper is best-effort, as the changelog indicates, 
> and we shouldn't have a reliance on this high priority: oom kill exiting 
> can't possibly be expected to be immediate.  This high priority should be 
> removed so memcg oom conditions are isolated and don't affect other loads.

If this is a concern then I would be tempted to simply disable oom
reaper for memcg oom altogether. For me it is much more important that
the reaper, even though a best effort, is guaranteed to schedule if
something goes terribly wrong on the machine.

Is this acceptable?

Thanks
-- 
Michal Hocko
SUSE Labs