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

[Tetsuo Handa]

Michal Hocko wrote:
> [Hmm, I do not see the original patch which this has been a reply to]

urbl.hostedemail.com and b.barracudacentral.org blocked my IP address,
and the rest are "Recipient address rejected: Greylisted" or
"Deferred: 451-4.3.0 Multiple destination domains per transaction is unsupported.",
and after all dropped at the servers. Sad...

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

Please describe such warning at
"int register_oom_notifier(struct notifier_block *nb)" definition.

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

virtballoon_oom_notify() can guarantee GFP_NOIO using memalloc_noio_{save,restore}()
(which is currently missing because blocking_notifier_call_chain() might be called by
GFP_NOIO allocation request (e.g. disk_events_workfn)). But there is no
memalloc_nodirectreclaim_{save,restore}() for guaranteeing GFP_NOWAIT is used.
virtballoon_oom_notify() will need to use some flag and switch GFP_NOWAIT and
GFP_KERNEL based on that flag. I worry that such approach is prone to oversight.

>                                                If the lock is held we
> would simply fall back to the normal OOM handling. As a follow up it
> would be great if virtio could use some other form of aging e.g.
> shrinker.
> -- 
> Michal Hocko
> SUSE Labs
>