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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 02, 2017 at 03:13:30PM +0200, Michal Hocko wrote:
> On Mon 02-10-17 22:05:17, Tetsuo Handa wrote:
> > (Reducing recipients in a hope not to be filtered at the servers.)
> > 
> > Michal Hocko wrote:
> > > On Mon 02-10-17 20:33:52, Tetsuo Handa wrote:
> > > > > On Mon 02-10-17 06:59:12, Michael S. Tsirkin wrote:
> > > > > > On Sun, Oct 01, 2017 at 02:44:34PM +0900, Tetsuo Handa wrote:
> > > > > > > Tetsuo Handa wrote:
> > > > > > > > Michael S. Tsirkin wrote:
> > > > > > > > > On Mon, Sep 11, 2017 at 07:27:19PM +0900, Tetsuo Handa wrote:
> > > > > > > > > > Hello.
> > > > > > > > > > 
> > > > > > > > > > I noticed that virtio_balloon is using register_oom_notifier() and
> > > > > > > > > > leak_balloon() from virtballoon_oom_notify() might depend on
> > > > > > > > > > __GFP_DIRECT_RECLAIM memory allocation.
> > > > > > > > > > 
> > > > > > > > > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > > > > > > > > serialize against fill_balloon(). But in fill_balloon(),
> > > > > > > > > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > > > > > > > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
> > > > > > > > > > __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
> > > > > > > > > > depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
> > > > > > > > > > allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
> > > > > > > > > > __alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
> > > > > > > > > > And leak_balloon() is called by virtballoon_oom_notify() via
> > > > > > > > > > blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
> > > > > > > > > > held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
> > > > > > > > > > fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
> > > > > > > > > > at leak_balloon().
> > > > > 
> > > > > This is really nasty! And I would argue that this is an abuse of the oom
> > > > > notifier interface from the virtio code. OOM notifiers are an ugly hack
> > > > > on its own but all its users have to be really careful to not depend on
> > > > > any allocation request because that is a straight deadlock situation.
> > > > 
> > > > > 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.

I think we don't. What allocates memory is fill_balloon only.



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