Re: [PATCH v17 3/6] mm/balloon_compaction.c: split balloon page allocation and enqueue

[Tetsuo Handa]

Wei Wang wrote:
> Here's a detailed analysis of the deadlock by Tetsuo Handa:
> 
> In leak_balloon(), mutex_lock(>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().

Please drop "Thus, do not wait for vb->balloon_lock mutex if leak_balloon()
is called from out_of_memory()." part. This is not what this patch will do.

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

Re: [PATCH v17 3/6] mm/balloon_compaction.c: split balloon page allocation and enqueue

[Tetsuo Handa]

Wei Wang wrote:
> Here's a detailed analysis of the deadlock by Tetsuo Handa:
> 
> In leak_balloon(), mutex_lock(>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().

Please drop "Thus, do not wait for vb->balloon_lock mutex if leak_balloon()
is called from out_of_memory()." part. This is not what this patch will do.

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

[PATCH v17 3/6] mm/balloon_compaction.c: split balloon page allocation and enqueue

[Wei Wang]

From: "Michael S. Tsirkin" 

fill_balloon doing memory allocations under balloon_lock
can cause a deadlock when leak_balloon is called from
virtballoon_oom_notify and tries to take same lock.

To fix, split page allocation and enqueue and do allocations outside
the lock.

Here's a detailed analysis of the deadlock by Tetsuo Handa:

In leak_balloon(), mutex_lock(>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().

Thread1Thread2
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

Reported-by: Tetsuo Handa 
Signed-off-by: Michael S. Tsirkin 
Cc: Michal Hocko 
Cc: Wei Wang 
Reviewed-by: Wei Wang 

---
 drivers/virtio/virtio_balloon.c| 23 ++-
 include/linux/balloon_compaction.h | 34 +-
 mm/balloon_compaction.c| 28 +---
 3 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..45fe6a8 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -143,16 +143,17 @@ static void set_page_pfns(struct virtio_balloon *vb,
 
 static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
 {
-   struct balloon_dev_info *vb_dev_info = >vb_dev_info;
unsigned num_allocated_pages;
+   unsigned int num_pfns;
+   struct page *page;
+   LIST_HEAD(pages);
 
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
 
-   mutex_lock(>balloon_lock);
-   for (vb->num_pfns = 0; vb->num_pfns < num;
-vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
-   struct page *page = balloon_page_enqueue(vb_dev_info);
+   for (num_pfns = 0; num_pfns < num;
+num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+   struct page *page = balloon_page_alloc();
 
if (!page) {
dev_info_ratelimited(>vdev->dev,
@@ -162,6 +163,18 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
size_t num)
msleep(200);
break;
}
+
+   balloon_page_push(, page);
+   }
+
+   mutex_lock(>balloon_lock);
+
+   vb->num_pfns = 0;
+   while ((page = balloon_page_pop())) {
+   balloon_page_enqueue(>vb_dev_info, page);
+
+   vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
+
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
if (!virtio_has_feature(vb->vdev,
diff --git a/include/linux/balloon_compaction.h 
b/include/linux/balloon_compaction.h
index 79542b2..bdc055a 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Balloon device information descriptor.
@@ -66,7 +67,9 @@ struct balloon_dev_info {
struct inode *inode;
 };
 
-extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
+extern struct page *balloon_page_alloc(void);
+extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
+struct page *page);
 extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
 
 static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
@@ -86,6 +89,35 @@ 

[PATCH v17 3/6] mm/balloon_compaction.c: split balloon page allocation and enqueue

[Wei Wang]

From: "Michael S. Tsirkin" 

fill_balloon doing memory allocations under balloon_lock
can cause a deadlock when leak_balloon is called from
virtballoon_oom_notify and tries to take same lock.

To fix, split page allocation and enqueue and do allocations outside
the lock.

Here's a detailed analysis of the deadlock by Tetsuo Handa:

In leak_balloon(), mutex_lock(>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().

Thread1Thread2
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

Reported-by: Tetsuo Handa 
Signed-off-by: Michael S. Tsirkin 
Cc: Michal Hocko 
Cc: Wei Wang 
Reviewed-by: Wei Wang 

---
 drivers/virtio/virtio_balloon.c| 23 ++-
 include/linux/balloon_compaction.h | 34 +-
 mm/balloon_compaction.c| 28 +---
 3 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..45fe6a8 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -143,16 +143,17 @@ static void set_page_pfns(struct virtio_balloon *vb,
 
 static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
 {
-   struct balloon_dev_info *vb_dev_info = >vb_dev_info;
unsigned num_allocated_pages;
+   unsigned int num_pfns;
+   struct page *page;
+   LIST_HEAD(pages);
 
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
 
-   mutex_lock(>balloon_lock);
-   for (vb->num_pfns = 0; vb->num_pfns < num;
-vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
-   struct page *page = balloon_page_enqueue(vb_dev_info);
+   for (num_pfns = 0; num_pfns < num;
+num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+   struct page *page = balloon_page_alloc();
 
if (!page) {
dev_info_ratelimited(>vdev->dev,
@@ -162,6 +163,18 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
size_t num)
msleep(200);
break;
}
+
+   balloon_page_push(, page);
+   }
+
+   mutex_lock(>balloon_lock);
+
+   vb->num_pfns = 0;
+   while ((page = balloon_page_pop())) {
+   balloon_page_enqueue(>vb_dev_info, page);
+
+   vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
+
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
if (!virtio_has_feature(vb->vdev,
diff --git a/include/linux/balloon_compaction.h 
b/include/linux/balloon_compaction.h
index 79542b2..bdc055a 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Balloon device information descriptor.
@@ -66,7 +67,9 @@ struct balloon_dev_info {
struct inode *inode;
 };
 
-extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
+extern struct page *balloon_page_alloc(void);
+extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
+struct page *page);
 extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
 
 static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
@@ -86,6 +89,35 @@ extern void balloon_page_putback(struct page *page);
 extern int balloon_page_migrate(struct address_space *mapping,