Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

Michal Hocko wrote:
> On Mon 02-10-17 22:05:17, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Mon 02-10-17 20:33:52, Tetsuo Handa wrote:
> > > > > I do not think that making oom notifier API more complex is the way to
> > > > > go. Can we simply change the lock to try_lock?
> > > > 
> > > > Using mutex_trylock(&vb->balloon_lock) alone is not sufficient. Inside the
> > > > mutex, __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocation attempt is used
> > > > which will fail to make progress due to oom_lock already held. Therefore,
> > > > virtballoon_oom_notify() needs to guarantee that all allocation attempts use
> > > > GFP_NOWAIT when called from virtballoon_oom_notify().
> > > 
> > > Ohh, I missed your point and thought the dependency is indirect and some
> > > other call path is allocating while holding the lock. But you seem to be
> > > right and
> > > leak_balloon
> > >   tell_host
> > >     virtqueue_add_outbuf
> > >       virtqueue_add
> > > 
> > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> > > try to allocate while we are in the OOM path. Michael, is there any way
> > > to drop this?
> > 
> > Michael already said
> > 
> >   That would be tricky to fix. I guess we'll need to drop the lock
> >   while allocating memory - not an easy fix.
> 
> We are OOM, we cannot allocate _any_ memory! This is just broken.
> 
> > and I think that it would be possible for virtio to locally offload
> > virtballoon_oom_notify() using this patch's approach, if you don't like
> > globally offloading at the OOM notifier API level.
> 
> Even if the allocation is offloaded to a different context we are sill
> OOM and we would have to block waiting for it which is just error prone.

Like I comment below, I'm assuming that this deadlock should rarely
happen from the beginning. Since GFP_KERNEL allocation is conditional,
we might be able to avoid the allocation from virtballoon_oom_notify().

Michael S. Tsirkin wrote:
> > @@ -1005,17 +1033,21 @@ int unregister_oom_notifier(struct notifier_block *nb)
> >   */
> >  bool out_of_memory(struct oom_control *oc)
> >  {
> > -	unsigned long freed = 0;
> >  	enum oom_constraint constraint = CONSTRAINT_NONE;
> >  
> >  	if (oom_killer_disabled)
> >  		return false;
> >  
> > -	if (!is_memcg_oom(oc)) {
> > -		blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> > -		if (freed > 0)
> > +	if (!is_memcg_oom(oc) && oom_notifier_th) {
> > +		oom_notifier_requested = true;
> > +		wake_up(&oom_notifier_request_wait);
> > +		wait_event_timeout(oom_notifier_response_wait,
> > +				   !oom_notifier_requested, 5 * HZ);
> 
> I guess this means what was earlier a deadlock will free up after 5
> seconds,

Yes.

>          by a 5 sec downtime is still a lot, isn't it?

This timeout should unlikely expire. Please note that this offloading is
intended for handling the worst scenario, that is, "out_of_memory() is called
when somebody is already holding vb->balloon_lock lock" and
"GFP_KERNEL allocation is attempted from virtballoon_oom_notify()".

As far as I know, this lock is held when fill_balloon() or leak_balloon() is
called. Majority of OOM events call out_of_memory() without holding this lock.
Thus, "out_of_memory() is called when somebody is already holding vb->balloon_lock
lock" should rarely happen from the beginning.

If you can artificially trigger this deadlock (i.e. user triggerable OOM DoS),
a patch for fixing this problem needs to be backported to older/distributor
kernels...

Yes, conditional GFP_KERNEL allocation attempt from virtqueue_add() might
still cause this deadlock. But that depends on whether you can trigger this
deadlock. As far as I know, there is no report. Thus, I think that avoiding
theoretical deadlock using timeout will be sufficient.

> 
> 
> > +		if (oom_notifier_freed) {
> > +			oom_notifier_freed = 0;
> >  			/* Got some memory back in the last second. */
> >  			return true;
> > +		}
> >  	}
> >  
> >  	/*
> > -- 
> > 1.8.3.1
> 

--
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>