Mailing List Archive: Linux: Kernel

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

Index | Next | Previous | View Threaded

aquini at redhat

Jul 17, 2012, 9:50 AM

Post #1 of 16 (104 views)
Permalink

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

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 [at] redhat>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

minchan at kernel

Jul 17, 2012, 10:48 PM

Post #2 of 16 (99 views)
Permalink

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

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 [at] redhat>
> ---
> 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 [at] kvack For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont [at] kvack"> email [at] kvack </a>

--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

akpm at linux-foundation

Jul 18, 2012, 3:46 PM

Post #3 of 16 (103 views)
Permalink

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

On Tue, 17 Jul 2012 13:50:41 -0300
Rafael Aquini <aquini [at] redhat> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Jul 18, 2012, 4:07 PM

Post #4 of 16 (105 views)
Permalink

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

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 [at] redhat> 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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

akpm at linux-foundation

Jul 18, 2012, 4:12 PM

Post #5 of 16 (101 views)
Permalink

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

On Wed, 18 Jul 2012 20:07:07 -0300
Rafael Aquini <aquini [at] redhat> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Jul 18, 2012, 6:00 PM

Post #6 of 16 (100 views)
Permalink

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

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 [at] redhat> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

akpm at linux-foundation

Jul 18, 2012, 6:29 PM

Post #7 of 16 (93 views)
Permalink

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

On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini [at] redhat> 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()?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Jul 19, 2012, 5:32 AM

Post #8 of 16 (95 views)
Permalink

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

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 [at] redhat> 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 */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Jul 20, 2012, 12:48 PM

Post #9 of 16 (91 views)
Permalink

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

minchan at kernel

Jul 22, 2012, 7:33 PM

Post #10 of 16 (89 views)
Permalink

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

minchan at kernel

Jul 22, 2012, 7:35 PM

Post #11 of 16 (90 views)
Permalink

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

aquini at redhat

Jul 23, 2012, 11:19 AM

Post #12 of 16 (88 views)
Permalink

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

riel at redhat

Aug 1, 2012, 1:51 PM

Post #13 of 16 (60 views)
Permalink

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

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

riel at redhat

Aug 1, 2012, 1:53 PM

Post #14 of 16 (60 views)
Permalink

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Aug 3, 2012, 4:13 AM

Post #15 of 16 (59 views)
Permalink

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

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

aquini at redhat

Aug 3, 2012, 5:26 AM

Post #16 of 16 (61 views)
Permalink

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

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads