This patchset follows the main idea discussed at 2012 LSFMMS section:
"Ballooning for transparent huge pages" -- http://lwn.net/Articles/490114/
to introduce the required changes to the virtio_balloon driver, as well as
changes to the core compaction & migration bits, in order to allow
memory balloon pages become movable within a guest.
Rafael Aquini (4):
mm: introduce compaction and migration for virtio ballooned pages
virtio_balloon: handle concurrent accesses to virtio_balloon struct
elements
virtio_balloon: introduce migration primitives to balloon pages
mm: add vm event counters for balloon pages compaction
drivers/virtio/virtio_balloon.c | 142 +++++++++++++++++++++++++++++++++++----
include/linux/mm.h | 17 +++++
include/linux/virtio_balloon.h | 6 ++
include/linux/vm_event_item.h | 2 +
mm/compaction.c | 74 ++++++++++++++++++++
mm/migrate.c | 32 ++++++++-
mm/vmstat.c | 4 ++
7 files changed, 263 insertions(+), 14 deletions(-)
Preliminary test results:
(2 VCPU 1024mB RAM KVM guest running 3.5.0_rc4+)
* 64mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 0
compact_balloon_failed 0
compact_balloon_isolated 0
compact_balloon_freed 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 4); do echo 1> /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo > /proc/sys/vm/compact_memory
[2] Done echo > /proc/sys/vm/compact_memory
[3]- Done echo > /proc/sys/vm/compact_memory
[4]+ Done echo > /proc/sys/vm/compact_memory
[root@localhost ~]#
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 2683
compact_pages_moved 47502
compact_pagemigrate_failed 61
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 16384
compact_balloon_failed 0
compact_balloon_isolated 16384
compact_balloon_freed 16384
* 128mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 0
compact_balloon_failed 0
compact_balloon_isolated 0
compact_balloon_freed 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 4); do echo 1> /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo > /proc/sys/vm/compact_memory
[2] Done echo > /proc/sys/vm/compact_memory
[3]- Done echo > /proc/sys/vm/compact_memory
[4]+ Done echo > /proc/sys/vm/compact_memory
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 2624
compact_pages_moved 49195
compact_pagemigrate_failed 54
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 29350
compact_balloon_failed 29
compact_balloon_isolated 29379
compact_balloon_freed 29350
--
1.7.10.2
This patch makes balloon pages movable at allocation time and introduces the
infrastructure needed to perform the balloon page migration operation.
Signed-off-by: Rafael Aquini <[email protected]>
---
drivers/virtio/virtio_balloon.c | 96 ++++++++++++++++++++++++++++++++++++++-
include/linux/virtio_balloon.h | 6 +++
2 files changed, 100 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index d47c5c2..53386aa 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -27,6 +27,8 @@
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/pagemap.h>
/*
* Balloon device works in 4K page units. So each page is pointed to by
@@ -140,8 +142,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
spin_lock(&vb->pfn_list_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,
@@ -154,6 +157,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
totalram_pages--;
list_add(&page->lru, &vb->pages);
+ page->mapping = balloon_mapping;
}
/* Didn't get any? Oh well. */
@@ -195,6 +199,7 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
page = list_first_entry(&vb->pages, struct page, lru);
+ page->mapping = NULL;
list_del(&page->lru);
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
@@ -365,6 +370,77 @@ 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;
+ struct scatterlist sg;
+
+ /* balloon's page migration 1st step */
+ spin_lock(&vb->pfn_list_lock);
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ list_add(&newpage->lru, &vb->pages);
+ set_page_pfns(vb->pfns, newpage);
+ sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+ spin_unlock(&vb->pfn_list_lock);
+ tell_host(vb, vb->inflate_vq, &sg);
+
+ /* balloon's page migration 2nd step */
+ spin_lock(&vb->pfn_list_lock);
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ set_page_pfns(vb->pfns, page);
+ sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+ spin_unlock(&vb->pfn_list_lock);
+ tell_host(vb, vb->deflate_vq, &sg);
+
+ 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 *mapping = page->mapping;
+ struct virtio_balloon *vb = (void *)mapping->backing_dev_info;
+ spin_lock(&vb->pfn_list_lock);
+ list_del(&page->lru);
+ spin_unlock(&vb->pfn_list_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 *mapping = page->mapping;
+ struct virtio_balloon *vb = (void *)mapping->backing_dev_info;
+ spin_lock(&vb->pfn_list_lock);
+ list_add(&page->lru, &vb->pages);
+ spin_unlock(&vb->pfn_list_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;
@@ -384,6 +460,19 @@ static int virtballoon_probe(struct virtio_device *vdev)
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_mapping;
+ }
+
+ 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;
@@ -398,6 +487,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
out_del_vqs:
vdev->config->del_vqs(vdev);
+out_free_mapping:
+ kfree(balloon_mapping);
out_free_vb:
kfree(vb);
out:
@@ -424,6 +515,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..db21300 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -56,4 +56,10 @@ struct virtio_balloon_stat {
u64 val;
} __attribute__((packed));
+#if defined(CONFIG_COMPACTION)
+extern struct address_space *balloon_mapping;
+#else
+struct address_space *balloon_mapping;
+#endif
+
#endif /* _LINUX_VIRTIO_BALLOON_H */
--
1.7.10.2
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 <[email protected]>
---
drivers/virtio/virtio_balloon.c | 1 +
include/linux/vm_event_item.h | 2 ++
mm/compaction.c | 4 +++-
mm/migrate.c | 6 ++++--
mm/vmstat.c | 4 ++++
5 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 53386aa..c4a929d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -406,6 +406,7 @@ int virtballoon_migratepage(struct address_space *mapping,
spin_unlock(&vb->pfn_list_lock);
tell_host(vb, vb->deflate_vq, &sg);
+ 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 8835d55..cf250b8 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -318,8 +318,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
* for PageLRU, as well as skip the LRU page isolation steps.
*/
if (PageBalloon(page))
- if (isolate_balloon_page(page))
+ if (isolate_balloon_page(page)) {
+ count_vm_event(COMPACTBALLOONISOLATED);
goto isolated_balloon_page;
+ }
if (!PageLRU(page))
continue;
diff --git a/mm/migrate.c b/mm/migrate.c
index ffc02a4..3dbca33 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(PageBalloon(page)))
+ if (unlikely(PageBalloon(page))) {
VM_BUG_ON(!putback_balloon_page(page));
- else
+ count_vm_event(COMPACTBALLOONFAILED);
+ } 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.2
This patch introduces helper functions that teach compaction and migration bits
how to cope with pages which are part of a guest memory balloon, in order to
make them movable by memory compaction procedures.
Signed-off-by: Rafael Aquini <[email protected]>
---
include/linux/mm.h | 17 +++++++++++++
mm/compaction.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
mm/migrate.c | 30 +++++++++++++++++++++-
3 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..360656e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1629,5 +1629,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
static inline bool page_is_guard(struct page *page) { return false; }
#endif /* CONFIG_DEBUG_PAGEALLOC */
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+ defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern int is_balloon_page(struct page *);
+extern int isolate_balloon_page(struct page *);
+extern int putback_balloon_page(struct page *);
+
+/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
+static inline int PageBalloon(struct page *page)
+{
+ return is_balloon_page(page);
+}
+#else
+static inline int PageBalloon(struct page *page) { return 0; }
+static inline int isolate_balloon_page(struct page *page) { return 0; }
+static inline int putback_balloon_page(struct page *page) { return 0; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+
#endif /* __KERNEL__ */
#endif /* _LINUX_MM_H */
diff --git a/mm/compaction.c b/mm/compaction.c
index 7ea259d..8835d55 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
@@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
continue;
}
+ /*
+ * For ballooned pages, we need to isolate them before testing
+ * for PageLRU, as well as skip the LRU page isolation steps.
+ */
+ if (PageBalloon(page))
+ if (isolate_balloon_page(page))
+ goto isolated_balloon_page;
+
if (!PageLRU(page))
continue;
@@ -338,6 +347,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
/* Successfully isolated */
del_page_from_lru_list(page, lruvec, page_lru(page));
+isolated_balloon_page:
list_add(&page->lru, migratelist);
cc->nr_migratepages++;
nr_isolated++;
@@ -903,4 +913,66 @@ void compaction_unregister_node(struct node *node)
}
#endif /* CONFIG_SYSFS && CONFIG_NUMA */
+#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
+/*
+ * Balloon pages special page->mapping.
+ * users must properly allocate and initiliaze an instance of balloon_mapping,
+ * and set it as the page->mapping for balloon enlisted page instances.
+ *
+ * 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(balloon_mapping);
+
+/* ballooned page id check */
+int is_balloon_page(struct page *page)
+{
+ struct address_space *mapping = page->mapping;
+ if (mapping == balloon_mapping)
+ return 1;
+ return 0;
+}
+
+/* __isolate_lru_page() counterpart for a ballooned page */
+int isolate_balloon_page(struct page *page)
+{
+ struct address_space *mapping = page->mapping;
+ if (mapping->a_ops->invalidatepage) {
+ /*
+ * We can race against move_to_new_page() and stumble across a
+ * locked 'newpage'. If we succeed on isolating it, the result
+ * tends to be disastrous. So, we sanely skip PageLocked here.
+ */
+ if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
+ /*
+ * A ballooned page, by default, has just one refcount.
+ * Prevent concurrent compaction threads from isolating
+ * an already isolated balloon page.
+ */
+ if (page_count(page) == 2) {
+ mapping->a_ops->invalidatepage(page, 0);
+ return 1;
+ }
+ /* Drop refcount taken for this already isolated page */
+ put_page(page);
+ }
+ }
+ return 0;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+int putback_balloon_page(struct page *page)
+{
+ struct address_space *mapping = page->mapping;
+ if (mapping->a_ops->freepage) {
+ mapping->a_ops->freepage(page);
+ put_page(page);
+ return 1;
+ }
+ return 0;
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
#endif /* CONFIG_COMPACTION */
diff --git a/mm/migrate.c b/mm/migrate.c
index be26d5c..ffc02a4 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(PageBalloon(page)))
+ VM_BUG_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 (PageBalloon(page)) {
+ /*
+ * A ballooned page does not need any special attention from
+ * physical to virtual reverse mapping procedures.
+ * To avoid burning cycles at rmap level,
+ * skip attempts to unmap PTEs or remap swapcache.
+ */
+ 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 (PageBalloon(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.2
This patch introduces access sychronization to critical elements of struct
virtio_balloon, in order to allow the thread concurrency compaction/migration
bits might ended up imposing to the balloon driver on several situations.
Signed-off-by: Rafael Aquini <[email protected]>
---
drivers/virtio/virtio_balloon.c | 45 +++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index bfbc15c..d47c5c2 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -51,6 +51,10 @@ struct virtio_balloon
/* Number of balloon pages we've told the Host we're not using. */
unsigned int num_pages;
+
+ /* Protect 'pages', 'pfns' & 'num_pnfs' against concurrent updates */
+ spinlock_t pfn_list_lock;
+
/*
* The pages we've told the Host we're not using.
* Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
@@ -97,21 +101,23 @@ static void balloon_ack(struct virtqueue *vq)
complete(&vb->acked);
}
-static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
-{
- struct scatterlist sg;
-
- sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+/* Protection for concurrent accesses to balloon virtqueues and vb->acked */
+DEFINE_MUTEX(vb_queue_completion);
+static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
+ struct scatterlist *sg)
+{
+ mutex_lock(&vb_queue_completion);
init_completion(&vb->acked);
/* We should always be able to add one buffer to an empty queue. */
- if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
+ if (virtqueue_add_buf(vq, sg, 1, 0, vb, GFP_KERNEL) < 0)
BUG();
virtqueue_kick(vq);
/* When host has read buffer, this completes via balloon_ack */
wait_for_completion(&vb->acked);
+ mutex_unlock(&vb_queue_completion);
}
static void set_page_pfns(u32 pfns[], struct page *page)
@@ -126,9 +132,12 @@ static void set_page_pfns(u32 pfns[], struct page *page)
static void fill_balloon(struct virtio_balloon *vb, size_t num)
{
+ struct scatterlist sg;
+ int alloc_failed = 0;
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ spin_lock(&vb->pfn_list_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 |
@@ -138,8 +147,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
dev_printk(KERN_INFO, &vb->vdev->dev,
"Out of puff! Can't get %zu pages\n",
num);
- /* Sleep for at least 1/5 of a second before retry. */
- msleep(200);
+ alloc_failed = 1;
break;
}
set_page_pfns(vb->pfns + vb->num_pfns, page);
@@ -149,10 +157,19 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
}
/* Didn't get any? Oh well. */
- if (vb->num_pfns == 0)
+ if (vb->num_pfns == 0) {
+ spin_unlock(&vb->pfn_list_lock);
return;
+ }
+
+ sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+ spin_unlock(&vb->pfn_list_lock);
- tell_host(vb, vb->inflate_vq);
+ /* alloc_page failed, sleep for at least 1/5 of a sec before retry. */
+ if (alloc_failed)
+ msleep(200);
+
+ tell_host(vb, vb->inflate_vq, &sg);
}
static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
@@ -169,10 +186,12 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
static void leak_balloon(struct virtio_balloon *vb, size_t num)
{
struct page *page;
+ struct scatterlist sg;
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ spin_lock(&vb->pfn_list_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
page = list_first_entry(&vb->pages, struct page, lru);
@@ -180,13 +199,15 @@ static void leak_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;
}
+ sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+ spin_unlock(&vb->pfn_list_lock);
/*
* Note that if
* 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);
+ tell_host(vb, vb->deflate_vq, &sg);
release_pages_by_pfn(vb->pfns, vb->num_pfns);
}
@@ -356,6 +377,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
}
INIT_LIST_HEAD(&vb->pages);
+ spin_lock_init(&vb->pfn_list_lock);
+
vb->num_pages = 0;
init_waitqueue_head(&vb->config_change);
vb->vdev = vdev;
--
1.7.10.2
On Mon, Jun 25, 2012 at 7:25 PM, Rafael Aquini <[email protected]> wrote:
> This patch introduces helper functions that 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.
>
Should the names that are exported be prefixed with kvm_?
> Signed-off-by: Rafael Aquini <[email protected]>
> ---
> ?include/linux/mm.h | ? 17 +++++++++++++
> ?mm/compaction.c ? ?| ? 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> ?mm/migrate.c ? ? ? | ? 30 +++++++++++++++++++++-
> ?3 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b36d08c..360656e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1629,5 +1629,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
> ?static inline bool page_is_guard(struct page *page) { return false; }
> ?#endif /* CONFIG_DEBUG_PAGEALLOC */
>
> +#if (defined(CONFIG_VIRTIO_BALLOON) || \
> + ? ? ? defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> +extern int is_balloon_page(struct page *);
> +extern int isolate_balloon_page(struct page *);
> +extern int putback_balloon_page(struct page *);
> +
> +/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
> +static inline int PageBalloon(struct page *page)
> +{
> + ? ? ? return is_balloon_page(page);
> +}
> +#else
> +static inline int PageBalloon(struct page *page) ? ? ? ? ? ? ? { return 0; }
> +static inline int isolate_balloon_page(struct page *page) ? ? ?{ return 0; }
> +static inline int putback_balloon_page(struct page *page) ? ? ?{ return 0; }
> +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> +
> ?#endif /* __KERNEL__ */
> ?#endif /* _LINUX_MM_H */
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 7ea259d..8835d55 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
> @@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> ? ? ? ? ? ? ? ? ? ? ? ?continue;
> ? ? ? ? ? ? ? ?}
>
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* For ballooned pages, we need to isolate them before testing
> + ? ? ? ? ? ? ? ?* for PageLRU, as well as skip the LRU page isolation steps.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? if (PageBalloon(page))
> + ? ? ? ? ? ? ? ? ? ? ? if (isolate_balloon_page(page))
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto isolated_balloon_page;
> +
> ? ? ? ? ? ? ? ?if (!PageLRU(page))
> ? ? ? ? ? ? ? ? ? ? ? ?continue;
>
> @@ -338,6 +347,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>
> ? ? ? ? ? ? ? ?/* Successfully isolated */
> ? ? ? ? ? ? ? ?del_page_from_lru_list(page, lruvec, page_lru(page));
> +isolated_balloon_page:
> ? ? ? ? ? ? ? ?list_add(&page->lru, migratelist);
> ? ? ? ? ? ? ? ?cc->nr_migratepages++;
> ? ? ? ? ? ? ? ?nr_isolated++;
> @@ -903,4 +913,66 @@ void compaction_unregister_node(struct node *node)
> ?}
> ?#endif /* CONFIG_SYSFS && CONFIG_NUMA */
>
> +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
> +/*
> + * Balloon pages special page->mapping.
> + * users must properly allocate and initiliaze an instance of balloon_mapping,
> + * and set it as the page->mapping for balloon enlisted page instances.
> + *
> + * 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(balloon_mapping);
> +
> +/* ballooned page id check */
> +int is_balloon_page(struct page *page)
> +{
> + ? ? ? struct address_space *mapping = page->mapping;
> + ? ? ? if (mapping == balloon_mapping)
> + ? ? ? ? ? ? ? return 1;
> + ? ? ? return 0;
> +}
> +
> +/* __isolate_lru_page() counterpart for a ballooned page */
> +int isolate_balloon_page(struct page *page)
> +{
> + ? ? ? struct address_space *mapping = page->mapping;
> + ? ? ? if (mapping->a_ops->invalidatepage) {
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* We can race against move_to_new_page() and stumble across a
> + ? ? ? ? ? ? ? ?* locked 'newpage'. If we succeed on isolating it, the result
> + ? ? ? ? ? ? ? ?* tends to be disastrous. So, we sanely skip PageLocked here.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
> + ? ? ? ? ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ? ? ? ? ?* A ballooned page, by default, has just one refcount.
> + ? ? ? ? ? ? ? ? ? ? ? ?* Prevent concurrent compaction threads from isolating
> + ? ? ? ? ? ? ? ? ? ? ? ?* an already isolated balloon page.
> + ? ? ? ? ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? ? ? ? ? if (page_count(page) == 2) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mapping->a_ops->invalidatepage(page, 0);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? /* Drop refcount taken for this already isolated page */
> + ? ? ? ? ? ? ? ? ? ? ? put_page(page);
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> + ? ? ? return 0;
> +}
> +
> +/* putback_lru_page() counterpart for a ballooned page */
> +int putback_balloon_page(struct page *page)
> +{
> + ? ? ? struct address_space *mapping = page->mapping;
> + ? ? ? if (mapping->a_ops->freepage) {
> + ? ? ? ? ? ? ? mapping->a_ops->freepage(page);
> + ? ? ? ? ? ? ? put_page(page);
> + ? ? ? ? ? ? ? return 1;
> + ? ? ? }
> + ? ? ? return 0;
> +}
> +#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
> ?#endif /* CONFIG_COMPACTION */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index be26d5c..ffc02a4 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(PageBalloon(page)))
> + ? ? ? ? ? ? ? ? ? ? ? VM_BUG_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 (PageBalloon(page)) {
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* A ballooned page does not need any special attention from
> + ? ? ? ? ? ? ? ?* physical to virtual reverse mapping procedures.
> + ? ? ? ? ? ? ? ?* To avoid burning cycles at rmap level,
> + ? ? ? ? ? ? ? ?* skip attempts to unmap PTEs or remap swapcache.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? 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 (PageBalloon(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.2
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. ?For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
On Mon, Jun 25, 2012 at 07:31:38PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Jun 25, 2012 at 7:25 PM, Rafael Aquini <[email protected]> wrote:
> > This patch introduces helper functions that 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.
> >
>
> Should the names that are exported be prefixed with kvm_?
>
I rather let them as generic as possible, specially because I believe other
balloon drivers can leverage the same technique to make their page lists movable
as well, in the near future. However, I do agree with your tip if we ended up
finding out no one else can reuse this piece of code.
> > Signed-off-by: Rafael Aquini <[email protected]>
> > ---
> > ?include/linux/mm.h | ? 17 +++++++++++++
> > ?mm/compaction.c ? ?| ? 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > ?mm/migrate.c ? ? ? | ? 30 +++++++++++++++++++++-
> > ?3 files changed, 118 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index b36d08c..360656e 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1629,5 +1629,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
> > ?static inline bool page_is_guard(struct page *page) { return false; }
> > ?#endif /* CONFIG_DEBUG_PAGEALLOC */
> >
> > +#if (defined(CONFIG_VIRTIO_BALLOON) || \
> > + ? ? ? defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> > +extern int is_balloon_page(struct page *);
> > +extern int isolate_balloon_page(struct page *);
> > +extern int putback_balloon_page(struct page *);
> > +
> > +/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
> > +static inline int PageBalloon(struct page *page)
> > +{
> > + ? ? ? return is_balloon_page(page);
> > +}
> > +#else
> > +static inline int PageBalloon(struct page *page) ? ? ? ? ? ? ? { return 0; }
> > +static inline int isolate_balloon_page(struct page *page) ? ? ?{ return 0; }
> > +static inline int putback_balloon_page(struct page *page) ? ? ?{ return 0; }
> > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> > +
> > ?#endif /* __KERNEL__ */
> > ?#endif /* _LINUX_MM_H */
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 7ea259d..8835d55 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
> > @@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> > ? ? ? ? ? ? ? ? ? ? ? ?continue;
> > ? ? ? ? ? ? ? ?}
> >
> > + ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ?* For ballooned pages, we need to isolate them before testing
> > + ? ? ? ? ? ? ? ?* for PageLRU, as well as skip the LRU page isolation steps.
> > + ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? if (PageBalloon(page))
> > + ? ? ? ? ? ? ? ? ? ? ? if (isolate_balloon_page(page))
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto isolated_balloon_page;
> > +
> > ? ? ? ? ? ? ? ?if (!PageLRU(page))
> > ? ? ? ? ? ? ? ? ? ? ? ?continue;
> >
> > @@ -338,6 +347,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> >
> > ? ? ? ? ? ? ? ?/* Successfully isolated */
> > ? ? ? ? ? ? ? ?del_page_from_lru_list(page, lruvec, page_lru(page));
> > +isolated_balloon_page:
> > ? ? ? ? ? ? ? ?list_add(&page->lru, migratelist);
> > ? ? ? ? ? ? ? ?cc->nr_migratepages++;
> > ? ? ? ? ? ? ? ?nr_isolated++;
> > @@ -903,4 +913,66 @@ void compaction_unregister_node(struct node *node)
> > ?}
> > ?#endif /* CONFIG_SYSFS && CONFIG_NUMA */
> >
> > +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
> > +/*
> > + * Balloon pages special page->mapping.
> > + * users must properly allocate and initiliaze an instance of balloon_mapping,
> > + * and set it as the page->mapping for balloon enlisted page instances.
> > + *
> > + * 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(balloon_mapping);
> > +
> > +/* ballooned page id check */
> > +int is_balloon_page(struct page *page)
> > +{
> > + ? ? ? struct address_space *mapping = page->mapping;
> > + ? ? ? if (mapping == balloon_mapping)
> > + ? ? ? ? ? ? ? return 1;
> > + ? ? ? return 0;
> > +}
> > +
> > +/* __isolate_lru_page() counterpart for a ballooned page */
> > +int isolate_balloon_page(struct page *page)
> > +{
> > + ? ? ? struct address_space *mapping = page->mapping;
> > + ? ? ? if (mapping->a_ops->invalidatepage) {
> > + ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ?* We can race against move_to_new_page() and stumble across a
> > + ? ? ? ? ? ? ? ?* locked 'newpage'. If we succeed on isolating it, the result
> > + ? ? ? ? ? ? ? ?* tends to be disastrous. So, we sanely skip PageLocked here.
> > + ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
> > + ? ? ? ? ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ? ? ? ? ?* A ballooned page, by default, has just one refcount.
> > + ? ? ? ? ? ? ? ? ? ? ? ?* Prevent concurrent compaction threads from isolating
> > + ? ? ? ? ? ? ? ? ? ? ? ?* an already isolated balloon page.
> > + ? ? ? ? ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? ? ? ? ? if (page_count(page) == 2) {
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mapping->a_ops->invalidatepage(page, 0);
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
> > + ? ? ? ? ? ? ? ? ? ? ? }
> > + ? ? ? ? ? ? ? ? ? ? ? /* Drop refcount taken for this already isolated page */
> > + ? ? ? ? ? ? ? ? ? ? ? put_page(page);
> > + ? ? ? ? ? ? ? }
> > + ? ? ? }
> > + ? ? ? return 0;
> > +}
> > +
> > +/* putback_lru_page() counterpart for a ballooned page */
> > +int putback_balloon_page(struct page *page)
> > +{
> > + ? ? ? struct address_space *mapping = page->mapping;
> > + ? ? ? if (mapping->a_ops->freepage) {
> > + ? ? ? ? ? ? ? mapping->a_ops->freepage(page);
> > + ? ? ? ? ? ? ? put_page(page);
> > + ? ? ? ? ? ? ? return 1;
> > + ? ? ? }
> > + ? ? ? return 0;
> > +}
> > +#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
> > ?#endif /* CONFIG_COMPACTION */
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index be26d5c..ffc02a4 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(PageBalloon(page)))
> > + ? ? ? ? ? ? ? ? ? ? ? VM_BUG_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 (PageBalloon(page)) {
> > + ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ?* A ballooned page does not need any special attention from
> > + ? ? ? ? ? ? ? ?* physical to virtual reverse mapping procedures.
> > + ? ? ? ? ? ? ? ?* To avoid burning cycles at rmap level,
> > + ? ? ? ? ? ? ? ?* skip attempts to unmap PTEs or remap swapcache.
> > + ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? 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 (PageBalloon(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.2
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to [email protected]. ?For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
> >
(apologies if there are excessive typos. I damaged my left hand and
typing is painful).
Adding Andi to cc for question on VM_BUG_ON.
On Mon, Jun 25, 2012 at 08:25:56PM -0300, Rafael Aquini wrote:
> This patch introduces helper functions that teach compaction and migration bits
> how to cope with pages which are part of a guest memory balloon, in order to
> make them movable by memory compaction procedures.
>
> Signed-off-by: Rafael Aquini <[email protected]>
> ---
> include/linux/mm.h | 17 +++++++++++++
> mm/compaction.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> mm/migrate.c | 30 +++++++++++++++++++++-
> 3 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b36d08c..360656e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1629,5 +1629,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
> static inline bool page_is_guard(struct page *page) { return false; }
> #endif /* CONFIG_DEBUG_PAGEALLOC */
>
> +#if (defined(CONFIG_VIRTIO_BALLOON) || \
> + defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> +extern int is_balloon_page(struct page *);
> +extern int isolate_balloon_page(struct page *);
> +extern int putback_balloon_page(struct page *);
> +
> +/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
> +static inline int PageBalloon(struct page *page)
> +{
> + return is_balloon_page(page);
> +}
bool
Why is there both is_balloon_page and PageBalloon?
is_ballon_page is so simple it should just be a static inline here
extern struct address_space *balloon_mapping;
static inline bool is_balloon_page(page)
{
return page->mapping == balloon_mapping;
}
> +#else
> +static inline int PageBalloon(struct page *page) { return 0; }
> +static inline int isolate_balloon_page(struct page *page) { return 0; }
> +static inline int putback_balloon_page(struct page *page) { return 0; }
> +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> +
> #endif /* __KERNEL__ */
> #endif /* _LINUX_MM_H */
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 7ea259d..8835d55 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
> @@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> continue;
> }
>
> + /*
> + * For ballooned pages, we need to isolate them before testing
> + * for PageLRU, as well as skip the LRU page isolation steps.
> + */
This says what, but not why.
I didn't check the exact mechanics of a balloon page but I expect it's that
balloon pages are not on the LRU. If they are on the LRU, that's pretty dumb.
/*
* Balloon pages can be migrated but are not on the LRU. Isolate
* them before LRU checks.
*/
It would be nicer to do this without gotos
/*
* It is possible to migrate LRU pages and balloon pages. Skip
* any other type of page
*/
if (is_balloon_page(page)) {
if (!isolate_balloon_page(page))
continue;
} else if (PageLRU(page)) {
....
}
You will need to shuffle things around a little to make it work properly
but if we handle other page types in the future it will be neater
overall.
> + if (PageBalloon(page))
> + if (isolate_balloon_page(page))
> + goto isolated_balloon_page;
> +
> if (!PageLRU(page))
> continue;
>
> @@ -338,6 +347,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>
> /* Successfully isolated */
> del_page_from_lru_list(page, lruvec, page_lru(page));
> +isolated_balloon_page:
> list_add(&page->lru, migratelist);
> cc->nr_migratepages++;
> nr_isolated++;
> @@ -903,4 +913,66 @@ void compaction_unregister_node(struct node *node)
> }
> #endif /* CONFIG_SYSFS && CONFIG_NUMA */
>
> +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
> +/*
> + * Balloon pages special page->mapping.
> + * users must properly allocate and initiliaze an instance of balloon_mapping,
> + * and set it as the page->mapping for balloon enlisted page instances.
> + *
> + * 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(balloon_mapping);
> +
EXPORT_SYMBOL_GPL?
I don't mind how it is exported as such. I'm idly curious if there are
external closed modules that use the driver.
> +/* ballooned page id check */
> +int is_balloon_page(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
> + if (mapping == balloon_mapping)
> + return 1;
> + return 0;
> +}
> +
> +/* __isolate_lru_page() counterpart for a ballooned page */
> +int isolate_balloon_page(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
This is a publicly visible function and while your current usage looks
correct it would not hurt to do something like this;
if (WARN_ON(!is_page_ballon(page))
return 0;
> + if (mapping->a_ops->invalidatepage) {
> + /*
> + * We can race against move_to_new_page() and stumble across a
> + * locked 'newpage'. If we succeed on isolating it, the result
> + * tends to be disastrous. So, we sanely skip PageLocked here.
> + */
> + if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
But the page can get locked after this point.
Would it not be better to do a trylock_page() and unlock the page on
exit after the isolation completes?
> + /*
> + * A ballooned page, by default, has just one refcount.
> + * Prevent concurrent compaction threads from isolating
> + * an already isolated balloon page.
> + */
> + if (page_count(page) == 2) {
> + mapping->a_ops->invalidatepage(page, 0);
> + return 1;
> + }
> + /* Drop refcount taken for this already isolated page */
> + put_page(page);
> + }
> + }
> + return 0;
> +}
Otherwise looks reasonable. The comments really help so thanks for that.
> +
> +/* putback_lru_page() counterpart for a ballooned page */
> +int putback_balloon_page(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
> + if (mapping->a_ops->freepage) {
> + mapping->a_ops->freepage(page);
> + put_page(page);
> + return 1;
> + }
> + return 0;
> +}
> +#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
> #endif /* CONFIG_COMPACTION */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index be26d5c..ffc02a4 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(PageBalloon(page)))
> + VM_BUG_ON(!putback_balloon_page(page));
Why not BUG_ON?
What shocked me actually is that VM_BUG_ON code is executed on
!CONFIG_DEBUG_VM builds and has been since 2.6.36 due to commit [4e60c86bd:
gcc-4.6: mm: fix unused but set warnings]. I thought the whole point of
VM_BUG_ON was to avoid expensive and usually unnecessary checks. Andi,
was this deliberate?
Either way, you always want to call putback_ballon_page() so BUG_ON is
more appropriate although gracefully recovering from the situation and a
WARN would be better.
> + else
> + putback_lru_page(page);
> }
> }
>
> @@ -783,6 +786,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> }
> }
>
> + if (PageBalloon(page)) {
> + /*
> + * A ballooned page does not need any special attention from
> + * physical to virtual reverse mapping procedures.
> + * To avoid burning cycles at rmap level,
> + * skip attempts to unmap PTEs or remap swapcache.
> + */
> + 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 (PageBalloon(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.2
>
--
Mel Gorman
SUSE Labs
On 06/25/2012 07:25 PM, Rafael Aquini wrote:
> This patch introduces helper functions that teach compaction and migration bits
> how to cope with pages which are part of a guest memory balloon, in order to
> make them movable by memory compaction procedures.
>
> Signed-off-by: Rafael Aquini<[email protected]>
The function fill_balloon in drivers/virtio/virtio_balloon.c
should probably add __GFP_MOVABLE to the gfp mask for alloc_pages,
to keep the pageblock where balloon pages are allocated marked
as movable.
--
All rights reversed
On Tue, Jun 26, 2012 at 09:17:43AM -0400, Rik van Riel wrote:
> On 06/25/2012 07:25 PM, Rafael Aquini wrote:
> >This patch introduces helper functions that teach compaction and migration bits
> >how to cope with pages which are part of a guest memory balloon, in order to
> >make them movable by memory compaction procedures.
> >
> >Signed-off-by: Rafael Aquini<[email protected]>
>
> The function fill_balloon in drivers/virtio/virtio_balloon.c
> should probably add __GFP_MOVABLE to the gfp mask for alloc_pages,
> to keep the pageblock where balloon pages are allocated marked
> as movable.
>
You're right, but patch 3 is already doing that at the same time the
migration primitives are introduced. It does mean the full series has to
be applied to do anything but I think it's bisect safe.
--
Mel Gorman
SUSE Labs
>
> What shocked me actually is that VM_BUG_ON code is executed on
> !CONFIG_DEBUG_VM builds and has been since 2.6.36 due to commit [4e60c86bd:
> gcc-4.6: mm: fix unused but set warnings]. I thought the whole point of
> VM_BUG_ON was to avoid expensive and usually unnecessary checks. Andi,
> was this deliberate?
The idea was that the compiler optimizes it away anyways.
I'm not fully sure what putback_balloon_page does, but if it just tests
a bit (without non variable test_bit) it should be ok.
-Andi
> I'm not fully sure what putback_balloon_page does, but if it just tests
> a bit (without non variable test_bit) it should be ok.
^^^^^^^^^^^^^
should be "without variable ..."
-Andi
--
[email protected] -- Speaking for myself only.
On Tue, Jun 26, 2012 at 06:52:58PM +0200, Andi Kleen wrote:
> >
> > What shocked me actually is that VM_BUG_ON code is executed on
> > !CONFIG_DEBUG_VM builds and has been since 2.6.36 due to commit [4e60c86bd:
> > gcc-4.6: mm: fix unused but set warnings]. I thought the whole point of
> > VM_BUG_ON was to avoid expensive and usually unnecessary checks. Andi,
> > was this deliberate?
>
> The idea was that the compiler optimizes it away anyways.
>
> I'm not fully sure what putback_balloon_page does, but if it just tests
> a bit (without non variable test_bit) it should be ok.
>
This was the definition before
#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { } while (0)
#endif
and now it's
#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
#endif
How is the compiler meant to optimise away "cond" if it's a function
call?
In the old definition VM_BUG_ON did nothing and the intention was that the
"cond" should never had any side-effects. It was to be used for potentially
expensive tests to catch additional issues in DEBUG_VM kernels. My concern
is that after commit 4e60c86bd that the VM doing these additional checks
unnecesarily with a performance hit. In most cases the checks are small
but in others such as free_pages we are calling virt_addr_valid() which
is heavier.
What did I miss? If nothing, then I will revert this particular change
and Rafael will need to be sure his patch is not using VM_BUG_ON to call
a function with side-effects.
--
Mel Gorman
SUSE Labs
> How is the compiler meant to optimise away "cond" if it's a function
> call?
Inlines can be optimized away. These tests are usually inlines.
> What did I miss? If nothing, then I will revert this particular change
> and Rafael will need to be sure his patch is not using VM_BUG_ON to call
> a function with side-effects.
Do you have an example where the code is actually different,
or are you just speculating?
FWIW for my config both generates the same code:
size vmlinux-andi-vmbug vmlinux-vmbug-nothing
text data bss dec hex filename
11809704 1457352 1159168 14426224 dc2070 vmlinux-andi-vmbug
11809704 1457352 1159168 14426224 dc2070 vmlinux-vmbug-nothing
-Andi
Mel,
First and foremost, thank you for taking the time to review these bits and
provide such valuable feedback.
On Tue, Jun 26, 2012 at 11:17:29AM +0100, Mel Gorman wrote:
> > +/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
> > +static inline int PageBalloon(struct page *page)
> > +{
> > + return is_balloon_page(page);
> > +}
>
> bool
>
> Why is there both is_balloon_page and PageBalloon?
>
> is_ballon_page is so simple it should just be a static inline here
>
> extern struct address_space *balloon_mapping;
> static inline bool is_balloon_page(page)
> {
> return page->mapping == balloon_mapping;
> }
>
I was thinking about sustain the same syntax other page tests utilize,
but I rather stick to your suggestion on this one.
> > #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> > @@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> > continue;
> > }
> >
> > + /*
> > + * For ballooned pages, we need to isolate them before testing
> > + * for PageLRU, as well as skip the LRU page isolation steps.
> > + */
>
> This says what, but not why.
>
> I didn't check the exact mechanics of a balloon page but I expect it's that
> balloon pages are not on the LRU. If they are on the LRU, that's pretty dumb.
>
>
> /*
> * Balloon pages can be migrated but are not on the LRU. Isolate
> * them before LRU checks.
> */
>
>
> It would be nicer to do this without gotos
>
> /*
> * It is possible to migrate LRU pages and balloon pages. Skip
> * any other type of page
> */
> if (is_balloon_page(page)) {
> if (!isolate_balloon_page(page))
> continue;
> } else if (PageLRU(page)) {
> ....
> }
>
> You will need to shuffle things around a little to make it work properly
> but if we handle other page types in the future it will be neater
> overall.
>
I'm glad you've put things this way on this one. Despite I was thinking on doing it
the way you suggested, I took the goto approach because I was afraid of doing
otherwise could be considered as an unnecessary radical surgery on established code.
Will do it, certainly.
> > +struct address_space *balloon_mapping;
> > +EXPORT_SYMBOL(balloon_mapping);
> > +
>
> EXPORT_SYMBOL_GPL?
>
> I don't mind how it is exported as such. I'm idly curious if there are
> external closed modules that use the driver.
>
To be honest with you, that was picked with no particular case in mind. And, since
you've raised this question, I'm also curious. However, after giving a thought
on your feedback, I believe EXPORT_SYMBOL_GPL suits far better.
> > +/* ballooned page id check */
> > +int is_balloon_page(struct page *page)
> > +{
> > + struct address_space *mapping = page->mapping;
> > + if (mapping == balloon_mapping)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +/* __isolate_lru_page() counterpart for a ballooned page */
> > +int isolate_balloon_page(struct page *page)
> > +{
> > + struct address_space *mapping = page->mapping;
>
> This is a publicly visible function and while your current usage looks
> correct it would not hurt to do something like this;
>
> if (WARN_ON(!is_page_ballon(page))
> return 0;
>
Excellent point!
> > + if (mapping->a_ops->invalidatepage) {
> > + /*
> > + * We can race against move_to_new_page() and stumble across a
> > + * locked 'newpage'. If we succeed on isolating it, the result
> > + * tends to be disastrous. So, we sanely skip PageLocked here.
> > + */
> > + if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
>
> But the page can get locked after this point.
>
> Would it not be better to do a trylock_page() and unlock the page on
> exit after the isolation completes?
>
Far better, for sure! thanks (again)
> > @@ -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(PageBalloon(page)))
> > + VM_BUG_ON(!putback_balloon_page(page));
>
> Why not BUG_ON?
>
> What shocked me actually is that VM_BUG_ON code is executed on
> !CONFIG_DEBUG_VM builds and has been since 2.6.36 due to commit [4e60c86bd:
> gcc-4.6: mm: fix unused but set warnings]. I thought the whole point of
> VM_BUG_ON was to avoid expensive and usually unnecessary checks. Andi,
> was this deliberate?
>
> Either way, you always want to call putback_ballon_page() so BUG_ON is
> more appropriate although gracefully recovering from the situation and a
> WARN would be better.
>
Shame on me!
I was lazy enough to not carefully read VM_BUG_ON's definition and get its
original purpose. Will change it, for sure.
Once more, thank you!
> +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
> +/*
> + * Balloon pages special page->mapping.
> + * users must properly allocate and initiliaze an instance of balloon_mapping,
initialize
> + * and set it as the page->mapping for balloon enlisted page instances.
> + *
> + * 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(balloon_mapping);
Why don't you call this kvm_balloon_mapping - and when other balloon
drivers use it, then change it to something more generic. Also at that
future point the other balloon drivers might do it a bit differently so
it might be that will be reworked completly.
On Tue, Jun 26, 2012 at 10:34:00PM +0200, Andi Kleen wrote:
> > How is the compiler meant to optimise away "cond" if it's a function
> > call?
>
> Inlines can be optimized away. These tests are usually inlines.
>
> > What did I miss? If nothing, then I will revert this particular change
> > and Rafael will need to be sure his patch is not using VM_BUG_ON to call
> > a function with side-effects.
>
> Do you have an example where the code is actually different,
> or are you just speculating?
>
> FWIW for my config both generates the same code:
>
> size vmlinux-andi-vmbug vmlinux-vmbug-nothing
> text data bss dec hex filename
> 11809704 1457352 1159168 14426224 dc2070 vmlinux-andi-vmbug
> 11809704 1457352 1159168 14426224 dc2070 vmlinux-vmbug-nothing
>
They are the same size because CONFIG_DEBUG_VM and !CONFIG_DEBUG_VM
generate the same code! I applied the patch below again 3.4 and got the
following results
text data bss dec hex filename
6918617 1795640 2260992 10975249 a77811 vmlinux-default-no-vmdebug
6916633 1795640 2260992 10973265 a77051 vmlinux-patched-no-vmdebug
That's almost 2K of text!
I see now that in 3.5 this was already spotted and fixed by Konstantin
in commit [02602a18: bug: completely remove code generated by disabled
VM_BUG_ON()]. That patch restores the rule that VM_BUG_ON() cannot call
anything with side-effects. So Rafael, watch your use of VM_BUG_ON or
you'll find that the your patches work in 3.4 and leak in 3.5.
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index c04ecfe..ee24ef8 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -4,7 +4,7 @@
#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
-#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
+#define VM_BUG_ON(cond) do { } while (0)
#endif
#ifdef CONFIG_DEBUG_VIRTUAL
On Tue, Jun 26, 2012 at 07:57:55PM -0400, Konrad Rzeszutek Wilk wrote:
> > +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
> > +/*
> > + * Balloon pages special page->mapping.
> > + * users must properly allocate and initiliaze an instance of balloon_mapping,
>
> initialize
>
Thanks! will fix it.
> > + * and set it as the page->mapping for balloon enlisted page instances.
> > + *
> > + * 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(balloon_mapping);
>
> Why don't you call this kvm_balloon_mapping - and when other balloon
> drivers use it, then change it to something more generic. Also at that
> future point the other balloon drivers might do it a bit differently so
> it might be that will be reworked completly.
Ok, I see your point. However I really think it's better to keep the naming as
generic as possible today and, in the future, those who need to change it a bit can
do it with no pain at all. I believe this way we potentially prevent unnecessary code
duplication, as it will just be a matter of adjusting those preprocessor checking to
include other balloon driver to the scheme, or get rid of all of them (in case all
balloon drivers assume the very same technique for their page mobility primitives).
As I can be utterly wrong on this, lets see if other folks raise the same
concerns about this naming scheme I'm using here. If it ends up being a general
concern that it would be better not being generic at this point, I'll happily
switch my approach to whatever comes up to be the most feasible way of doing it.
Thanks a lot for taking such consideration and provide good feedback on this
work.
On Wed, Jun 27, 2012 at 12:17:17PM -0300, Rafael Aquini wrote:
> On Tue, Jun 26, 2012 at 07:57:55PM -0400, Konrad Rzeszutek Wilk wrote:
> > > +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
.. snip..
> > > +struct address_space *balloon_mapping;
> > > +EXPORT_SYMBOL(balloon_mapping);
> >
> > Why don't you call this kvm_balloon_mapping - and when other balloon
> > drivers use it, then change it to something more generic. Also at that
> > future point the other balloon drivers might do it a bit differently so
> > it might be that will be reworked completly.
>
> Ok, I see your point. However I really think it's better to keep the naming as
> generic as possible today and, in the future, those who need to change it a bit can
> do it with no pain at all. I believe this way we potentially prevent unnecessary code
> duplication, as it will just be a matter of adjusting those preprocessor checking to
> include other balloon driver to the scheme, or get rid of all of them (in case all
> balloon drivers assume the very same technique for their page mobility primitives).
Either way, if a driver is going to use this, they would need to adjust the
preprocessor checking (as you pointed out) to include: #ifdef CONFIG_HYPERVISORX_BALLOON
in this file. At which point they might as well rename the exported symbol to be more
generic - and do whatever else they need to do (add extra stuff maybe?).
>
> As I can be utterly wrong on this, lets see if other folks raise the same
> concerns about this naming scheme I'm using here. If it ends up being a general
> concern that it would be better not being generic at this point, I'll happily
> switch my approach to whatever comes up to be the most feasible way of doing it.
My point here is that its more of name-space pollution. I've gotten flak on doing
this with drivers - which had very generic sounding names, and it made more sense
to rename them with a proper prefix. You are adding pieces of code for the
benefit of one driver.
But that (getting flak on the namespace) might be because the mailing list where I
had posted had more aggressive reviewers and this one is composed of more mellow folks
who are OK with this. Andrew is the final man - and I am not sure what he
prefers.