make balloon pages movable by compaction

Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload.

This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures.

Signed-off-by Rafael Aquini <[email protected]>

Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload.

This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures.

Signed-off-by Rafael Aquini <[email protected]>

Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload.

This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures.

Signed-off-by Rafael Aquini <[email protected]>

---
 include/linux/mm.h |  17 ++++++++
 mm/compaction.c    | 125 +++++++++++++++++++++++++++++++++++++++++++++--------
 mm/migrate.c       |  30 ++++++++++++-
 3 files changed, 152 insertions(+), 20 deletions(-)

include/linux/mm.h +0 -0

index 311be90..56cc553 100644
@@ -1662,5 +1662,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
 static inline bool page_is_guard(struct page *page) { return false; }
 #endif /* CONFIG_DEBUG_PAGEALLOC */

include/linux/mm.h +0 -0

 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/compaction.c +24 -0

index e78cb96..e4e871b 100644
@@ -14,6 +14,7 @@
 #include <linux/backing-dev.h>
 #include <linux/sysctl.h>
 #include <linux/sysfs.h>
+#include <linux/export.h>
 #include "internal.h"
 
 #if defined CONFIG_COMPACTION || defined CONFIG_CMA

@@ -21,6 +22,84 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/compaction.h>
 
+#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
+/*
+ * Balloon pages special page->mapping.
+ * Users must properly allocate and initialize an instance of balloon_mapping,
+ * and set it as the page->mapping for balloon enlisted page instances.
+ * There is no need on utilizing struct address_space locking schemes for
+ * balloon_mapping as, once it gets initialized at balloon driver, it will
+ * remain just like a static reference that helps us on identifying a guest
+ * ballooned page by its mapping, as well as it will keep the 'a_ops' callback
+ * pointers to the functions that will execute the balloon page mobility tasks.
+ *
+ * address_space_operations necessary methods for ballooned pages:
+ *   .migratepage    - used to perform balloon's page migration (as is)
+ *   .invalidatepage - used to isolate a page from balloon's page list
+ *   .freepage       - used to reinsert an isolated page to balloon's page list
+ */
+struct address_space *balloon_mapping;
+EXPORT_SYMBOL_GPL(balloon_mapping);
+
+static inline void __isolate_balloon_page(struct page *page)
+{
+	page->mapping->a_ops->invalidatepage(page, 0);
+}

mm/compaction.c +72 -19

+		return false;
+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * As balloon pages are not isolated from LRU lists, concurrent
+		 * compaction threads can race against page migration functions
+		 * move_to_new_page() & __unmap_and_move().
+		 * In order to avoid having an already isolated balloon page
+		 * being (wrongly) re-isolated while it is under migration,
+		 * lets be sure we have the page lock before proceeding with
+		 * the balloon page isolation steps.
+		 */
+		if (likely(trylock_page(page))) {
+			/*
+			 * A ballooned page, by default, has just one refcount.
+			 * Prevent concurrent compaction threads from isolating
+			 * an already isolated balloon page.
+			 */
+			if (movable_balloon_page(page) &&
+			    (page_count(page) == 2)) {
+				__isolate_balloon_page(page);
+				unlock_page(page);
+				return true;
+			}
+			unlock_page(page);
+		}
+		/* Drop refcount taken for this already isolated page */
+		put_page(page);
+	}
+	return false;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+void putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))
+		return;
+
+	lock_page(page);
+	__putback_balloon_page(page);
+	put_page(page);
+	unlock_page(page);
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
+
 static unsigned long release_freepages(struct list_head *freelist)
 {
 	struct page *page, *next;

@@ -312,32 +391,40 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			continue;
 		}
 
-		if (!PageLRU(page))
-			continue;
-
 		/*
-		 * PageLRU is set, and lru_lock excludes isolation,
-		 * splitting and collapsing (collapsing has already
-		 * happened if PageLRU is set).
+		 * It is possible to migrate LRU pages and balloon pages.
+		 * Skip any other type of page.
 		 */
-		if (PageTransHuge(page)) {
-			low_pfn += (1 << compound_order(page)) - 1;
-			continue;
-		}
+		if (PageLRU(page)) {
+			/*
+			 * PageLRU is set, and lru_lock excludes isolation,
+			 * splitting and collapsing (collapsing has already
+			 * happened if PageLRU is set).
+			 */
+			if (PageTransHuge(page)) {
+				low_pfn += (1 << compound_order(page)) - 1;
+				continue;
+			}
 
-		if (!cc->sync)
-			mode |= ISOLATE_ASYNC_MIGRATE;
+			if (!cc->sync)
+				mode |= ISOLATE_ASYNC_MIGRATE;
 
-		lruvec = mem_cgroup_page_lruvec(page, zone);
+			lruvec = mem_cgroup_page_lruvec(page, zone);
 
-		/* Try isolate the page */
-		if (__isolate_lru_page(page, mode) != 0)
-			continue;
+			/* Try isolate the page */
+			if (__isolate_lru_page(page, mode) != 0)
+				continue;
+
+			VM_BUG_ON(PageTransCompound(page));
 
-		VM_BUG_ON(PageTransCompound(page));
+			/* Successfully isolated */
+			del_page_from_lru_list(page, lruvec, page_lru(page));
+		} else if (unlikely(movable_balloon_page(page))) {
+			if (!isolate_balloon_page(page))
+				continue;
+		} else
+			continue;
 
-		/* Successfully isolated */
-		del_page_from_lru_list(page, lruvec, page_lru(page));
 		list_add(&page->lru, migratelist);
 		cc->nr_migratepages++;
 		nr_isolated++;

mm/migrate.c +29 -1

index 77ed2d7..80f22bb 100644
@@ -79,7 +79,10 @@ void putback_lru_pages(struct list_head *l)
 		list_del(&page->lru);
 		dec_zone_page_state(page, NR_ISOLATED_ANON +
 				page_is_file_cache(page));
-		putback_lru_page(page);
+		if (unlikely(movable_balloon_page(page)))
+			putback_balloon_page(page);
+		else
+			putback_lru_page(page);
 	}
 }
 

@@ -778,6 +781,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		}
 	}
 
+	if (unlikely(movable_balloon_page(page))) {
+		/*
+		 * A ballooned page does not need any special attention from
+		 * physical to virtual reverse mapping procedures.
+		 * Skip any attempt to unmap PTEs or to remap swap cache,
+		 * in order to avoid burning cycles at rmap level.
+		 */
+		remap_swapcache = 0;
+		goto skip_unmap;
+	}
+
 	/*
 	 * Corner case handling:
 	 * 1. When a new swap-cache page is read into, it is added to the LRU

@@ -846,6 +860,20 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 			goto out;
 
 	rc = __unmap_and_move(page, newpage, force, offlining, mode);
+
+	if (unlikely(movable_balloon_page(newpage))) {
+		/*
+		 * A ballooned page has been migrated already. Now, it is the
+		 * time to wrap-up counters, handle the old page back to Buddy
+		 * and return.
+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);
+		return rc;
+	}
 out:
 	if (rc != -EAGAIN) {
 		/*

Signature · 1 line

1.7.11.2

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 6 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 6 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Quoted 7 lines · Michael S. Tsirkin

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

I'm curious about your guessing here. Could you ellaborate it further, please?

Quoted 10 lines

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 6 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please?

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 23 lines · Rafael Aquini

Quoted 7 lines · Michael S. Tsirkin

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

I'm curious about your guessing here. Could you ellaborate it further, please?

Quoted 10 lines

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 6 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

Quoted 6 lines

This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please?

E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers.

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 3 lines

E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers.

But of course I agree this is not top priority, no need to block submission on this, just nice to have.

Quoted 3 lines

E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers.

But of course I agree this is not top priority, no need to block submission on this, just nice to have.

Quoted 23 lines · Rafael Aquini

Quoted 7 lines · Michael S. Tsirkin

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

I'm curious about your guessing here. Could you ellaborate it further, please?

Quoted 10 lines

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 6 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

Quoted 6 lines

This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please?

E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers.

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 8 lines · Michael S. Tsirkin

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually.

Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch.

If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ?

Mel, what are your thoughts here?

Quoted 1 line

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 8 lines · Michael S. Tsirkin

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually.

Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch.

If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ?

Mel, what are your thoughts here?

Quoted 1 line

Quoted 1 line

While testing it, I wasn't lucky to see this small window opening, though.

Quoted 8 lines · Michael S. Tsirkin

Quoted 8 lines

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return.

If it is legal, why warn? For that matter why test here at all?

As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually.

Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch.

If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ?

Mel, what are your thoughts here?

Quoted 7 lines · Michael S. Tsirkin

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1673

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline bool movable_balloon_page(struct page *page)
+{
+	return (page->mapping && page->mapping == balloon_mapping);

I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ...

I'm curious about your guessing here. Could you ellaborate it further, please?

Quoted 11 lines

include/linux/mm.h static inline unsigned int debug_guardpage_minorder(void) { return 0; } · line 1681

Quoted 7 lines

include/linux/mm.h +6 -0

+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
+

This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ...

This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please?

Quoted 8 lines

mm/compaction.c line 57

Quoted 4 lines

mm/compaction.c +4 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here.

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. While testing it, I wasn't lucky to see this small window opening, though.

Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload.

This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures.

Signed-off-by Rafael Aquini <[email protected]>

---
 include/linux/mm.h |  17 ++++++++
 mm/compaction.c    | 125 +++++++++++++++++++++++++++++++++++++++++++++--------
 mm/migrate.c       |  30 ++++++++++++-
 3 files changed, 152 insertions(+), 20 deletions(-)

include/linux/mm.h +0 -0

index 311be90..56cc553 100644
@@ -1662,5 +1662,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
 static inline bool page_is_guard(struct page *page) { return false; }
 #endif /* CONFIG_DEBUG_PAGEALLOC */

include/linux/mm.h +0 -0

 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/compaction.c +24 -0

index e78cb96..e4e871b 100644
@@ -14,6 +14,7 @@
 #include <linux/backing-dev.h>
 #include <linux/sysctl.h>
 #include <linux/sysfs.h>
+#include <linux/export.h>
 #include "internal.h"
 
 #if defined CONFIG_COMPACTION || defined CONFIG_CMA

@@ -21,6 +22,84 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/compaction.h>
 
+#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
+/*
+ * Balloon pages special page->mapping.
+ * Users must properly allocate and initialize an instance of balloon_mapping,
+ * and set it as the page->mapping for balloon enlisted page instances.
+ * There is no need on utilizing struct address_space locking schemes for
+ * balloon_mapping as, once it gets initialized at balloon driver, it will
+ * remain just like a static reference that helps us on identifying a guest
+ * ballooned page by its mapping, as well as it will keep the 'a_ops' callback
+ * pointers to the functions that will execute the balloon page mobility tasks.
+ *
+ * address_space_operations necessary methods for ballooned pages:
+ *   .migratepage    - used to perform balloon's page migration (as is)
+ *   .invalidatepage - used to isolate a page from balloon's page list
+ *   .freepage       - used to reinsert an isolated page to balloon's page list
+ */
+struct address_space *balloon_mapping;
+EXPORT_SYMBOL_GPL(balloon_mapping);
+
+static inline void __isolate_balloon_page(struct page *page)
+{
+	page->mapping->a_ops->invalidatepage(page, 0);
+}

mm/compaction.c +72 -19

+		return false;
+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * As balloon pages are not isolated from LRU lists, concurrent
+		 * compaction threads can race against page migration functions
+		 * move_to_new_page() & __unmap_and_move().
+		 * In order to avoid having an already isolated balloon page
+		 * being (wrongly) re-isolated while it is under migration,
+		 * lets be sure we have the page lock before proceeding with
+		 * the balloon page isolation steps.
+		 */
+		if (likely(trylock_page(page))) {
+			/*
+			 * A ballooned page, by default, has just one refcount.
+			 * Prevent concurrent compaction threads from isolating
+			 * an already isolated balloon page.
+			 */
+			if (movable_balloon_page(page) &&
+			    (page_count(page) == 2)) {
+				__isolate_balloon_page(page);
+				unlock_page(page);
+				return true;
+			}
+			unlock_page(page);
+		}
+		/* Drop refcount taken for this already isolated page */
+		put_page(page);
+	}
+	return false;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+void putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))
+		return;
+
+	lock_page(page);
+	__putback_balloon_page(page);
+	put_page(page);
+	unlock_page(page);
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
+
 static unsigned long release_freepages(struct list_head *freelist)
 {
 	struct page *page, *next;

@@ -312,32 +391,40 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			continue;
 		}
 
-		if (!PageLRU(page))
-			continue;
-
 		/*
-		 * PageLRU is set, and lru_lock excludes isolation,
-		 * splitting and collapsing (collapsing has already
-		 * happened if PageLRU is set).
+		 * It is possible to migrate LRU pages and balloon pages.
+		 * Skip any other type of page.
 		 */
-		if (PageTransHuge(page)) {
-			low_pfn += (1 << compound_order(page)) - 1;
-			continue;
-		}
+		if (PageLRU(page)) {
+			/*
+			 * PageLRU is set, and lru_lock excludes isolation,
+			 * splitting and collapsing (collapsing has already
+			 * happened if PageLRU is set).
+			 */
+			if (PageTransHuge(page)) {
+				low_pfn += (1 << compound_order(page)) - 1;
+				continue;
+			}
 
-		if (!cc->sync)
-			mode |= ISOLATE_ASYNC_MIGRATE;
+			if (!cc->sync)
+				mode |= ISOLATE_ASYNC_MIGRATE;
 
-		lruvec = mem_cgroup_page_lruvec(page, zone);
+			lruvec = mem_cgroup_page_lruvec(page, zone);
 
-		/* Try isolate the page */
-		if (__isolate_lru_page(page, mode) != 0)
-			continue;
+			/* Try isolate the page */
+			if (__isolate_lru_page(page, mode) != 0)
+				continue;
+
+			VM_BUG_ON(PageTransCompound(page));
 
-		VM_BUG_ON(PageTransCompound(page));
+			/* Successfully isolated */
+			del_page_from_lru_list(page, lruvec, page_lru(page));
+		} else if (unlikely(movable_balloon_page(page))) {
+			if (!isolate_balloon_page(page))
+				continue;
+		} else
+			continue;
 
-		/* Successfully isolated */
-		del_page_from_lru_list(page, lruvec, page_lru(page));
 		list_add(&page->lru, migratelist);
 		cc->nr_migratepages++;
 		nr_isolated++;

mm/migrate.c +29 -1

index 77ed2d7..80f22bb 100644
@@ -79,7 +79,10 @@ void putback_lru_pages(struct list_head *l)
 		list_del(&page->lru);
 		dec_zone_page_state(page, NR_ISOLATED_ANON +
 				page_is_file_cache(page));
-		putback_lru_page(page);
+		if (unlikely(movable_balloon_page(page)))
+			putback_balloon_page(page);
+		else
+			putback_lru_page(page);
 	}
 }
 

@@ -778,6 +781,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		}
 	}
 
+	if (unlikely(movable_balloon_page(page))) {
+		/*
+		 * A ballooned page does not need any special attention from
+		 * physical to virtual reverse mapping procedures.
+		 * Skip any attempt to unmap PTEs or to remap swap cache,
+		 * in order to avoid burning cycles at rmap level.
+		 */
+		remap_swapcache = 0;
+		goto skip_unmap;
+	}
+
 	/*
 	 * Corner case handling:
 	 * 1. When a new swap-cache page is read into, it is added to the LRU

@@ -846,6 +860,20 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 			goto out;
 
 	rc = __unmap_and_move(page, newpage, force, offlining, mode);
+
+	if (unlikely(movable_balloon_page(newpage))) {
+		/*
+		 * A ballooned page has been migrated already. Now, it is the
+		 * time to wrap-up counters, handle the old page back to Buddy
+		 * and return.
+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);
+		return rc;
+	}
 out:
 	if (rc != -EAGAIN) {
 		/*

Signature · 1 line

1.7.11.2

Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload.

Besides making balloon pages movable at allocation time and introducing the necessary primitives to perform balloon page migration/compaction, this patch also introduces the following locking scheme to provide the proper synchronization and protection for struct virtio_balloon elements against concurrent accesses due to parallel operations introduced by memory compaction / page migration.
- balloon_lock (mutex) : synchronizes the access demand to elements of

			  struct virtio_balloon and its queue operations;
 - pages_lock (spinlock): special protection to balloon pages list against
			  concurrent list handling operations;

Signed-off-by Rafael Aquini <[email protected]>

---
 drivers/virtio/virtio_balloon.c | 138 +++++++++++++++++++++++++++++++++++++---
 include/linux/virtio_balloon.h  |   4 ++
 2 files changed, 134 insertions(+), 8 deletions(-)

drivers/virtio/virtio_balloon.c +12 -2

index 0908e60..7c937a0 100644
@@ -27,6 +27,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/fs.h>
 
 /*
  * Balloon device works in 4K page units.  So each page is pointed to by

@@ -35,6 +36,12 @@
  */
 #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
 
+/* Synchronizes accesses/updates to the struct virtio_balloon elements */
+DEFINE_MUTEX(balloon_lock);
+
+/* Protects 'virtio_balloon->pages' list against concurrent handling */
+DEFINE_SPINLOCK(pages_lock);
+
 struct virtio_balloon
 {
 	struct virtio_device *vdev;

@@ -51,6 +58,7 @@ struct virtio_balloon
 
 	/* Number of balloon pages we've told the Host we're not using. */
 	unsigned int num_pages;
+
 	/*
 	 * The pages we've told the Host we're not using.
 	 * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE

@@ -125,10 +133,12 @@ static void fill_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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
-		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
-					__GFP_NOMEMALLOC | __GFP_NOWARN);
+		struct page *page = alloc_page(GFP_HIGHUSER_MOVABLE |
+						__GFP_NORETRY | __GFP_NOWARN |
+						__GFP_NOMEMALLOC);
 		if (!page) {
 			if (printk_ratelimit())
 				dev_printk(KERN_INFO, &vb->vdev->dev,

drivers/virtio/virtio_balloon.c +5 -0

+		page->mapping = balloon_mapping;
+		spin_unlock(&pages_lock);
 	}
 
 	/* Didn't get any?  Oh well. */

@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		return;
 
 	tell_host(vb, vb->inflate_vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -169,10 +183,22 @@ static void 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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		/*

drivers/virtio/virtio_balloon.c +17 -6

+		spin_unlock(&pages_lock);
 		set_page_pfns(vb->pfns + vb->num_pfns, page);
 		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
 	}

@@ -182,8 +208,11 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);
-	release_pages_by_pfn(vb->pfns, vb->num_pfns);
+	if (vb->num_pfns > 0) {
+		tell_host(vb, vb->deflate_vq);
+		release_pages_by_pfn(vb->pfns, vb->num_pfns);
+	}
+	mutex_unlock(&balloon_lock);
 }
 
 static inline void update_stat(struct virtio_balloon *vb, int idx,

@@ -239,6 +268,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
 	struct scatterlist sg;
 	unsigned int len;
 
+	mutex_lock(&balloon_lock);
 	vb->need_stats_update = 0;
 	update_balloon_stats(vb);
 

@@ -249,6 +279,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
 	if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
 		BUG();
 	virtqueue_kick(vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void virtballoon_changed(struct virtio_device *vdev)

@@ -261,22 +292,27 @@ static void virtballoon_changed(struct virtio_device *vdev)
 static inline s64 towards_target(struct virtio_balloon *vb)
 {
 	__le32 v;
-	s64 target;
+	s64 target, actual;
 
+	mutex_lock(&balloon_lock);
+	actual = vb->num_pages;
 	vb->vdev->config->get(vb->vdev,
 			      offsetof(struct virtio_balloon_config, num_pages),
 			      &v, sizeof(v));
 	target = le32_to_cpu(v);
-	return target - vb->num_pages;
+	mutex_unlock(&balloon_lock);
+	return target - actual;
 }
 
 static void update_balloon_size(struct virtio_balloon *vb)
 {
-	__le32 actual = cpu_to_le32(vb->num_pages);
-
+	__le32 actual;
+	mutex_lock(&balloon_lock);
+	actual = cpu_to_le32(vb->num_pages);
 	vb->vdev->config->set(vb->vdev,
 			      offsetof(struct virtio_balloon_config, actual),
 			      &actual, sizeof(actual));
+	mutex_unlock(&balloon_lock);
 }
 
 static int balloon(void *_vballoon)

drivers/virtio/virtio_balloon.c +48 -0

+struct virtio_balloon *vb_ptr;
+
+/*
+ * Populate balloon_mapping->a_ops->migratepage method to perform the balloon
+ * page migration task.
+ *
+ * After a ballooned page gets isolated by compaction procedures, this is the
+ * function that performs the page migration on behalf of move_to_new_page(),
+ * when the last calls (page)->mapping->a_ops->migratepage.
+ *
+ * Page migration for virtio balloon is done in a simple swap fashion which
+ * follows these two steps:
+ *  1) insert newpage into vb->pages list and update the host about it;
+ *  2) update the host about the removed old page from vb->pages list;
+ */
+int virtballoon_migratepage(struct address_space *mapping,
+		struct page *newpage, struct page *page, enum migrate_mode mode)
+{
+	mutex_lock(&balloon_lock);
+
+	/* balloon's page migration 1st step */
+	vb_ptr->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+	spin_lock(&pages_lock);
+	list_add(&newpage->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);
+	set_page_pfns(vb_ptr->pfns, newpage);
+	tell_host(vb_ptr, vb_ptr->inflate_vq);
+
+	/* balloon's page migration 2nd step */
+	vb_ptr->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+	set_page_pfns(vb_ptr->pfns, page);
+	tell_host(vb_ptr, vb_ptr->deflate_vq);
+
+	mutex_unlock(&balloon_lock);
+
+	return 0;
+}
+
+/*
+ * Populate balloon_mapping->a_ops->invalidatepage method to help compaction on
+ * isolating a page from the balloon page list.
+ */
+void virtballoon_isolatepage(struct page *page, unsigned long mode)
+{
+	spin_lock(&pages_lock);
+	list_del(&page->lru);
+	spin_unlock(&pages_lock);
+}

drivers/virtio/virtio_balloon.c +9 -0

+}
+
+static const struct address_space_operations virtio_balloon_aops = {
+	.migratepage = virtballoon_migratepage,
+	.invalidatepage = virtballoon_isolatepage,
+	.freepage = virtballoon_putbackpage,
+};
+
 static int virtballoon_probe(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb;

@@ -351,11 +457,25 @@ static int virtballoon_probe(struct virtio_device *vdev)
 	}
 
 	INIT_LIST_HEAD(&vb->pages);
+
 	vb->num_pages = 0;
 	init_waitqueue_head(&vb->config_change);
 	init_waitqueue_head(&vb->acked);

drivers/virtio/virtio_balloon.c +0 -0

 }
 
 #ifdef CONFIG_PM

include/linux/virtio_balloon.h +0 -0

index 652dc8b..930f1b7 100644

include/linux/virtio_balloon.h +0 -0

 #endif /* _LINUX_VIRTIO_BALLOON_H */

Signature · 1 line

1.7.11.2

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

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

Cheers,
Rusty.

Quoted 22 lines · Rusty Russell

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

Quoted 8 lines

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

Cheers,
Rusty.

I know I meant something else.
Let me lay this out:

CPU1 executes:
void virtballoon_putbackpage(struct page *page)

{
	spin_lock(&pages_lock);
	list_add(&page->lru, &vb_ptr->pages);
	spin_unlock(&pages_lock);


		at this point CPU2 unloads module:
						leak_balloon
						......

		next CPU2 loads another module so code memory gets overwritten

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

No?

Quoted 22 lines · Rusty Russell

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

Quoted 8 lines

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

Cheers,
Rusty.

I know I meant something else.
Let me lay this out:

CPU1 executes:
void virtballoon_putbackpage(struct page *page)

{
	spin_lock(&pages_lock);
	list_add(&page->lru, &vb_ptr->pages);
	spin_unlock(&pages_lock);


		at this point CPU2 unloads module:
						leak_balloon
						......

		next CPU2 loads another module so code memory gets overwritten

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

No?

Quoted 56 lines · Michael S. Tsirkin

Quoted 22 lines · Rusty Russell

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

Quoted 8 lines

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

Cheers,
Rusty.

I know I meant something else.
Let me lay this out:

CPU1 executes:
void virtballoon_putbackpage(struct page *page)

{
	spin_lock(&pages_lock);
	list_add(&page->lru, &vb_ptr->pages);
	spin_unlock(&pages_lock);


		at this point CPU2 unloads module:
						leak_balloon
						......

		next CPU2 loads another module so code memory gets overwritten

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Quoted 7 lines

Quoted 7 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 8 lines

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

Quoted 7 lines

Quoted 7 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 8 lines

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines

Quoted 8 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Quoted 2 lines

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

CPU1 will be returning to putback_balloon_page() which code is located at core mm/compaction.c, outside the driver.

Quoted 17 lines · Rafael Aquini

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines

Quoted 8 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Quoted 5 lines

Quoted 2 lines

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

CPU1 will be returning to putback_balloon_page() which code is located at core mm/compaction.c, outside the driver.

Sorry, I don't seem to be able to articulate this clearly. But this is a correctness issue so I am compelled to try again.

In the end the rule is simple: you can not
prevent module unloading from within module
itself. It always must be the caller of your
module that uses some lock to do this.

Quoted 17 lines · Rafael Aquini

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines

Quoted 8 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Quoted 5 lines

Quoted 2 lines

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

CPU1 will be returning to putback_balloon_page() which code is located at core mm/compaction.c, outside the driver.

Sorry, I don't seem to be able to articulate this clearly. But this is a correctness issue so I am compelled to try again.

In the end the rule is simple: you can not
prevent module unloading from within module
itself. It always must be the caller of your
module that uses some lock to do this.

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Rafael Aquini

Quoted 18 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 8 lines

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

Quoted 5 lines

Quoted 2 lines

CPU1 will be returning to putback_balloon_page() which code is located at core mm/compaction.c, outside the driver.

Sorry, I don't seem to be able to articulate this clearly. But this is a correctness issue so I am compelled to try again.

But if there are 0 balloon pages, how is it migrating a page?

Quoted 4 lines

In the end the rule is simple: you can not
prevent module unloading from within module
itself. It always must be the caller of your
module that uses some lock to do this.

Not quite. If you clean up everything in your cleanup function, it also works,

which is what this does, right?

Cheers,
Rusty.

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Rafael Aquini

Quoted 18 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 8 lines

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Yes but only until unlock finishes. The last return from function is not guarded and can be overwritten.

Quoted 5 lines

Quoted 2 lines

CPU1 will be returning to putback_balloon_page() which code is located at core mm/compaction.c, outside the driver.

Sorry, I don't seem to be able to articulate this clearly. But this is a correctness issue so I am compelled to try again.

But if there are 0 balloon pages, how is it migrating a page?

Quoted 4 lines

In the end the rule is simple: you can not
prevent module unloading from within module
itself. It always must be the caller of your
module that uses some lock to do this.

Not quite. If you clean up everything in your cleanup function, it also works,

which is what this does, right?

Cheers,
Rusty.

Quoted 56 lines · Michael S. Tsirkin

Quoted 22 lines · Rusty Russell

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

Quoted 8 lines

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

Cheers,
Rusty.

I know I meant something else.
Let me lay this out:

CPU1 executes:
void virtballoon_putbackpage(struct page *page)

{
	spin_lock(&pages_lock);
	list_add(&page->lru, &vb_ptr->pages);
	spin_unlock(&pages_lock);


		at this point CPU2 unloads module:
						leak_balloon
						......

		next CPU2 loads another module so code memory gets overwritten

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 1 line

No?

At the point CPU2 is unloading the module, it will be kept looping at the snippet Rusty pointed out because the isolation / migration steps do not mess with 'vb->num_pages'. The driver will only unload after leaking the total amount of balloon's inflated pages, which means (for this hypothetical case) CPU2 will wait until CPU1 finishes the putaback procedure.

Quoted 22 lines · Rusty Russell

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

Quoted 8 lines

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

Cheers,
Rusty.

I know I meant something else.
Let me lay this out:

CPU1 executes:
void virtballoon_putbackpage(struct page *page)

{
	spin_lock(&pages_lock);
	list_add(&page->lru, &vb_ptr->pages);
	spin_unlock(&pages_lock);


		at this point CPU2 unloads module:
						leak_balloon
						......

		next CPU2 loads another module so code memory gets overwritten

now CPU1 executes the next instruction:

}

which would normally return to function's caller, but it has been overwritten by CPU2 so we get corruption.

Quoted 17 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 439

Quoted 9 lines · Rafael Aquini

drivers/virtio/virtio_balloon.c +9 -0

+/*
+ * Populate balloon_mapping->a_ops->freepage method to help compaction on
+ * re-inserting an isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+	spin_lock(&pages_lock);
+	list_add(&page->lru, &vb_ptr->pages);
+	spin_unlock(&pages_lock);

Could the following race trigger:
migration happens while module unloading is in progress, module goes away between here and when the function returns, then code for this function gets overwritten? If yes we need locking external to module to prevent this. Maybe add a spinlock to struct address_space?

The balloon module cannot be unloaded until it has leaked all its pages, so I think this is safe:

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

Cheers,
Rusty.

drivers/virtio/virtio_balloon.c static void fill_balloon(struct virtio_balloon *vb, size_t num) · line 155

Quoted 6 lines

drivers/virtio/virtio_balloon.c +1 -0

@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		set_page_pfns(vb->pfns + vb->num_pfns, page);
 		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
+		spin_lock(&pages_lock);
 		list_add(&page->lru, &vb->pages);

If list_add above is reordered with mapping assignment below, then nothing bad happens because balloon_mapping takes pages_lock.

Quoted 21 lines

drivers/virtio/virtio_balloon.c +5 -0

+		page->mapping = balloon_mapping;
+		spin_unlock(&pages_lock);
 	}
 
 	/* Didn't get any?  Oh well. */

@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		return;
 
 	tell_host(vb, vb->inflate_vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -169,10 +183,22 @@ static void 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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		/*

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 199

Quoted 10 lines

drivers/virtio/virtio_balloon.c +9 -0

+		 * We can race against virtballoon_isolatepage() and end up
+		 * stumbling across a _temporarily_ empty 'pages' list.
+		 */
+		spin_lock(&pages_lock);
+		if (unlikely(list_empty(&vb->pages))) {
+			spin_unlock(&pages_lock);
+			break;
+		}
 		page = list_first_entry(&vb->pages, struct page, lru);
+		page->mapping = NULL;

Unlike the case above, here
if = NULL write above is reordered with list_del below, then isolate_page can run on a page that is not on lru.

So I think this needs a wmb().
And maybe a comment above explaining why it is safe?

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 200

Quoted 1 line

drivers/virtio/virtio_balloon.c +0 -0

 		list_del(&page->lru);

I wonder why changing page->lru here is safe against races with unmap_and_move in the previous patch.

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 381

Quoted 4 lines

drivers/virtio/virtio_balloon.c +4 -0

+/*
+ * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
+ * access pertinent elements from struct virtio_balloon
+ */

What if there is more than one balloon device?

drivers/virtio/virtio_balloon.c static int virtballoon_probe(struct virtio_device *vdev) · line 473

Quoted 6 lines

drivers/virtio/virtio_balloon.c +6 -0

+	/* Init the ballooned page->mapping special balloon_mapping */
+	balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
+	if (!balloon_mapping) {
+		err = -ENOMEM;
+		goto out_free_vb;
+	}

Can balloon_mapping be dereferenced at this point? Then what happens?

Quoted 6 lines

drivers/virtio/virtio_balloon.c +5 -0

+
+	INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
+	INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
+	spin_lock_init(&balloon_mapping->tree_lock);
+	balloon_mapping->a_ops = &virtio_balloon_aops;

drivers/virtio/virtio_balloon.c out_del_vqs: · line 496

Quoted 7 lines

drivers/virtio/virtio_balloon.c +1 -0

 	err = init_vqs(vb);
 	if (err)

@@ -373,6 +493,7 @@ out_del_vqs:
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	kfree(vb);
+	kfree(balloon_mapping);

No need to set it to NULL? It seems if someone else allocates a mapping and gets this chunk of memory by chance, the logic in mm will get confused.

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 521

Quoted 8 lines

drivers/virtio/virtio_balloon.c +1 -0

 out:
 	return err;
 }

@@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kthread_stop(vb->thread);
 	remove_common(vb);
 	kfree(vb);
+	kfree(balloon_mapping);

Neither here?

Quoted 11 lines

drivers/virtio/virtio_balloon.c +0 -0

 }
 
 #ifdef CONFIG_PM

include/linux/virtio_balloon.h +0 -0

index 652dc8b..930f1b7 100644
@@ -56,4 +56,8 @@ struct virtio_balloon_stat {
 	u64 val;
 } __attribute__((packed));
 

include/linux/virtio_balloon.h struct virtio_balloon_stat { · line 62

Quoted 4 lines

include/linux/virtio_balloon.h +4 -0

+#if !defined(CONFIG_COMPACTION)
+struct address_space *balloon_mapping;
+#endif
+

Anyone including this header will get a different copy of balloon_mapping. Besides, need to be ifdef KERNEL.

drivers/virtio/virtio_balloon.c static void fill_balloon(struct virtio_balloon *vb, size_t num) · line 155

Quoted 6 lines

drivers/virtio/virtio_balloon.c +1 -0

@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		set_page_pfns(vb->pfns + vb->num_pfns, page);
 		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
+		spin_lock(&pages_lock);
 		list_add(&page->lru, &vb->pages);

If list_add above is reordered with mapping assignment below, then nothing bad happens because balloon_mapping takes pages_lock.

Quoted 21 lines

drivers/virtio/virtio_balloon.c +5 -0

+		page->mapping = balloon_mapping;
+		spin_unlock(&pages_lock);
 	}
 
 	/* Didn't get any?  Oh well. */

@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		return;
 
 	tell_host(vb, vb->inflate_vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -169,10 +183,22 @@ static void 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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		/*

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 199

Quoted 10 lines

drivers/virtio/virtio_balloon.c +9 -0

+		 * We can race against virtballoon_isolatepage() and end up
+		 * stumbling across a _temporarily_ empty 'pages' list.
+		 */
+		spin_lock(&pages_lock);
+		if (unlikely(list_empty(&vb->pages))) {
+			spin_unlock(&pages_lock);
+			break;
+		}
 		page = list_first_entry(&vb->pages, struct page, lru);
+		page->mapping = NULL;

Unlike the case above, here
if = NULL write above is reordered with list_del below, then isolate_page can run on a page that is not on lru.

So I think this needs a wmb().
And maybe a comment above explaining why it is safe?

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 200

Quoted 1 line

drivers/virtio/virtio_balloon.c +0 -0

 		list_del(&page->lru);

I wonder why changing page->lru here is safe against races with unmap_and_move in the previous patch.

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 381

Quoted 4 lines

drivers/virtio/virtio_balloon.c +4 -0

+/*
+ * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
+ * access pertinent elements from struct virtio_balloon
+ */

What if there is more than one balloon device?

drivers/virtio/virtio_balloon.c static int virtballoon_probe(struct virtio_device *vdev) · line 473

Quoted 6 lines

drivers/virtio/virtio_balloon.c +6 -0

+	/* Init the ballooned page->mapping special balloon_mapping */
+	balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
+	if (!balloon_mapping) {
+		err = -ENOMEM;
+		goto out_free_vb;
+	}

Can balloon_mapping be dereferenced at this point? Then what happens?

Quoted 6 lines

drivers/virtio/virtio_balloon.c +5 -0

+
+	INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
+	INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
+	spin_lock_init(&balloon_mapping->tree_lock);
+	balloon_mapping->a_ops = &virtio_balloon_aops;

drivers/virtio/virtio_balloon.c out_del_vqs: · line 496

Quoted 7 lines

drivers/virtio/virtio_balloon.c +1 -0

 	err = init_vqs(vb);
 	if (err)

@@ -373,6 +493,7 @@ out_del_vqs:
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	kfree(vb);
+	kfree(balloon_mapping);

No need to set it to NULL? It seems if someone else allocates a mapping and gets this chunk of memory by chance, the logic in mm will get confused.

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 521

Quoted 8 lines

drivers/virtio/virtio_balloon.c +1 -0

 out:
 	return err;
 }

@@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kthread_stop(vb->thread);
 	remove_common(vb);
 	kfree(vb);
+	kfree(balloon_mapping);

Neither here?

Quoted 11 lines

drivers/virtio/virtio_balloon.c +0 -0

 }
 
 #ifdef CONFIG_PM

include/linux/virtio_balloon.h +0 -0

index 652dc8b..930f1b7 100644
@@ -56,4 +56,8 @@ struct virtio_balloon_stat {
 	u64 val;
 } __attribute__((packed));
 

include/linux/virtio_balloon.h struct virtio_balloon_stat { · line 62

Quoted 4 lines

include/linux/virtio_balloon.h +4 -0

+#if !defined(CONFIG_COMPACTION)
+struct address_space *balloon_mapping;
+#endif
+

Anyone including this header will get a different copy of balloon_mapping. Besides, need to be ifdef KERNEL.

Quoted 48 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static void fill_balloon(struct virtio_balloon *vb, size_t num) · line 155

Quoted 6 lines

drivers/virtio/virtio_balloon.c +1 -0

@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		set_page_pfns(vb->pfns + vb->num_pfns, page);
 		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
+		spin_lock(&pages_lock);
 		list_add(&page->lru, &vb->pages);

If list_add above is reordered with mapping assignment below, then nothing bad happens because balloon_mapping takes pages_lock.

Quoted 21 lines

drivers/virtio/virtio_balloon.c +5 -0

+		page->mapping = balloon_mapping;
+		spin_unlock(&pages_lock);
 	}
 
 	/* Didn't get any?  Oh well. */

@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		return;
 
 	tell_host(vb, vb->inflate_vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -169,10 +183,22 @@ static void 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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		/*

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 199

Quoted 10 lines

drivers/virtio/virtio_balloon.c +9 -0

+		 * We can race against virtballoon_isolatepage() and end up
+		 * stumbling across a _temporarily_ empty 'pages' list.
+		 */
+		spin_lock(&pages_lock);
+		if (unlikely(list_empty(&vb->pages))) {
+			spin_unlock(&pages_lock);
+			break;
+		}
 		page = list_first_entry(&vb->pages, struct page, lru);
+		page->mapping = NULL;

Unlike the case above, here
if = NULL write above is reordered with list_del below, then isolate_page can run on a page that is not on lru.

Quoted 2 lines

So I think this needs a wmb().
And maybe a comment above explaining why it is safe?

Good point. Presumably, this nit has potential to explain your guessing on the read barrier stuff at movable_balloon_page() on the other patch.

Quoted 4 lines

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 200

Quoted 1 line

drivers/virtio/virtio_balloon.c +0 -0

 		list_del(&page->lru);

I wonder why changing page->lru here is safe against races with unmap_and_move in the previous patch.

leak_balloon() doesn't race against unmap_and_move() because the later works on an isolated page set. So, theoretically, pages being dequeued from balloon page list here are either migrated (already) or they were not isolated yet.

Quoted 6 lines

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 381

Quoted 4 lines

drivers/virtio/virtio_balloon.c +4 -0

+/*
+ * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
+ * access pertinent elements from struct virtio_balloon
+ */

What if there is more than one balloon device?

Is it possible to load this driver twice, or are you foreseeing a future case where this driver will be able to manage several distinct memory balloons for the same guest?

Quoted 9 lines

drivers/virtio/virtio_balloon.c static int virtballoon_probe(struct virtio_device *vdev) · line 473

Quoted 6 lines

drivers/virtio/virtio_balloon.c +6 -0

+	/* Init the ballooned page->mapping special balloon_mapping */
+	balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
+	if (!balloon_mapping) {
+		err = -ENOMEM;
+		goto out_free_vb;
+	}

Can balloon_mapping be dereferenced at this point? Then what happens?

Since there's no balloon page enqueued for this balloon driver yet, there's no chance of balloon_mapping being dereferenced at this point.

Quoted 6 lines

Quoted 6 lines

drivers/virtio/virtio_balloon.c +5 -0

+
+	INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
+	INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
+	spin_lock_init(&balloon_mapping->tree_lock);
+	balloon_mapping->a_ops = &virtio_balloon_aops;

Quoted 11 lines

drivers/virtio/virtio_balloon.c out_del_vqs: · line 496

Quoted 7 lines

drivers/virtio/virtio_balloon.c +1 -0

 	err = init_vqs(vb);
 	if (err)

@@ -373,6 +493,7 @@ out_del_vqs:
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	kfree(vb);
+	kfree(balloon_mapping);

No need to set it to NULL? It seems if someone else allocates a mapping and gets this chunk of memory by chance, the logic in mm will get confused.

Good point. It surely doesn't hurt be asured of this sort of safety.

Quoted 10 lines

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 521

Quoted 8 lines

drivers/virtio/virtio_balloon.c +1 -0

 out:
 	return err;
 }

@@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kthread_stop(vb->thread);
 	remove_common(vb);
 	kfree(vb);
+	kfree(balloon_mapping);

Neither here?

ditto.

Quoted 11 lines

Quoted 11 lines

drivers/virtio/virtio_balloon.c +0 -0

 }
 
 #ifdef CONFIG_PM

include/linux/virtio_balloon.h +0 -0

index 652dc8b..930f1b7 100644
@@ -56,4 +56,8 @@ struct virtio_balloon_stat {
 	u64 val;
 } __attribute__((packed));
 

Quoted 7 lines

include/linux/virtio_balloon.h struct virtio_balloon_stat { · line 62

Quoted 4 lines

include/linux/virtio_balloon.h +4 -0

+#if !defined(CONFIG_COMPACTION)
+struct address_space *balloon_mapping;
+#endif
+

Anyone including this header will get a different copy of balloon_mapping. Besides, need to be ifdef KERNEL.

Good point. I'll move this hunk to the balloon driver itself.

Quoted 49 lines · Rafael Aquini

Quoted 48 lines · Michael S. Tsirkin

drivers/virtio/virtio_balloon.c static void fill_balloon(struct virtio_balloon *vb, size_t num) · line 155

Quoted 6 lines

drivers/virtio/virtio_balloon.c +1 -0

@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		set_page_pfns(vb->pfns + vb->num_pfns, page);
 		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
+		spin_lock(&pages_lock);
 		list_add(&page->lru, &vb->pages);

If list_add above is reordered with mapping assignment below, then nothing bad happens because balloon_mapping takes pages_lock.

Quoted 21 lines

drivers/virtio/virtio_balloon.c +5 -0

+		page->mapping = balloon_mapping;
+		spin_unlock(&pages_lock);
 	}
 
 	/* Didn't get any?  Oh well. */

@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 		return;
 
 	tell_host(vb, vb->inflate_vq);
+	mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -169,10 +183,22 @@ static void 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(&balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
 	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		/*

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 199

Quoted 10 lines

drivers/virtio/virtio_balloon.c +9 -0

+		 * We can race against virtballoon_isolatepage() and end up
+		 * stumbling across a _temporarily_ empty 'pages' list.
+		 */
+		spin_lock(&pages_lock);
+		if (unlikely(list_empty(&vb->pages))) {
+			spin_unlock(&pages_lock);
+			break;
+		}
 		page = list_first_entry(&vb->pages, struct page, lru);
+		page->mapping = NULL;

Unlike the case above, here
if = NULL write above is reordered with list_del below, then isolate_page can run on a page that is not on lru.

Quoted 6 lines

Quoted 2 lines

So I think this needs a wmb().
And maybe a comment above explaining why it is safe?

Good point. Presumably, this nit has potential to explain your guessing on the read barrier stuff at movable_balloon_page() on the other patch.

What I think you should do is use rcu for access. And here sync rcu before freeing.
Maybe an overkill but at least a documented synchronization primitive, and it is very light weight.

Quoted 9 lines

Quoted 4 lines

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 200

Quoted 1 line

drivers/virtio/virtio_balloon.c +0 -0

 		list_del(&page->lru);

I wonder why changing page->lru here is safe against races with unmap_and_move in the previous patch.

leak_balloon() doesn't race against unmap_and_move() because the later works on an isolated page set. So, theoretically, pages being dequeued from balloon page list here are either migrated (already) or they were not isolated yet.

So add a comment explaining why it is safe pls.

Quoted 7 lines

Quoted 6 lines

drivers/virtio/virtio_balloon.c static int init_vqs(struct virtio_balloon *vb) · line 381

Quoted 4 lines

drivers/virtio/virtio_balloon.c +4 -0

+/*
+ * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
+ * access pertinent elements from struct virtio_balloon
+ */

What if there is more than one balloon device?