On 10/10/2017 06:47 PM, Tetsuo Handa wrote:
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, despite __GFP_NORETRY
is specified, this allocation attempt might indirectly depend on somebody
else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
__GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
will cause OOM lockup. Thus, do not wait for vb->balloon_lock mutex if
leak_balloon() is called from out_of_memory().
Thread1 Thread2
fill_balloon()
takes a balloon_lock
balloon_page_enqueue()
alloc_page(GFP_HIGHUSER_MOVABLE)
direct reclaim (__GFP_FS context) takes a fs lock
waits for that fs lock alloc_page(GFP_NOFS)
__alloc_pages_may_oom()
takes the oom_lock
out_of_memory()
blocking_notifier_call_chain()
leak_balloon()
tries to take that balloon_lock and deadlocks
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
I think we could also use 'bool wait' to let the OOM use the traditional
leak_balloon code path,
which doesn't need memory allocation from xb_preload(). That is, inside
leak_balloon(), we can have
use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG) & wait;
Reviewed-by: Wei Wang <wei.w.wang@xxxxxxxxx>
---
drivers/virtio/virtio_balloon.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..03e6078 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -192,7 +192,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
}
}
-static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
+static unsigned leak_balloon(struct virtio_balloon *vb, size_t num, bool wait)
{
unsigned num_freed_pages;
struct page *page;
@@ -202,7 +202,13 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
- mutex_lock(&vb->balloon_lock);
+ if (wait)
+ mutex_lock(&vb->balloon_lock);
+ else if (!mutex_trylock(&vb->balloon_lock)) {
+ pr_info("virtio_balloon: Unable to release %lu pages due to lock contention.\n",
+ (unsigned long) min(num, (size_t)vb->num_pages));
+ return 0;
+ }
/* We can't release more pages than taken */
num = min(num, (size_t)vb->num_pages);
for (vb->num_pfns = 0; vb->num_pfns < num;
@@ -367,7 +373,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
return NOTIFY_OK;
freed = parm;
- num_freed_pages = leak_balloon(vb, oom_pages);
+ num_freed_pages = leak_balloon(vb, oom_pages, false);
update_balloon_size(vb);
*freed += num_freed_pages;
@@ -395,7 +401,7 @@ static void update_balloon_size_func(struct work_struct *work)
if (diff > 0)
diff -= fill_balloon(vb, diff);
else if (diff < 0)
- diff += leak_balloon(vb, -diff);
+ diff += leak_balloon(vb, -diff, true);
update_balloon_size(vb);
if (diff)
@@ -597,7 +603,7 @@ static void remove_common(struct virtio_balloon *vb)
{
/* There might be pages left in the balloon: free them. */
while (vb->num_pages)
- leak_balloon(vb, vb->num_pages);
+ leak_balloon(vb, vb->num_pages, true);
update_balloon_size(vb);
/* Now we reset the device so we can clean up the queues. */
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
