make balloon pages movable by compaction

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 |   15 +++++++
 mm/compaction.c    |  126 ++++++++++++++++++++++++++++++++++++++++++++--------
 mm/migrate.c       |   30 ++++++++++++-
 3 files changed, 151 insertions(+), 20 deletions(-)

include/linux/mm.h +15 -0

index b36d08c..3112198 100644
@@ -1629,5 +1629,20 @@ 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 */
 
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+	return (page->mapping == balloon_mapping) ? true : false;
+}
+#else
+static inline bool is_balloon_page(struct page *page)       { return false; }
+static inline bool isolate_balloon_page(struct page *page)  { return false; }
+static inline bool putback_balloon_page(struct page *page)  { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/compaction.c +25 -0

index 2f42d95..51eac0c 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,85 @@
 #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 +13 -0

+			/*
+			 * A ballooned page, by default, has just one refcount.
+			 * Prevent concurrent compaction threads from isolating
+			 * an already isolated balloon page.
+			 */
+			if (is_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 */

mm/compaction.c +29 -16

+
+	if (likely(trylock_page(page))) {
+		if (is_balloon_page(page)) {
+			__putback_balloon_page(page);
+			put_page(page);
+			unlock_page(page);
+			return true;
+		}
+		unlock_page(page);
+	}
+	return false;
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
+
 static unsigned long release_freepages(struct list_head *freelist)
 {
 	struct page *page, *next;

@@ -312,32 +392,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;

mm/compaction.c +4 -2

+			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 +0 -0

index be26d5c..59c7bc5 100644

mm/migrate.c +10 -0

+		/*
+		 * 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

mm/migrate.c +2 -0

+		return rc;
+	}
 out:
 	if (rc != -EAGAIN) {
 		/*

Signature · 1 line

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * We can race against move_to_new_page() & __unmap_and_move().
+		 * If we stumble across a locked balloon page and succeed on
+		 * isolating it, the result tends to be disastrous.
+		 */
+		if (likely(trylock_page(page))) {

Hmm, I can't understand your comment.
What does this lock protect? Could you elaborate it with code sequence?

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

You already check WARN_ON in putback_lru_pages so we don't need it in here. And you can add likely in here, too.

Review

Quoted 1 line

(quoted diff) +1 -0

+		} else if (is_balloon_page(page)) {

unlikely?

mm/migrate.c void putback_lru_pages(struct list_head *l) · line 84

Quoted 10 lines

mm/migrate.c +4 -1

@@ -78,7 +78,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(is_balloon_page(page)))
+			WARN_ON(!putback_balloon_page(page));
+		else
+			putback_lru_page(page);

Hmm, I don't like this.
The function name says we putback *lru* pages, but balloon page isn't. How about this?

mm/compaction.c +3 -1

index aad0a16..b07cd67 100644
@@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
  * Apart from cc->migratepages and cc->nr_migratetypes this function
  * does not modify any cc's fields, in particular it does not modify
  * (or read for that matter) cc->migrate_pfn.
+ * 
+ * For returning page, you should use putback_pages instead of putback_lru_pages
  */
 unsigned long
 isolate_migratepages_range(struct zone *zone, struct compact_control *cc,

@@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 
                /* Release LRU pages not migrated */
                if (err) {
-                       putback_lru_pages(&cc->migratepages);
+                       putback_pages(&cc->migratepages);
                        cc->nr_migratepages = 0;
                        if (err == -ENOMEM) {
                                ret = COMPACT_PARTIAL;

mm/migrate.c +17 -0

index 9705e70..a96b840 100644
@@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
        }
 }
 
+ /* blah blah .... */ 
+void putback_pages(struct list_head *l)
+{
+       struct page *page;
+       struct page *page2;
+
+       list_for_each_entry_safe(page, page2, l, lru) {
+               list_del(&page->lru);
+               dec_zone_page_state(page, NR_ISOLATED_ANON +
+                               page_is_file_cache(page));
+               if (unlikely(is_balloon_page(page)))
+                       WARN_ON(!putback_balloon_page(page));
+               else
+                       putback_lru_page(page);
+       }
+}
+
 /*
  * Restore a potential migration pte to a working pte entry
  */

mm/page_alloc.c +0 -0

index 32985dd..decb82a 100644

Review void putback_lru_pages(struct list_head *l) · line 85

Quoted 2 lines

(quoted diff) +1 -0

+	if (is_balloon_page(page)) {

unlikely?

Review void putback_lru_pages(struct list_head *l) · line 86

Quoted 1 line

(quoted diff) +1 -0

+	if (is_balloon_page(newpage)) {

unlikely?

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * 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.

mm/migrate.c void putback_lru_pages(struct list_head *l)

Quoted 6 lines

mm/migrate.c +6 -0

+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);

Why do you use __free_page instead of put_page?

The feeling I look at your code in detail is that it makes compaction/migration code rather complicated because compaction/migration assumes source/target would be LRU pages.

How often memory ballooning happens? Does it make sense to hook it in generic functions if it's very rare?

Couldn't you implement it like huge page?
It doesn't make current code complicated and doesn't affect performance

In compaction,
#ifdef CONFIG_VIRTIO_BALLOON
static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)

{
        if (balloon) {
                isolate_balloonpages

                migrate_balloon_pages
                        unmap_and_move_balloon_page

                putback_balloonpages
        }
}

#endif

I'm not sure memory ballooning so it might be dumb question. Can we trigger it once only when ballooning happens?

Howdy Minchan,

Once again, thanks for raising such valuable feedback over here.

Quoted 10 lines · Minchan Kim

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

This check point was set in place because isolate_balloon_page() was a publicly visible function and while our current usage looks correct it would not hurt to have something like that done -- think of it as an insurance policy, in case someone else, in the future, attempts to use it on any other place outside this specifc context.
Despite not seeing it as a dealbreaker for the patch as is, I do agree, however, this snippet can _potentially_ be removed from isolate_balloon_page(), since this function has become static to compaction.c.

Quoted 14 lines

Review

Quoted 10 lines

(quoted diff) +8 -0

+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * We can race against move_to_new_page() & __unmap_and_move().
+		 * If we stumble across a locked balloon page and succeed on
+		 * isolating it, the result tends to be disastrous.
+		 */
+		if (likely(trylock_page(page))) {

Hmm, I can't understand your comment.
What does this lock protect? Could you elaborate it with code sequence?

As we are coping with a corner case -- balloon pages are not on LRU lists -- compaction concurrency can lead to a disastrous race which results in isolate_balloon_page() re-isolating already isolated balloon pages, or isolating a 'newpage' recently promoted to 'balloon page', while these pages are still under migration. The only way we have to prevent that from happening is attempting to grab the page lock, as pages under migration are already locked. I had that comment rephrased to what follows (please, tell me how it sounds to you now)
if (likely(get_page_unless_zero(page))) {

(quoted diff) +0 -0

 		/*

(quoted diff) +0 -0

 		 */
 		if (likely(trylock_page(page))) {
 			/*

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +5 -0

+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

You already check WARN_ON in putback_lru_pages so we don't need it in here. And you can add likely in here, too.

This check point is in place by the same reason why it is for isolate_balloon_page(). However, I don't think we should drop it here because putback_balloon_page() is still a publicly visible symbol. Besides, the check that is performed at putback_lru_pages() level has a different meaning, which is to warn us when we fail on re-inserting an isolated (but not migrated) page back to the balloon page list, thus it does not superceeds nor it's superceeded by this checkpoint here.

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +1 -0

+		} else if (is_balloon_page(page)) {

unlikely?

This can be done, for sure. Also, it reminds me that I had a 'likely(PageLRU(page))' set in place for this vary same patch, on v2 submission. Do you recollect you've asked me to get rid of it?. Wouldn't it be better having that suggestion of yours reverted, since PageLRU(page) is the likelihood case here anyway? What about this?

   " if (likely(PageLRU(page))) { 
       ... 
     } else if (unlikely(is_balloon_page(page))) {
       ...
     } else
	continue;
   "

Quoted 74 lines

mm/migrate.c void putback_lru_pages(struct list_head *l) · line 84

Quoted 11 lines

mm/migrate.c +4 -1

@@ -78,7 +78,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(is_balloon_page(page)))
+			WARN_ON(!putback_balloon_page(page));
+		else
+			putback_lru_page(page);

Hmm, I don't like this.
The function name says we putback *lru* pages, but balloon page isn't. How about this?

mm/compaction.c +3 -1

index aad0a16..b07cd67 100644
@@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
  * Apart from cc->migratepages and cc->nr_migratetypes this function
  * does not modify any cc's fields, in particular it does not modify
  * (or read for that matter) cc->migrate_pfn.
+ * 
+ * For returning page, you should use putback_pages instead of putback_lru_pages
  */
 unsigned long
 isolate_migratepages_range(struct zone *zone, struct compact_control *cc,

@@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 
                /* Release LRU pages not migrated */
                if (err) {
-                       putback_lru_pages(&cc->migratepages);
+                       putback_pages(&cc->migratepages);
                        cc->nr_migratepages = 0;
                        if (err == -ENOMEM) {
                                ret = COMPACT_PARTIAL;

mm/migrate.c +17 -0

index 9705e70..a96b840 100644
@@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
        }
 }
 
+ /* blah blah .... */ 
+void putback_pages(struct list_head *l)
+{
+       struct page *page;
+       struct page *page2;
+
+       list_for_each_entry_safe(page, page2, l, lru) {
+               list_del(&page->lru);
+               dec_zone_page_state(page, NR_ISOLATED_ANON +
+                               page_is_file_cache(page));
+               if (unlikely(is_balloon_page(page)))
+                       WARN_ON(!putback_balloon_page(page));
+               else
+                       putback_lru_page(page);
+       }
+}
+
 /*
  * Restore a potential migration pte to a working pte entry
  */

mm/page_alloc.c +0 -0

index 32985dd..decb82a 100644
@@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
                                    0, false, MIGRATE_SYNC);
        }
 

mm/page_alloc.c static int __alloc_contig_migrate_range(unsigned long start, unsigned long end) · line 5661

Quoted 5 lines

mm/page_alloc.c +1 -1

-       putback_lru_pages(&cc.migratepages);
+       putback_pages(&cc.migratepages);
        return ret > 0 ? 0 : ret;
 }
 

Despite being a very nice cleanup, this code refactoring has nothing to do with this particular patch purpose. For the sake of this implementation, think about the balloon page list acting as a particular LRU, so although ballooned pages are not enlisted on real LRUs, per se, this doesn't mean we cannot have them dealt as a corner case amongst putback_lru_pages() code for the sake of simplicity and maintainability ease. OTOH, I do agree with your nice suggestion, thus I can submit it as a separate clean-up patch attached to this series (if you don't mind).

Quoted 7 lines

Review void putback_lru_pages(struct list_head *l) · line 85

Quoted 4 lines

(quoted diff) +1 -0

+	if (is_balloon_page(page)) {

unlikely?

Yeah, why not? Will do it.

Quoted 4 lines

Review void putback_lru_pages(struct list_head *l) · line 86

Quoted 1 line

(quoted diff) +1 -0

+	if (is_balloon_page(newpage)) {

unlikely?

ditto.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * 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.

Quoted 9 lines

mm/migrate.c void putback_lru_pages(struct list_head *l)

Quoted 6 lines

mm/migrate.c +6 -0

+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);

Why do you use __free_page instead of put_page?

Do you mean perform an extra put_page() and let putback_lru_page() do the rest? Besides looking odd at code level, what other improvement we can potentially gain here?

Quoted 25 lines

The feeling I look at your code in detail is that it makes compaction/migration code rather complicated because compaction/migration assumes source/target would be LRU pages.

How often memory ballooning happens? Does it make sense to hook it in generic functions if it's very rare?

Couldn't you implement it like huge page?
It doesn't make current code complicated and doesn't affect performance

In compaction,
#ifdef CONFIG_VIRTIO_BALLOON
static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)

{
        if (balloon) {
                isolate_balloonpages

                migrate_balloon_pages
                        unmap_and_move_balloon_page

                putback_balloonpages
        }
}

Quoted 4 lines

#endif

I'm not sure memory ballooning so it might be dumb question. Can we trigger it once only when ballooning happens?

I strongly believe, this is the simplest and easiest way to get this task accomplished, mostly becasue:
i. It does not require any code duplication at all;
ii. It requires very few code insertion/surgery to be fully operative;
iii. It is embeded on already well established and maintained code;
iv. The approach makes easier to other balloon drivers leverage compaction
code;
v. It shows no significative impact to the entangled code paths;

Hi Rafael,

Quoted 16 lines · Rafael Aquini

Howdy Minchan,

Once again, thanks for raising such valuable feedback over here.

Quoted 10 lines · Minchan Kim

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

Quoted 8 lines

This check point was set in place because isolate_balloon_page() was a publicly visible function and while our current usage looks correct it would not hurt to have something like that done -- think of it as an insurance policy, in case someone else, in the future, attempts to use it on any other place outside this specifc context.
Despite not seeing it as a dealbreaker for the patch as is, I do agree, however, this snippet can _potentially_ be removed from isolate_balloon_page(), since this function has become static to compaction.c.

Yes. It's not static.

Hi Rafael,

Quoted 16 lines · Rafael Aquini

Howdy Minchan,

Once again, thanks for raising such valuable feedback over here.

Quoted 10 lines · Minchan Kim

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

Quoted 8 lines

This check point was set in place because isolate_balloon_page() was a publicly visible function and while our current usage looks correct it would not hurt to have something like that done -- think of it as an insurance policy, in case someone else, in the future, attempts to use it on any other place outside this specifc context.
Despite not seeing it as a dealbreaker for the patch as is, I do agree, however, this snippet can _potentially_ be removed from isolate_balloon_page(), since this function has become static to compaction.c.

Yes. It's not static.

Hi Rafael,

Quoted 16 lines · Rafael Aquini

Howdy Minchan,

Once again, thanks for raising such valuable feedback over here.

Quoted 10 lines · Minchan Kim

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

Quoted 8 lines

This check point was set in place because isolate_balloon_page() was a publicly visible function and while our current usage looks correct it would not hurt to have something like that done -- think of it as an insurance policy, in case someone else, in the future, attempts to use it on any other place outside this specifc context.
Despite not seeing it as a dealbreaker for the patch as is, I do agree, however, this snippet can _potentially_ be removed from isolate_balloon_page(), since this function has become static to compaction.c.

Yes. It's not static.

Quoted 25 lines

Quoted 14 lines

Review

Quoted 10 lines

(quoted diff) +8 -0

+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * We can race against move_to_new_page() & __unmap_and_move().
+		 * If we stumble across a locked balloon page and succeed on
+		 * isolating it, the result tends to be disastrous.
+		 */
+		if (likely(trylock_page(page))) {

Hmm, I can't understand your comment.
What does this lock protect? Could you elaborate it with code sequence?

As we are coping with a corner case -- balloon pages are not on LRU lists -- compaction concurrency can lead to a disastrous race which results in isolate_balloon_page() re-isolating already isolated balloon pages, or isolating a 'newpage' recently promoted to 'balloon page', while these pages are still under migration. The only way we have to prevent that from happening is attempting to grab the page lock, as pages under migration are already locked. I had that comment rephrased to what follows (please, tell me how it sounds to you now)
if (likely(get_page_unless_zero(page))) {

(quoted diff) +0 -0

 		/*

Review

Quoted 10 lines

(quoted diff) +7 -3

-		 * We can race against move_to_new_page() & __unmap_and_move().
-		 * If we stumble across a locked balloon page and succeed on
-		 * isolating it, the result tends to be disastrous.
+		 * As balloon pages are not isolated from LRU lists, several
+		 * concurrent compaction threads will potentially race against
+		 * page migration at 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.

Looks good to me.

Quoted 15 lines

(quoted diff) +0 -0

 		 */
 		if (likely(trylock_page(page))) {
 			/*

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +5 -0

+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

You already check WARN_ON in putback_lru_pages so we don't need it in here. And you can add likely in here, too.

Quoted 7 lines

This check point is in place by the same reason why it is for isolate_balloon_page(). However, I don't think we should drop it here because putback_balloon_page() is still a publicly visible symbol. Besides, the check that is performed at putback_lru_pages() level has a different meaning, which is to warn us when we fail on re-inserting an isolated (but not migrated) page back to the balloon page list, thus it does not superceeds nor it's superceeded by this checkpoint here.

I'm not against you strongly but IMHO, putback_balloon_page is never generic function which is likely to be used by many others so I hope not overdesign.

Quoted 12 lines

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +1 -0

+		} else if (is_balloon_page(page)) {

unlikely?

This can be done, for sure. Also, it reminds me that I had a 'likely(PageLRU(page))' set in place for this vary same patch, on v2 submission. Do you recollect you've asked me to get rid of it?. Wouldn't it be better having that suggestion of yours reverted, since PageLRU(page) is the likelihood case here anyway? What about this?

Quoted 7 lines

   " if (likely(PageLRU(page))) { 
       ... 
     } else if (unlikely(is_balloon_page(page))) {
       ...
     } else
	continue;
   "

I don't like that.
PageLRU isn't likelihood case in this.

Quoted 74 lines

Quoted 74 lines

mm/migrate.c void putback_lru_pages(struct list_head *l) · line 84

Quoted 11 lines

mm/migrate.c +4 -1

@@ -78,7 +78,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(is_balloon_page(page)))
+			WARN_ON(!putback_balloon_page(page));
+		else
+			putback_lru_page(page);

Hmm, I don't like this.
The function name says we putback *lru* pages, but balloon page isn't. How about this?

mm/compaction.c +3 -1

index aad0a16..b07cd67 100644
@@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
  * Apart from cc->migratepages and cc->nr_migratetypes this function
  * does not modify any cc's fields, in particular it does not modify
  * (or read for that matter) cc->migrate_pfn.
+ * 
+ * For returning page, you should use putback_pages instead of putback_lru_pages
  */
 unsigned long
 isolate_migratepages_range(struct zone *zone, struct compact_control *cc,

@@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 
                /* Release LRU pages not migrated */
                if (err) {
-                       putback_lru_pages(&cc->migratepages);
+                       putback_pages(&cc->migratepages);
                        cc->nr_migratepages = 0;
                        if (err == -ENOMEM) {
                                ret = COMPACT_PARTIAL;

mm/migrate.c +17 -0

index 9705e70..a96b840 100644
@@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
        }
 }
 
+ /* blah blah .... */ 
+void putback_pages(struct list_head *l)
+{
+       struct page *page;
+       struct page *page2;
+
+       list_for_each_entry_safe(page, page2, l, lru) {
+               list_del(&page->lru);
+               dec_zone_page_state(page, NR_ISOLATED_ANON +
+                               page_is_file_cache(page));
+               if (unlikely(is_balloon_page(page)))
+                       WARN_ON(!putback_balloon_page(page));
+               else
+                       putback_lru_page(page);
+       }
+}
+
 /*
  * Restore a potential migration pte to a working pte entry
  */

mm/page_alloc.c +0 -0

index 32985dd..decb82a 100644
@@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
                                    0, false, MIGRATE_SYNC);
        }
 

Quoted 14 lines

mm/page_alloc.c static int __alloc_contig_migrate_range(unsigned long start, unsigned long end) · line 5661

Quoted 5 lines

mm/page_alloc.c +1 -1

-       putback_lru_pages(&cc.migratepages);
+       putback_pages(&cc.migratepages);
        return ret > 0 ? 0 : ret;
 }
 

Despite being a very nice cleanup, this code refactoring has nothing to do with this particular patch purpose. For the sake of this implementation, think about the balloon page list acting as a particular LRU, so although ballooned pages are not enlisted on real LRUs, per se, this doesn't mean we cannot have them dealt as a corner case amongst putback_lru_pages() code for the sake of simplicity and maintainability ease. OTOH, I do agree with your nice suggestion, thus I can submit it as a separate clean-up patch attached to this series (if you don't mind).

No problem. But the concern is that I can't agree this patch implementation. Please see below.

Quoted 31 lines

Quoted 7 lines

Review void putback_lru_pages(struct list_head *l) · line 85

Quoted 4 lines

(quoted diff) +1 -0

+	if (is_balloon_page(page)) {

unlikely?

Yeah, why not? Will do it.

Quoted 4 lines

Review void putback_lru_pages(struct list_head *l) · line 86

Quoted 1 line

(quoted diff) +1 -0

+	if (is_balloon_page(newpage)) {

unlikely?

ditto.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * 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.

Quoted 9 lines

mm/migrate.c void putback_lru_pages(struct list_head *l)

Quoted 6 lines

mm/migrate.c +6 -0

+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);

Why do you use __free_page instead of put_page?

Quoted 2 lines

Do you mean perform an extra put_page() and let putback_lru_page() do the rest? Besides looking odd at code level, what other improvement we can potentially gain here?

I mean just do
put_page(page); /* blah blah */ put_page(page); /* It will free the page to buddy */

Yeb it's more fat than __free_page for your case. If so, please comment write out why you use __free_page instead of put_page.

/*
* balloon page should be never compund page and not on the LRU so we
* call __free_page directly instead of put_page.
*/

Quoted 30 lines

Quoted 25 lines

The feeling I look at your code in detail is that it makes compaction/migration code rather complicated because compaction/migration assumes source/target would be LRU pages.

How often memory ballooning happens? Does it make sense to hook it in generic functions if it's very rare?

Couldn't you implement it like huge page?
It doesn't make current code complicated and doesn't affect performance

In compaction,
#ifdef CONFIG_VIRTIO_BALLOON
static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)

{
        if (balloon) {
                isolate_balloonpages

                migrate_balloon_pages
                        unmap_and_move_balloon_page

                putback_balloonpages
        }
}

Quoted 4 lines

#endif

I'm not sure memory ballooning so it might be dumb question. Can we trigger it once only when ballooning happens?

Quoted 8 lines

I strongly believe, this is the simplest and easiest way to get this task accomplished, mostly becasue:
i. It does not require any code duplication at all;
ii. It requires very few code insertion/surgery to be fully operative;
iii. It is embeded on already well established and maintained code;
iv. The approach makes easier to other balloon drivers leverage compaction
code;
v. It shows no significative impact to the entangled code paths;

But it is adding a few hooks to generic functions which all assume they are LRU pages. Although it's trivial for performance, we don't need get such loss due to very infrequent event.

More concern to me that it makes code complicated because we assume functions related migration/compaction depend on LRU pages. Let me say a example.
suitable_migration_target in isolate_freepages returns true pageblock type is MIGRATE_MOVABLE or MIGRATE_CMA which expect pages in that block is migratable so they should be moved into another location or reclaimed if we have no space for getting big contiguos space.
All of such code should consider balloon page but balloon page can't be reclaimed unlike normal LRU page so it makes design more complex.

Look at memory-hotplug, offline_page calls has_unmovable_pages, scan_lru_pages and do_migrate_range which calls isolate_lru_page. They consider only LRU pages to migratable ones.

IMHO, better approach is that after we can get complete free pageblocks by compaction or reclaim, move balloon pages into that pageblocks and make that blocks to unmovable. It can prevent fragmentation and it makes current or future code don't need to consider balloon page.

Of course, it needs more code but I think it's valuable. What do you think about it?

Signature · 2 lines

Kind regards,
Minchan Kim

In a glance, I believe this whole dance you're suggesting might just be too much of an overcomplication, and the best approach would be simply teaching the hotplug bits about the ballooned corner case just like it's being done to compaction/migration. However, I'll look at it carefully before making any other adjustments/propositions over here.

In a glance, I believe this whole dance you're suggesting might just be too much of an overcomplication, and the best approach would be simply teaching the hotplug bits about the ballooned corner case just like it's being done to compaction/migration. However, I'll look at it carefully before making any other adjustments/propositions over here.

Quoted 5 lines · Rafael Aquini

In a glance, I believe this whole dance you're suggesting might just be too much of an overcomplication, and the best approach would be simply teaching the hotplug bits about the ballooned corner case just like it's being done to compaction/migration. However, I'll look at it carefully before making any other adjustments/propositions over here.

Compaction and hotplug do essentially the same thing
here: "collect all the movable pages from a page block, and move them elsewhere".

Whether or not it is easier for them to share code, or to duplicate a few lines of code, is something that can be looked into later.

IMHO, better approach is that after we can get complete free pageblocks by compaction or reclaim, move balloon pages into that pageblocks and make that blocks to unmovable. It can prevent fragmentation and it makes current or future code don't need to consider balloon page.

IMHO, better approach is that after we can get complete free pageblocks by compaction or reclaim, move balloon pages into that pageblocks and make that blocks to unmovable. It can prevent fragmentation and it makes current or future code don't need to consider balloon page.

Look at memory-hotplug, offline_page calls has_unmovable_pages, scan_lru_pages and do_migrate_range which calls isolate_lru_page. They consider only LRU pages to migratable ones.

IMHO, better approach is that after we can get complete free pageblocks by compaction or reclaim, move balloon pages into that pageblocks and make that blocks to unmovable. It can prevent fragmentation and it makes current or future code don't need to consider balloon page.

Look at memory-hotplug, offline_page calls has_unmovable_pages, scan_lru_pages and do_migrate_range which calls isolate_lru_page. They consider only LRU pages to migratable ones.

IMHO, better approach is that after we can get complete free pageblocks by compaction or reclaim, move balloon pages into that pageblocks and make that blocks to unmovable. It can prevent fragmentation and it makes current or future code don't need to consider balloon page.

Howdy Minchan,

Once again, thanks for raising such valuable feedback over here.

Quoted 10 lines · Minchan Kim

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

I am not sure we need this because you alreay check it before calling isolate_balloon_page. If you really need it, it would be better to add likely in isolate_balloon_page, too.

This check point was set in place because isolate_balloon_page() was a publicly visible function and while our current usage looks correct it would not hurt to have something like that done -- think of it as an insurance policy, in case someone else, in the future, attempts to use it on any other place outside this specifc context.
Despite not seeing it as a dealbreaker for the patch as is, I do agree, however, this snippet can _potentially_ be removed from isolate_balloon_page(), since this function has become static to compaction.c.

Quoted 14 lines

Review

Quoted 10 lines

(quoted diff) +8 -0

+
+	if (likely(get_page_unless_zero(page))) {
+		/*
+		 * We can race against move_to_new_page() & __unmap_and_move().
+		 * If we stumble across a locked balloon page and succeed on
+		 * isolating it, the result tends to be disastrous.
+		 */
+		if (likely(trylock_page(page))) {

Hmm, I can't understand your comment.
What does this lock protect? Could you elaborate it with code sequence?

As we are coping with a corner case -- balloon pages are not on LRU lists -- compaction concurrency can lead to a disastrous race which results in isolate_balloon_page() re-isolating already isolated balloon pages, or isolating a 'newpage' recently promoted to 'balloon page', while these pages are still under migration. The only way we have to prevent that from happening is attempting to grab the page lock, as pages under migration are already locked. I had that comment rephrased to what follows (please, tell me how it sounds to you now)
if (likely(get_page_unless_zero(page))) {

(quoted diff) +0 -0

 		/*

(quoted diff) +0 -0

 		 */
 		if (likely(trylock_page(page))) {
 			/*

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +5 -0

+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;

You already check WARN_ON in putback_lru_pages so we don't need it in here. And you can add likely in here, too.

This check point is in place by the same reason why it is for isolate_balloon_page(). However, I don't think we should drop it here because putback_balloon_page() is still a publicly visible symbol. Besides, the check that is performed at putback_lru_pages() level has a different meaning, which is to warn us when we fail on re-inserting an isolated (but not migrated) page back to the balloon page list, thus it does not superceeds nor it's superceeded by this checkpoint here.

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +1 -0

+		} else if (is_balloon_page(page)) {

unlikely?

This can be done, for sure. Also, it reminds me that I had a 'likely(PageLRU(page))' set in place for this vary same patch, on v2 submission. Do you recollect you've asked me to get rid of it?. Wouldn't it be better having that suggestion of yours reverted, since PageLRU(page) is the likelihood case here anyway? What about this?

   " if (likely(PageLRU(page))) { 
       ... 
     } else if (unlikely(is_balloon_page(page))) {
       ...
     } else
	continue;
   "

Quoted 74 lines

mm/migrate.c void putback_lru_pages(struct list_head *l) · line 84

Quoted 11 lines

mm/migrate.c +4 -1

@@ -78,7 +78,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(is_balloon_page(page)))
+			WARN_ON(!putback_balloon_page(page));
+		else
+			putback_lru_page(page);

Hmm, I don't like this.
The function name says we putback *lru* pages, but balloon page isn't. How about this?

mm/compaction.c +3 -1

index aad0a16..b07cd67 100644
@@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
  * Apart from cc->migratepages and cc->nr_migratetypes this function
  * does not modify any cc's fields, in particular it does not modify
  * (or read for that matter) cc->migrate_pfn.
+ * 
+ * For returning page, you should use putback_pages instead of putback_lru_pages
  */
 unsigned long
 isolate_migratepages_range(struct zone *zone, struct compact_control *cc,

@@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 
                /* Release LRU pages not migrated */
                if (err) {
-                       putback_lru_pages(&cc->migratepages);
+                       putback_pages(&cc->migratepages);
                        cc->nr_migratepages = 0;
                        if (err == -ENOMEM) {
                                ret = COMPACT_PARTIAL;

mm/migrate.c +17 -0

index 9705e70..a96b840 100644
@@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
        }
 }
 
+ /* blah blah .... */ 
+void putback_pages(struct list_head *l)
+{
+       struct page *page;
+       struct page *page2;
+
+       list_for_each_entry_safe(page, page2, l, lru) {
+               list_del(&page->lru);
+               dec_zone_page_state(page, NR_ISOLATED_ANON +
+                               page_is_file_cache(page));
+               if (unlikely(is_balloon_page(page)))
+                       WARN_ON(!putback_balloon_page(page));
+               else
+                       putback_lru_page(page);
+       }
+}
+
 /*
  * Restore a potential migration pte to a working pte entry
  */

mm/page_alloc.c +0 -0

index 32985dd..decb82a 100644
@@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
                                    0, false, MIGRATE_SYNC);
        }
 

mm/page_alloc.c static int __alloc_contig_migrate_range(unsigned long start, unsigned long end) · line 5661

Quoted 5 lines

mm/page_alloc.c +1 -1

-       putback_lru_pages(&cc.migratepages);
+       putback_pages(&cc.migratepages);
        return ret > 0 ? 0 : ret;
 }
 

Despite being a very nice cleanup, this code refactoring has nothing to do with this particular patch purpose. For the sake of this implementation, think about the balloon page list acting as a particular LRU, so although ballooned pages are not enlisted on real LRUs, per se, this doesn't mean we cannot have them dealt as a corner case amongst putback_lru_pages() code for the sake of simplicity and maintainability ease. OTOH, I do agree with your nice suggestion, thus I can submit it as a separate clean-up patch attached to this series (if you don't mind).

Quoted 7 lines

Review void putback_lru_pages(struct list_head *l) · line 85

Quoted 4 lines

(quoted diff) +1 -0

+	if (is_balloon_page(page)) {

unlikely?

Yeah, why not? Will do it.

Quoted 4 lines

Review void putback_lru_pages(struct list_head *l) · line 86

Quoted 1 line

(quoted diff) +1 -0

+	if (is_balloon_page(newpage)) {

unlikely?

ditto.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * 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.

Quoted 9 lines

mm/migrate.c void putback_lru_pages(struct list_head *l)

Quoted 6 lines

mm/migrate.c +6 -0

+		 */
+		list_del(&page->lru);
+		dec_zone_page_state(page, NR_ISOLATED_ANON +
+				    page_is_file_cache(page));
+		put_page(page);
+		__free_page(page);

Why do you use __free_page instead of put_page?

Do you mean perform an extra put_page() and let putback_lru_page() do the rest? Besides looking odd at code level, what other improvement we can potentially gain here?

Quoted 25 lines

The feeling I look at your code in detail is that it makes compaction/migration code rather complicated because compaction/migration assumes source/target would be LRU pages.

How often memory ballooning happens? Does it make sense to hook it in generic functions if it's very rare?

Couldn't you implement it like huge page?
It doesn't make current code complicated and doesn't affect performance

In compaction,
#ifdef CONFIG_VIRTIO_BALLOON
static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)

{
        if (balloon) {
                isolate_balloonpages

                migrate_balloon_pages
                        unmap_and_move_balloon_page

                putback_balloonpages
        }
}

Quoted 4 lines

#endif

I'm not sure memory ballooning so it might be dumb question. Can we trigger it once only when ballooning happens?

I strongly believe, this is the simplest and easiest way to get this task accomplished, mostly becasue:
i. It does not require any code duplication at all;
ii. It requires very few code insertion/surgery to be fully operative;
iii. It is embeded on already well established and maintained code;
iv. The approach makes easier to other balloon drivers leverage compaction
code;
v. It shows no significative impact to the entangled code paths;

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 |   15 +++++++
 mm/compaction.c    |  126 ++++++++++++++++++++++++++++++++++++++++++++--------
 mm/migrate.c       |   30 ++++++++++++-
 3 files changed, 151 insertions(+), 20 deletions(-)

include/linux/mm.h +15 -0

index b36d08c..3112198 100644
@@ -1629,5 +1629,20 @@ 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 */
 
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+	return (page->mapping == balloon_mapping) ? true : false;
+}
+#else
+static inline bool is_balloon_page(struct page *page)       { return false; }
+static inline bool isolate_balloon_page(struct page *page)  { return false; }
+static inline bool putback_balloon_page(struct page *page)  { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/compaction.c +25 -0

index 2f42d95..51eac0c 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,85 @@
 #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 +13 -0

+			/*
+			 * A ballooned page, by default, has just one refcount.
+			 * Prevent concurrent compaction threads from isolating
+			 * an already isolated balloon page.
+			 */
+			if (is_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 */

mm/compaction.c +29 -16

+
+	if (likely(trylock_page(page))) {
+		if (is_balloon_page(page)) {
+			__putback_balloon_page(page);
+			put_page(page);
+			unlock_page(page);
+			return true;
+		}
+		unlock_page(page);
+	}
+	return false;
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
+
 static unsigned long release_freepages(struct list_head *freelist)
 {
 	struct page *page, *next;

@@ -312,32 +392,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;

mm/compaction.c +4 -2

+			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 +0 -0

index be26d5c..59c7bc5 100644

mm/migrate.c +10 -0

+		/*
+		 * 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

mm/migrate.c +2 -0

+		return rc;
+	}
 out:
 	if (rc != -EAGAIN) {
 		/*

Signature · 1 line

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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.

...

include/linux/mm.h +0 -0

@@ -1629,5 +1629,20 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
 static inline bool page_is_guard(struct page *page) { return false; }

Quoted 11 lines · On Tue, 17 Jul 2012 13:50:41 -0300 Rafael Aquini <[email protected]> wrote:

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.

...

include/linux/mm.h +0 -0

@@ -1629,5 +1629,20 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
 static inline bool page_is_guard(struct page *page) { return false; }

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

Quoted 10 lines

include/linux/mm.h +8 -0

 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+	return (page->mapping == balloon_mapping) ? true : false;

You can simply do

	return page->mapping == balloon_mapping;

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

Quoted 7 lines

include/linux/mm.h +6 -0

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

This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n, is_balloon_page() will always return NULL. IOW, no pages are balloon pages! This is wrong.

I'm not sure what to do about this, apart from renaming the function to is_compactible_balloon_page() or something similarly aawkward.

Quoted 11 lines · On Tue, 17 Jul 2012 13:50:41 -0300 Rafael Aquini <[email protected]> wrote:

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.

...

include/linux/mm.h +0 -0

@@ -1629,5 +1629,20 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
 static inline bool page_is_guard(struct page *page) { return false; }

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

Quoted 10 lines

include/linux/mm.h +8 -0

 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+	return (page->mapping == balloon_mapping) ? true : false;

You can simply do

	return page->mapping == balloon_mapping;

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

Quoted 7 lines

include/linux/mm.h +6 -0

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

This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n, is_balloon_page() will always return NULL. IOW, no pages are balloon pages! This is wrong.

I'm not sure what to do about this, apart from renaming the function to is_compactible_balloon_page() or something similarly aawkward.

Quoted 12 lines

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

Quoted 7 lines

include/linux/mm.h +6 -0

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

This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n, is_balloon_page() will always return NULL. IOW, no pages are balloon pages! This is wrong.

I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
For such config case we cannot perform the is_balloon_page() test branches placed on mm/migration.c

Quoted 12 lines · On Wed, 18 Jul 2012 20:07:07 -0300 Rafael Aquini <[email protected]> wrote:

Quoted 12 lines

Review

Quoted 7 lines

(quoted diff) +6 -0

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

This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n, is_balloon_page() will always return NULL. IOW, no pages are balloon pages! This is wrong.

Quoted 4 lines

I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
For such config case we cannot perform the is_balloon_page() test branches placed on mm/migration.c

No, it isn't right. Look at the name: "is_balloon_page". If a caller runs is_balloon_page() against a balloon page with CONFIG_COMPACTION=n then they will get "false", which is incorrect.

So the function needs a better name - one which communicates that it is a balloon page *for the purposes of processing by the compaction code*. Making the function private to compaction.c would help with that, if feasible.

Quoted 12 lines · On Wed, 18 Jul 2012 20:07:07 -0300 Rafael Aquini <[email protected]> wrote:

Quoted 12 lines

Review

Quoted 7 lines

(quoted diff) +6 -0

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

This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n, is_balloon_page() will always return NULL. IOW, no pages are balloon pages! This is wrong.

Quoted 4 lines

I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
For such config case we cannot perform the is_balloon_page() test branches placed on mm/migration.c

No, it isn't right. Look at the name: "is_balloon_page". If a caller runs is_balloon_page() against a balloon page with CONFIG_COMPACTION=n then they will get "false", which is incorrect.

So the function needs a better name - one which communicates that it is a balloon page *for the purposes of processing by the compaction code*. Making the function private to compaction.c would help with that, if feasible.

Quoted 5 lines

So the function needs a better name - one which communicates that it is a balloon page *for the purposes of processing by the compaction code*. Making the function private to compaction.c would help with that, if feasible.

How about this (adjusted) approach:

include/linux/mm.h +7 -6

@@ -1629,8 +1629,7 @@ 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 */
 
-#if (defined(CONFIG_VIRTIO_BALLOON) || \
-	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
 extern bool putback_balloon_page(struct page *);
 extern struct address_space *balloon_mapping;

@@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
 {
 	return (page->mapping && page->mapping == balloon_mapping);
 }
+#if defined(CONFIG_COMPACTION)
+static inline bool balloon_compaction_enabled(void) { return true; }
 #else
-static inline bool is_balloon_page(struct page *page)       { return false; }
-static inline bool isolate_balloon_page(struct page *page)  { return false; }
-static inline bool putback_balloon_page(struct page *page)  { return false; }
-#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool balloon_compaction_enabled(void) { return false; }
+#endif /* CONFIG_COMPACTION */
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/migrate.c +0 -0

index 59c7bc5..f5f6a7d 100644

Quoted 6 lines · On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <[email protected]> wrote:

Quoted 5 lines

So the function needs a better name - one which communicates that it is a balloon page *for the purposes of processing by the compaction code*. Making the function private to compaction.c would help with that, if feasible.

Quoted 1 line

How about this (adjusted) approach:

it fails checkpatch ;)

Quoted 38 lines

include/linux/mm.h +7 -6

@@ -1629,8 +1629,7 @@ 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 */
 
-#if (defined(CONFIG_VIRTIO_BALLOON) || \
-	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
 extern bool putback_balloon_page(struct page *);
 extern struct address_space *balloon_mapping;

@@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
 {
 	return (page->mapping && page->mapping == balloon_mapping);
 }
+#if defined(CONFIG_COMPACTION)
+static inline bool balloon_compaction_enabled(void) { return true; }
 #else
-static inline bool is_balloon_page(struct page *page)       { return false; }
-static inline bool isolate_balloon_page(struct page *page)  { return false; }
-static inline bool putback_balloon_page(struct page *page)  { return false; }
-#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool balloon_compaction_enabled(void) { return false; }
+#endif /* CONFIG_COMPACTION */
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/migrate.c +0 -0

index 59c7bc5..f5f6a7d 100644
@@ -78,7 +78,8 @@ void putback_lru_pages(struct list_head *l)
 		list_del(&page->lru);
 		dec_zone_page_state(page, NR_ISOLATED_ANON +

mm/migrate.c void putback_lru_pages(struct list_head *l) · line 82

Quoted 4 lines

mm/migrate.c +2 -1

 				page_is_file_cache(page));
-		if (unlikely(is_balloon_page(page)))
+		if (unlikely(is_balloon_page(page)) &&
+		    balloon_compaction_enabled())

well, that helps readability. But what does is_balloon_page() return when invoked on a balloon page when CONFIG_COMPACTION=n? False, methinks.

I think the code as you previously had it was OK, but the is_balloon_page() name is misleading. It really wants to be called is_potentially_compactible_balloon_page() :( Maybe rename it to compactible_balloon_page()?

Quoted 5 lines

So the function needs a better name - one which communicates that it is a balloon page *for the purposes of processing by the compaction code*. Making the function private to compaction.c would help with that, if feasible.

How about this (adjusted) approach:

include/linux/mm.h +7 -6

@@ -1629,8 +1629,7 @@ 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 */
 
-#if (defined(CONFIG_VIRTIO_BALLOON) || \
-	defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
 extern bool putback_balloon_page(struct page *);
 extern struct address_space *balloon_mapping;

@@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
 {
 	return (page->mapping && page->mapping == balloon_mapping);
 }
+#if defined(CONFIG_COMPACTION)
+static inline bool balloon_compaction_enabled(void) { return true; }
 #else
-static inline bool is_balloon_page(struct page *page)       { return false; }
-static inline bool isolate_balloon_page(struct page *page)  { return false; }
-static inline bool putback_balloon_page(struct page *page)  { return false; }
-#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool balloon_compaction_enabled(void) { return false; }
+#endif /* CONFIG_COMPACTION */
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

mm/migrate.c +0 -0

index 59c7bc5..f5f6a7d 100644

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;

...

Quoted 13 lines · On Tue, 17 Jul 2012 13:50:42 -0300 Rafael Aquini <[email protected]> wrote:

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;

...

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

Quoted 2 lines

drivers/virtio/virtio_balloon.c +2 -0

+	balloon_mapping->a_ops = &virtio_balloon_aops;
+	balloon_mapping->backing_dev_info = (void *)vb;

hoo boy. We're making page->mapping->backing_dev_info point at a struct which does not have type `struct backing_dev_info'. And then we are exposing that page to core MM functions. So we're hoping that core MM will never walk down page->mapping->backing_dev_info and explode.

That's nasty, hacky and fragile.

Quoted 13 lines · On Tue, 17 Jul 2012 13:50:42 -0300 Rafael Aquini <[email protected]> wrote:

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;

...

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

Quoted 2 lines

drivers/virtio/virtio_balloon.c +2 -0

+	balloon_mapping->a_ops = &virtio_balloon_aops;
+	balloon_mapping->backing_dev_info = (void *)vb;

hoo boy. We're making page->mapping->backing_dev_info point at a struct which does not have type `struct backing_dev_info'. And then we are exposing that page to core MM functions. So we're hoping that core MM will never walk down page->mapping->backing_dev_info and explode.

That's nasty, hacky and fragile.