[PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

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

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..3112198 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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 */
diff --git a/mm/compaction.c b/mm/compaction.c
index 2f42d95..51eac0c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -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);
+}
+
+static inline void __putback_balloon_page(struct page *page)
+{
+	page->mapping->a_ops->freepage(page);
+}
+
+/* __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;
+
+	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))) {
+			/*
+			 * 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 */
+		put_page(page);
+	}
+	return false;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+	if (WARN_ON(!is_balloon_page(page)))
+		return false;
+
+	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;
+			/* 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 (is_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++;
diff --git a/mm/migrate.c b/mm/migrate.c
index be26d5c..59c7bc5 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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);
 	}
 }
 
@@ -783,6 +786,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		}
 	}
 
+	if (is_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
@@ -852,6 +866,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 (is_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) {
 		/*
-- 
1.7.10.4

[PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

[Rafael Aquini]

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 <aquini@redhat.com>
---
 drivers/virtio/virtio_balloon.c |  145 ++++++++++++++++++++++++++++++++++++---
 include/linux/virtio_balloon.h  |    4 ++
 2 files changed, 141 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..7669bc8 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -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,9 @@
  */
 #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
 
+/* Synchronize accesses/updates to the struct virtio_balloon elements */
+DEFINE_MUTEX(balloon_lock);
+
 struct virtio_balloon
 {
 	struct virtio_device *vdev;
@@ -51,6 +55,10 @@ struct virtio_balloon
 
 	/* Number of balloon pages we've told the Host we're not using. */
 	unsigned int num_pages;
+
+	/* Protect 'pages' list against concurrent handling */
+	spinlock_t pages_lock;
+
 	/*
 	 * 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,
@@ -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(&vb->pages_lock);
 		list_add(&page->lru, &vb->pages);
+		page->mapping = balloon_mapping;
+		spin_unlock(&vb->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) {
+		/*
+		 * We can race against virtballoon_isolatepage() and end up
+		 * stumbling across a _temporarily_ empty 'pages' list.
+		 */
+		spin_lock(&vb->pages_lock);
+		if (unlikely(list_empty(&vb->pages))) {
+			spin_unlock(&vb->pages_lock);
+			break;
+		}
 		page = list_first_entry(&vb->pages, struct page, lru);
+		page->mapping = NULL;
 		list_del(&page->lru);
+		spin_unlock(&vb->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)
@@ -339,6 +375,82 @@ static int init_vqs(struct virtio_balloon *vb)
 	return 0;
 }
 
+/*
+ * 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)
+{
+	struct virtio_balloon *vb = (void *)mapping->backing_dev_info;
+
+	mutex_lock(&balloon_lock);
+
+	/* balloon's page migration 1st step */
+	vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+	spin_lock(&vb->pages_lock);
+	list_add(&newpage->lru, &vb->pages);
+	spin_unlock(&vb->pages_lock);
+	set_page_pfns(vb->pfns, newpage);
+	tell_host(vb, vb->inflate_vq);
+
+	/* balloon's page migration 2nd step */
+	vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+	set_page_pfns(vb->pfns, page);
+	tell_host(vb, vb->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)
+{
+	/*
+	 * struct address_space.backing_dev_info specially holds the pointer
+	 * to our struct virtio_balloon *vb
+	 */
+	struct virtio_balloon *vb = (void *)page->mapping->backing_dev_info;
+	spin_lock(&vb->pages_lock);
+	list_del(&page->lru);
+	spin_unlock(&vb->pages_lock);
+}
+
+/*
+ * 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)
+{
+	/*
+	 * struct address_space.backing_dev_info specially holds the pointer
+	 * to our struct virtio_balloon *vb
+	 */
+	struct virtio_balloon *vb = (void *)page->mapping->backing_dev_info;
+	spin_lock(&vb->pages_lock);
+	list_add(&page->lru, &vb->pages);
+	spin_unlock(&vb->pages_lock);
+}
+
+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,12 +463,27 @@ static int virtballoon_probe(struct virtio_device *vdev)
 	}
 
 	INIT_LIST_HEAD(&vb->pages);
+	spin_lock_init(&vb->pages_lock);
+
 	vb->num_pages = 0;
 	init_waitqueue_head(&vb->config_change);
 	init_waitqueue_head(&vb->acked);
 	vb->vdev = vdev;
 	vb->need_stats_update = 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;
+	}
+
+	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;
+	balloon_mapping->backing_dev_info = (void *)vb;
+
 	err = init_vqs(vb);
 	if (err)
 		goto out_free_vb;
@@ -373,6 +500,7 @@ out_del_vqs:
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	kfree(vb);
+	kfree(balloon_mapping);
 out:
 	return err;
 }
@@ -397,6 +525,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kthread_stop(vb->thread);
 	remove_common(vb);
 	kfree(vb);
+	kfree(balloon_mapping);
 }
 
 #ifdef CONFIG_PM
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
index 652dc8b..930f1b7 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -56,4 +56,8 @@ struct virtio_balloon_stat {
 	u64 val;
 } __attribute__((packed));
 
+#if !defined(CONFIG_COMPACTION)
+struct address_space *balloon_mapping;
+#endif
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
-- 
1.7.10.4

[PATCH v4 3/3] mm: add vm event counters for balloon pages compaction

[Rafael Aquini]

This patch is only for testing report purposes and shall be dropped in case of
the rest of this patchset getting accepted for merging.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 drivers/virtio/virtio_balloon.c |    1 +
 include/linux/vm_event_item.h   |    2 ++
 mm/compaction.c                 |    1 +
 mm/migrate.c                    |    6 ++++--
 mm/vmstat.c                     |    4 ++++
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7669bc8..c4ddcd7 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -410,6 +410,7 @@ int virtballoon_migratepage(struct address_space *mapping,
 
 	mutex_unlock(&balloon_lock);
 
+	count_vm_event(COMPACTBALLOONMIGRATED);
 	return 0;
 }
 
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 06f8e38..e330c5a 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -40,6 +40,8 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 #ifdef CONFIG_COMPACTION
 		COMPACTBLOCKS, COMPACTPAGES, COMPACTPAGEFAILED,
 		COMPACTSTALL, COMPACTFAIL, COMPACTSUCCESS,
+		COMPACTBALLOONMIGRATED, COMPACTBALLOONFAILED,
+		COMPACTBALLOONISOLATED, COMPACTBALLOONFREED,
 #endif
 #ifdef CONFIG_HUGETLB_PAGE
 		HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
diff --git a/mm/compaction.c b/mm/compaction.c
index 51eac0c..d212dde 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -72,6 +72,7 @@ static bool isolate_balloon_page(struct page *page)
 			if (is_balloon_page(page) && (page_count(page) == 2)) {
 				__isolate_balloon_page(page);
 				unlock_page(page);
+				count_vm_event(COMPACTBALLOONISOLATED);
 				return true;
 			}
 			unlock_page(page);
diff --git a/mm/migrate.c b/mm/migrate.c
index 59c7bc5..5838719 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -78,9 +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));
-		if (unlikely(is_balloon_page(page)))
+		if (unlikely(is_balloon_page(page))) {
+			count_vm_event(COMPACTBALLOONFAILED);
 			WARN_ON(!putback_balloon_page(page));
-		else
+		} else
 			putback_lru_page(page);
 	}
 }
@@ -878,6 +879,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 				    page_is_file_cache(page));
 		put_page(page);
 		__free_page(page);
+		count_vm_event(COMPACTBALLOONFREED);
 		return rc;
 	}
 out:
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 1bbbbd9..3b7109f 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -767,6 +767,10 @@ const char * const vmstat_text[] = {
 	"compact_stall",
 	"compact_fail",
 	"compact_success",
+	"compact_balloon_migrated",
+	"compact_balloon_failed",
+	"compact_balloon_isolated",
+	"compact_balloon_freed",
 #endif
 
 #ifdef CONFIG_HUGETLB_PAGE
-- 
1.7.10.4

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Minchan Kim]

Hi Rafael,

On Tue, Jul 17, 2012 at 01:50:41PM -0300, Rafael Aquini 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.
> 
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
>  include/linux/mm.h |   15 +++++++
>  mm/compaction.c    |  126 ++++++++++++++++++++++++++++++++++++++++++++--------
>  mm/migrate.c       |   30 ++++++++++++-
>  3 files changed, 151 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b36d08c..3112198 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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 */
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 2f42d95..51eac0c 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -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);
> +}
> +
> +static inline void __putback_balloon_page(struct page *page)
> +{
> +	page->mapping->a_ops->freepage(page);
> +}
> +
> +/* __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.

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

> +			/*
> +			 * 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 */
> +		put_page(page);
> +	}
> +	return false;
> +}
> +
> +/* 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.

> +
> +	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;
> +			/* 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 (is_balloon_page(page)) {

unlikely?

> +			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++;
> diff --git a/mm/migrate.c b/mm/migrate.c
> index be26d5c..59c7bc5 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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?

diff --git a/mm/compaction.c b/mm/compaction.c
index aad0a16..b07cd67 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -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;
diff --git a/mm/migrate.c b/mm/migrate.c
index 9705e70..a96b840 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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
  */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 32985dd..decb82a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
                                    0, false, MIGRATE_SYNC);
        }
 
-       putback_lru_pages(&cc.migratepages);
+       putback_pages(&cc.migratepages);
        return ret > 0 ? 0 : ret;
 }

>  	}
>  }
>  
> @@ -783,6 +786,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>  		}
>  	}
>  
> +	if (is_balloon_page(page)) {

unlikely?

> +		/*
> +		 * 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
> @@ -852,6 +866,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 (is_balloon_page(newpage)) {

unlikely?

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

Why do you use __free_page instead of put_page?

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

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?

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

-- 
Kind regards,
Minchan Kim

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Andrew Morton]

On Tue, 17 Jul 2012 13:50:41 -0300
Rafael Aquini <aquini@redhat.com> 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.
> 
> ...
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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;

You can simply do

	return page->mapping == balloon_mapping;

> +}
> +#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.

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

Howdy Andrew,

Thanks for taking the time to go through this work and provide me with such good
feedback.

On Wed, Jul 18, 2012 at 03:46:05PM -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2012 13:50:41 -0300
> Rafael Aquini <aquini@redhat.com> 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.
> > 
> > ...
> >
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -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;
> 
> You can simply do
> 
> 	return page->mapping == balloon_mapping;

Yes, I will do 
   return (page->mapping && page->mapping == balloon_mapping);

actually. I just got a case of NULL pointer deref while running on bare-metal
with no balloon driver loaded.

> 
> > +}
> > +#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


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

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Andrew Morton]

On Wed, 18 Jul 2012 20:07:07 -0300
Rafael Aquini <aquini@redhat.com> wrote:

> > 
> > > +}
> > > +#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

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.

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

On Wed, Jul 18, 2012 at 04:12:39PM -0700, Andrew Morton wrote:
> On Wed, 18 Jul 2012 20:07:07 -0300
> Rafael Aquini <aquini@redhat.com> wrote:
> 
> > > 
> > > > +}
> > > > +#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
> 
> 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.
>
You're right, I got your point. 
 
> 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:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b94f17a..02a8f80 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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 */
diff --git a/mm/migrate.c b/mm/migrate.c
index 59c7bc5..f5f6a7d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -78,7 +78,8 @@ 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));
-		if (unlikely(is_balloon_page(page)))
+		if (unlikely(is_balloon_page(page)) &&
+		    balloon_compaction_enabled())
 			WARN_ON(!putback_balloon_page(page));
 		else
 			putback_lru_page(page);
@@ -786,7 +787,7 @@ static int __unmap_and_move(struct page *page, struct page
*newpage,
 		}
 	}
 
-	if (is_balloon_page(page)) {
+	if (is_balloon_page(page) && balloon_compaction_enabled()) {
 		/*
 		 * A ballooned page does not need any special attention from
 		 * physical to virtual reverse mapping procedures.
@@ -867,7 +868,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned
long private,
 
 	rc = __unmap_and_move(page, newpage, force, offlining, mode);
 
-	if (is_balloon_page(newpage)) {
+	if (is_balloon_page(newpage) && balloon_compaction_enabled()) {
 		/*
 		 * A ballooned page has been migrated already. Now, it is the
 		 * time to wrap-up counters, handle the old page back to Buddy

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Andrew Morton]

On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote:

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

it fails checkpatch ;)

> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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 */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 59c7bc5..f5f6a7d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -78,7 +78,8 @@ 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));
> -		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()?

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

On Wed, Jul 18, 2012 at 06:29:44PM -0700, Andrew Morton wrote:
> On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote:
> 
> > > 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:
> 
> it fails checkpatch ;)
>
Ugh! it fails due to a lacking whitespace... will fix that right away.
 
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -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 */
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 59c7bc5..f5f6a7d 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -78,7 +78,8 @@ 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));
> > -		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.
It will (now) return the right thing accordingly to the page->mapping tests.

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

With all due respect, sir, I don't believe renaming it is the right thing to do.
My major supporting reason is since Lumpy Reclaim is already evicted it looks
natural CONFIG_COMPACTION=y becoming a permanent feature, thus making that
preprocessor test useless and the renamed function signature nonsense, IMHO.
That's why I keep respectfully figthing against your argument.

Here goes another suggestion, to keep is_balloon_page() name as is. This way I
believe all concerns are potentially addressed, as there's no implicit and
misleading relationship between is_balloon_page and CONFIG_COMPACTION=y anymore,
as well as there are no potential build breakages due to (unexpected) config options.


diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..e29ad44 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1629,5 +1629,30 @@ 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))
+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 && page->mapping == balloon_mapping);
+}
+
+static inline bool balloon_compaction_enabled(void)
+{
+#if defined(CONFIG_COMPACTION)
+	return true;
+#else
+	return false;
+#endif /* CONFIG_COMPACTION */
+}
+
+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool is_balloon_page(struct page *page)      { return false; }
+static inline bool balloon_compaction_enabled(void)        { return false; }
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

Howdy Minchan,

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

On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > +/* __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.


> > +
> > +	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))) {
 		/*
-		 * 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.
 		 */
 		if (likely(trylock_page(page))) {
 			/*


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

 
> > +		} 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;
   "

> 
> > @@ -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?
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index aad0a16..b07cd67 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -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;
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 9705e70..a96b840 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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
>   */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 32985dd..decb82a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
>                                     0, false, MIGRATE_SYNC);
>         }
>  
> -       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).


> >  
> > +	if (is_balloon_page(page)) {
> 
> unlikely?
>

Yeah, why not? Will do it.
 
> > +	if (is_balloon_page(newpage)) {
> 
> unlikely?
> 

ditto.

> > +		/*
> > +		 * 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);
> 
> 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?


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

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;


It took me a little longer to address all your good questions because I
collected some data on this series impact for the ordinary average use case
(bare-metal boxes with no balloon at all). Here are some very simple numbers,
for the sake of comparison:

* Benchmark: "echo 1 > /proc/sys/vm/compact_memory" after a fresh reboot.
* Results represent the average of 4 sampling rounds for each test subject.

=== 3.5.0-rc7 (clean) ===
Measured overhead (perf record):
Events: 304  cycles                                                             

  4.44%  test.sh  [kernel.kallsyms]  [k] isolate_migratepages_range
  1.69%  test.sh  [kernel.kallsyms]  [k] migrate_pages


 Performance counter stats for './test.sh':

        314.794120 task-clock                #    0.913 CPUs utilized          
                34 context-switches          #    0.110 K/sec                  
                 1 CPU-migrations            #    0.004 K/sec                  
               354 page-faults               #    0.001 M/sec                  
       678,772,513 cycles                    #    2.173 GHz		[49.92%]
   <not supported> stalled-cycles-frontend 
   <not supported> stalled-cycles-backend  
       402,180,271 instructions              #    0.59  insns per cycle	[74.85%]
        72,746,956 branches                  #  231.328 M/sec		[75.05%]
           455,274 branch-misses             #    0.62% of all branches	[75.13%]

       0.344474338 seconds time elapsed

Compaction/Migration observed throughput:
-----------------------------------------
compact_blocks_moved 2988
compact_pages_moved 42219
compact_pagemigrate_failed 5996
compact_stall 0
compact_fail 0
compact_success 0
========================================

=== 3.5.0-rc7 (patch) ===
Measured overhead (perf record):
Events: 336  cycles                                                             

  4.73%  test.sh  [kernel.kallsyms]  [k] isolate_migratepages_range
  1.77%  test.sh  [kernel.kallsyms]  [k] migrate_pages
 
 Performance counter stats for './test.sh':

        369.363248 task-clock                #    0.913 CPUs utilized          
                40 context-switches          #    0.110 K/sec                  
                 1 CPU-migrations            #    0.004 K/sec                  
               354 page-faults               #    0.001 K/sec                  
       790,381,297 cycles                    #    2.173 GHz		[49.94%]
   <not supported> stalled-cycles-frontend 
   <not supported> stalled-cycles-backend  
       451,900,192 instructions              #    0.59  insns per cycle	[74.95%]
        81,679,252 branches                  #  222.352 M/sec		[74.99%]
           531,265 branch-misses             #    0.65% of all branches	[75.12%]

       0.404462151 seconds time elapsed

Compaction/Migration observed throughput:
-----------------------------------------
compact_blocks_moved 3287
compact_pages_moved 51794
compact_pagemigrate_failed 5971
compact_stall 0
compact_fail 0
compact_success 0
========================================


Despite the patched case apparently showing some low overhead, the numbers, in
proportion, are virtually the same, as one may notice.
In fact, I believe that applying your suggestions on better hinting the compiler
about the branch predictability can potentially turn this change into a win-win
deal.

Best regards!
-- Rafael

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Minchan Kim]

Hi Rafael,

On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> Howdy Minchan,
> 
> Once again, thanks for raising such valuable feedback over here.
> 
> On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > +/* __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.

Yes. It's not static.

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

>  		 */
>  		if (likely(trylock_page(page))) {
>  			/*
> 
> 
> > > +/* 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.

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.

> 
>  
> > > +		} 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;
>    "

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

> 
> > 
> > > @@ -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?
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index aad0a16..b07cd67 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -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;
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 9705e70..a96b840 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -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
> >   */
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 32985dd..decb82a 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> >                                     0, false, MIGRATE_SYNC);
> >         }
> >  
> > -       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.

> 
> 
> > >  
> > > +	if (is_balloon_page(page)) {
> > 
> > unlikely?
> >
> 
> Yeah, why not? Will do it.
>  
> > > +	if (is_balloon_page(newpage)) {
> > 
> > unlikely?
> > 
> 
> ditto.
> 
> > > +		/*
> > > +		 * 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);
> > 
> > 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?

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

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

-- 
Kind regards,
Minchan Kim

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Minchan Kim]

On Mon, Jul 23, 2012 at 11:33:32AM +0900, Minchan Kim wrote:
> Hi Rafael,
> 
> On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> > Howdy Minchan,
> > 
> > Once again, thanks for raising such valuable feedback over here.
> > 
> > On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > > +/* __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.
> 
> Yes. It's not static.

Typo. It's static.

-- 
Kind regards,
Minchan Kim

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

On Mon, Jul 23, 2012 at 11:33:32AM +0900, Minchan Kim wrote:
> Hi Rafael,
> 
> On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> > Howdy Minchan,
> > 
> > Once again, thanks for raising such valuable feedback over here.
> > 
> > On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > > +/* __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.
> 
> Yes. It's not static.
> 
> > 
> > 
> > > > +
> > > > +	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))) {
> >  		/*
> > -		 * 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.
> 
> >  		 */
> >  		if (likely(trylock_page(page))) {
> >  			/*
> > 
> > 
> > > > +/* 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.
> 
> 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.
> 
> > 
> >  
> > > > +		} 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;
> >    "
> 
> I don't like that.
> PageLRU isn't likelihood case in this.
> 
> > 
> > > 
> > > > @@ -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?
> > > 
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index aad0a16..b07cd67 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -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;
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 9705e70..a96b840 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -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
> > >   */
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 32985dd..decb82a 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> > >                                     0, false, MIGRATE_SYNC);
> > >         }
> > >  
> > > -       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.
> 
> > 
> > 
> > > >  
> > > > +	if (is_balloon_page(page)) {
> > > 
> > > unlikely?
> > >
> > 
> > Yeah, why not? Will do it.
> >  
> > > > +	if (is_balloon_page(newpage)) {
> > > 
> > > unlikely?
> > > 
> > 
> > ditto.
> > 
> > > > +		/*
> > > > +		 * 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);
> > > 
> > > 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?
> 
> 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.
>  */
> 

No, it's ugly and potentially buggy (for this case). Besides, it shows no gain
at all, as ballooned pages are a corner case here and they're not on LRUs. So,
being quite reasonable, how this suggestion of yours could possibly be
acceptable here?
The comment is also not necessary, as there already is a comment on this, properly
placed, which was (apparently) ignored by your eyes.


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

No, it is simply teaching compaction and migration about another possible and
plausible page type case. There is also no loss, the numbers I provided you
support that. The apparent overhead observed on the patched case is, actually,
due to the bigger throughput observed. 
In fact, the changes introduced here seem to have (accidentally) made that code
more effective. Checking the numbers, again:

- "compact_pages_moved" average throughput increase:
$ echo "scale=3; (1-(42219/51794))*100" | bc
18.500

- Avg time elapsed (sec) / page moved:
* 3.5.0-rc7 (clean)
 $ echo "scale=10; 0.344474338/42219" | bc
 .0000081592

* 3.5.0-rc7 (patch) 
 $ echo "scale=10; 0.404462151/51794" | bc
 .0000078090



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

You're simply ignoring that this series makes ballooned pages movable, so
there's no need for this special treatment you're quoting at
isolate_freepages_*() level. Ballooned pages don't need to be reclaimed, they
just need to be moved away to not cause too much fragmentation.

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

A-ha! Now you gave me the real reason why you're struggling against this
proposed implementation. I'll take a look at those bits, to come up with
something suitable, for sure.

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

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.

Thanks for your (always) valuable feedback.

Best regards,

Rafael


> -- 
> Kind regards,
> Minchan Kim

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rik van Riel]

On 07/23/2012 02:19 PM, Rafael Aquini wrote:

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

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

On Wed, Aug 01, 2012 at 04:53:47PM -0400, Rik van Riel wrote:
> On 07/23/2012 02:19 PM, Rafael Aquini wrote:
> 
> >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.

I'm 100% in agreement with your thoughts here.

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rik van Riel]

On 07/22/2012 10:33 PM, Minchan Kim wrote:

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

I believe this is the wrong thing to do.

In a KVM guest, getting applications in transparent
huge pages can be a 10-25% performance benefit.

Therefore, we need to make all the 2MB pageblocks
we can available for use by userland.

Using 2MB blocks for the balloon (which is never
touched) is extremely wasteful and could result in
a large performance penalty, if we cannot defragment
the remaining memory enough to give 2MB pages to
applications.

The 2MB blocks are prime real estate. They should
remain available for applications.

Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages

[Rafael Aquini]

On Mon, Jul 23, 2012 at 11:33:32AM +0900, Minchan Kim wrote:
> 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.
>
As promised, I looked into those bits. Yes, they only isolate LRU pages, and as
such, having this series merged or not doesn't change a bit for that code path.
In fact, having this series merged and teaching hotplug's
offline_pages()/do_migrate_rage() about ballooned pages might be extremely
beneficial in the rare event offlining memory stumbles across a balloon page.

As Rik said, I believe this is something we can look into in the near future.
 
> 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.
> 
I totally agree with Rik on this one, as well. This is the wrong approach here.

All that said, I'll soon respin a v5 based on your comments on branch hinting and
commentary improvements, as well as addressing AKPM's concerns. I'll also revert
isolate_balloon_page() last changes back to make it a public symbol again, as
(I believe) we'll shortly be using it for letting hotplug bits aware of how to
isolate ballooned pages.

Re: [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

[Andrew Morton]

On Tue, 17 Jul 2012 13:50:42 -0300
Rafael Aquini <aquini@redhat.com> 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;
> 
> ...
>
> +	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.

Re: [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

[Rafael Aquini]

Howdy Andrew,

Thanks for taking the time to go through this work and provide me with such good
feedback.

On Wed, Jul 18, 2012 at 03:49:08PM -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2012 13:50:42 -0300
> Rafael Aquini <aquini@redhat.com> 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;
> > 
> > ...
> >
> > +	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.

Shame on me, on this one.

Mea culpa: I took this approach, originally, because I stuck the spinlock within
the struct virtio_balloon and this was the easiest way to recover it just by
having the page pointer. I did this stupidity because on earlier stages of this
patch some functions that demanded access to that list spinlock were placed
outside the balloon driver's code -- this is a left-over
(I know, it's a total lame excuse, but it's the truth)

This is easily fixable, however, as the balloon page list spinlock is now only
being accessed within driver's code and it can be declared outside the struct.