[PATCH 1/2] mm: change enum migrate_mode with bitwise type

[PATCH 1/2] mm: change enum migrate_mode with bitwise type

[Minchan Kim]

This patch changes migrate_mode type to bitwise type because
next patch will add MIGRATE_DISCARD and it could be ORed with other
attributes so it would be better to change it with bitwise type.

Cc: Rik van Riel <riel@redhat.com>
Cc: Mel Gorman <mgorman@suse.de>
Suggested-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 fs/btrfs/disk-io.c           |    2 +-
 fs/hugetlbfs/inode.c         |    2 +-
 fs/nfs/internal.h            |    2 +-
 fs/nfs/write.c               |    2 +-
 include/linux/fs.h           |    4 ++--
 include/linux/migrate.h      |   10 +++++-----
 include/linux/migrate_mode.h |   15 +++++++++------
 mm/migrate.c                 |   38 +++++++++++++++++++-------------------
 8 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 62e0caf..70fbbe1 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -901,7 +901,7 @@ static int btree_submit_bio_hook(struct inode *inode, int rw, struct bio *bio,
 #ifdef CONFIG_MIGRATION
 static int btree_migratepage(struct address_space *mapping,
 			struct page *newpage, struct page *page,
-			enum migrate_mode mode)
+			migrate_mode_t mode)
 {
 	/*
 	 * we can't safely write a btree page from here,
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 7f11118..2b254f9 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -604,7 +604,7 @@ static int hugetlbfs_set_page_dirty(struct page *page)
 
 static int hugetlbfs_migrate_page(struct address_space *mapping,
 				struct page *newpage, struct page *page,
-				enum migrate_mode mode)
+				migrate_mode_t mode)
 {
 	int rc;
 
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 31fdb03..d554438 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -452,7 +452,7 @@ void nfs_init_cinfo(struct nfs_commit_info *cinfo,
 
 #ifdef CONFIG_MIGRATION
 extern int nfs_migrate_page(struct address_space *,
-		struct page *, struct page *, enum migrate_mode);
+		struct page *, struct page *, migrate_mode_t);
 #else
 #define nfs_migrate_page NULL
 #endif
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index e3b5537..093889b 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1783,7 +1783,7 @@ out_error:
 
 #ifdef CONFIG_MIGRATION
 int nfs_migrate_page(struct address_space *mapping, struct page *newpage,
-		struct page *page, enum migrate_mode mode)
+		struct page *page, migrate_mode_t mode)
 {
 	/*
 	 * If PagePrivate is set, then the page is currently associated with
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0b25c5d..a7fbdc6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -637,7 +637,7 @@ struct address_space_operations {
 	 * is false, it must not block.
 	 */
 	int (*migratepage) (struct address_space *,
-			struct page *, struct page *, enum migrate_mode);
+			struct page *, struct page *, migrate_mode_t);
 	int (*launder_page) (struct page *);
 	int (*is_partially_uptodate) (struct page *, read_descriptor_t *,
 					unsigned long);
@@ -2734,7 +2734,7 @@ extern int generic_check_addressable(unsigned, u64);
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
 				struct page *, struct page *,
-				enum migrate_mode);
+				migrate_mode_t);
 #else
 #define buffer_migrate_page NULL
 #endif
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index ce7e667..f7a50f5 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -11,13 +11,13 @@ typedef struct page *new_page_t(struct page *, unsigned long private, int **);
 
 extern void putback_lru_pages(struct list_head *l);
 extern int migrate_page(struct address_space *,
-			struct page *, struct page *, enum migrate_mode);
+			struct page *, struct page *, migrate_mode_t);
 extern int migrate_pages(struct list_head *l, new_page_t x,
 			unsigned long private, bool offlining,
-			enum migrate_mode mode);
+			migrate_mode_t mode);
 extern int migrate_huge_page(struct page *, new_page_t x,
 			unsigned long private, bool offlining,
-			enum migrate_mode mode);
+			migrate_mode_t mode);
 
 extern int fail_migrate_page(struct address_space *,
 			struct page *, struct page *);
@@ -35,10 +35,10 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
 static inline void putback_lru_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t x,
 		unsigned long private, bool offlining,
-		enum migrate_mode mode) { return -ENOSYS; }
+		migrate_mode_t mode) { return -ENOSYS; }
 static inline int migrate_huge_page(struct page *page, new_page_t x,
 		unsigned long private, bool offlining,
-		enum migrate_mode mode) { return -ENOSYS; }
+		migrate_mode_t mode) { return -ENOSYS; }
 
 static inline int migrate_prep(void) { return -ENOSYS; }
 static inline int migrate_prep_local(void) { return -ENOSYS; }
diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
index ebf3d89..8848cad 100644
--- a/include/linux/migrate_mode.h
+++ b/include/linux/migrate_mode.h
@@ -1,16 +1,19 @@
 #ifndef MIGRATE_MODE_H_INCLUDED
 #define MIGRATE_MODE_H_INCLUDED
+
+/* MIGRATE_ASYNC means never block */
+#define MIGRATE_ASYNC		((__force migrate_mode_t)0x1)
 /*
- * MIGRATE_ASYNC means never block
  * MIGRATE_SYNC_LIGHT in the current implementation means to allow blocking
  *	on most operations but not ->writepage as the potential stall time
  *	is too significant
+ */
+#define MIGRATE_SYNC_LIGHT	((__force migrate_mode_t)0x2)
+/*
  * MIGRATE_SYNC will block when migrating pages
  */
-enum migrate_mode {
-	MIGRATE_ASYNC,
-	MIGRATE_SYNC_LIGHT,
-	MIGRATE_SYNC,
-};
+#define MIGRATE_SYNC		((__force migrate_mode_t)0x4)
+
+typedef unsigned __bitwise__ migrate_mode_t;
 
 #endif		/* MIGRATE_MODE_H_INCLUDED */
diff --git a/mm/migrate.c b/mm/migrate.c
index 77ed2d7..28d464b 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -220,12 +220,12 @@ out:
 #ifdef CONFIG_BLOCK
 /* Returns true if all buffers are successfully locked */
 static bool buffer_migrate_lock_buffers(struct buffer_head *head,
-							enum migrate_mode mode)
+							migrate_mode_t mode)
 {
 	struct buffer_head *bh = head;
 
 	/* Simple case, sync compaction */
-	if (mode != MIGRATE_ASYNC) {
+	if (!(mode & MIGRATE_ASYNC)) {
 		do {
 			get_bh(bh);
 			lock_buffer(bh);
@@ -261,7 +261,7 @@ static bool buffer_migrate_lock_buffers(struct buffer_head *head,
 }
 #else
 static inline bool buffer_migrate_lock_buffers(struct buffer_head *head,
-							enum migrate_mode mode)
+							migrate_mode_t mode)
 {
 	return true;
 }
@@ -277,7 +277,7 @@ static inline bool buffer_migrate_lock_buffers(struct buffer_head *head,
  */
 static int migrate_page_move_mapping(struct address_space *mapping,
 		struct page *newpage, struct page *page,
-		struct buffer_head *head, enum migrate_mode mode)
+		struct buffer_head *head, migrate_mode_t mode)
 {
 	int expected_count;
 	void **pslot;
@@ -313,7 +313,7 @@ static int migrate_page_move_mapping(struct address_space *mapping,
 	 * the mapping back due to an elevated page count, we would have to
 	 * block waiting on other references to be dropped.
 	 */
-	if (mode == MIGRATE_ASYNC && head &&
+	if ((mode & MIGRATE_ASYNC) && head &&
 			!buffer_migrate_lock_buffers(head, mode)) {
 		page_unfreeze_refs(page, expected_count);
 		spin_unlock_irq(&mapping->tree_lock);
@@ -478,7 +478,7 @@ EXPORT_SYMBOL(fail_migrate_page);
  */
 int migrate_page(struct address_space *mapping,
 		struct page *newpage, struct page *page,
-		enum migrate_mode mode)
+		migrate_mode_t mode)
 {
 	int rc;
 
@@ -501,7 +501,7 @@ EXPORT_SYMBOL(migrate_page);
  * exist.
  */
 int buffer_migrate_page(struct address_space *mapping,
-		struct page *newpage, struct page *page, enum migrate_mode mode)
+		struct page *newpage, struct page *page, migrate_mode_t mode)
 {
 	struct buffer_head *bh, *head;
 	int rc;
@@ -521,7 +521,7 @@ int buffer_migrate_page(struct address_space *mapping,
 	 * with an IRQ-safe spinlock held. In the sync case, the buffers
 	 * need to be locked now
 	 */
-	if (mode != MIGRATE_ASYNC)
+	if (!(mode & MIGRATE_ASYNC))
 		BUG_ON(!buffer_migrate_lock_buffers(head, mode));
 
 	ClearPagePrivate(page);
@@ -599,11 +599,11 @@ static int writeout(struct address_space *mapping, struct page *page)
  * Default handling if a filesystem does not provide a migration function.
  */
 static int fallback_migrate_page(struct address_space *mapping,
-	struct page *newpage, struct page *page, enum migrate_mode mode)
+	struct page *newpage, struct page *page, migrate_mode_t mode)
 {
 	if (PageDirty(page)) {
 		/* Only writeback pages in full synchronous migration */
-		if (mode != MIGRATE_SYNC)
+		if (!(mode & MIGRATE_SYNC))
 			return -EBUSY;
 		return writeout(mapping, page);
 	}
@@ -631,7 +631,7 @@ static int fallback_migrate_page(struct address_space *mapping,
  *  == 0 - success
  */
 static int move_to_new_page(struct page *newpage, struct page *page,
-				int remap_swapcache, enum migrate_mode mode)
+				int remap_swapcache, migrate_mode_t mode)
 {
 	struct address_space *mapping;
 	int rc;
@@ -679,7 +679,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 }
 
 static int __unmap_and_move(struct page *page, struct page *newpage,
-			int force, bool offlining, enum migrate_mode mode)
+			int force, bool offlining, migrate_mode_t mode)
 {
 	int rc = -EAGAIN;
 	int remap_swapcache = 1;
@@ -687,7 +687,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 	struct anon_vma *anon_vma = NULL;
 
 	if (!trylock_page(page)) {
-		if (!force || mode == MIGRATE_ASYNC)
+		if (!force || (mode & MIGRATE_ASYNC))
 			goto out;
 
 		/*
@@ -733,7 +733,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		 * the retry loop is too short and in the sync-light case,
 		 * the overhead of stalling is too much
 		 */
-		if (mode != MIGRATE_SYNC) {
+		if (!(mode & MIGRATE_SYNC)) {
 			rc = -EBUSY;
 			goto uncharge;
 		}
@@ -827,7 +827,7 @@ out:
  */
 static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 			struct page *page, int force, bool offlining,
-			enum migrate_mode mode)
+			migrate_mode_t mode)
 {
 	int rc = 0;
 	int *result = NULL;
@@ -894,7 +894,7 @@ out:
 static int unmap_and_move_huge_page(new_page_t get_new_page,
 				unsigned long private, struct page *hpage,
 				int force, bool offlining,
-				enum migrate_mode mode)
+				migrate_mode_t mode)
 {
 	int rc = 0;
 	int *result = NULL;
@@ -907,7 +907,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	rc = -EAGAIN;
 
 	if (!trylock_page(hpage)) {
-		if (!force || mode != MIGRATE_SYNC)
+		if (!force || !(mode & MIGRATE_SYNC))
 			goto out;
 		lock_page(hpage);
 	}
@@ -958,7 +958,7 @@ out:
  */
 int migrate_pages(struct list_head *from,
 		new_page_t get_new_page, unsigned long private, bool offlining,
-		enum migrate_mode mode)
+		migrate_mode_t mode)
 {
 	int retry = 1;
 	int nr_failed = 0;
@@ -1009,7 +1009,7 @@ out:
 
 int migrate_huge_page(struct page *hpage, new_page_t get_new_page,
 		      unsigned long private, bool offlining,
-		      enum migrate_mode mode)
+		      migrate_mode_t mode)
 {
 	int pass, rc;
 
-- 
1.7.9.5

[PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

This patch introudes MIGRATE_DISCARD mode in migration.
It drops *clean cache pages* instead of migration so that
migration latency could be reduced by avoiding (memcpy + page remapping).
It's useful for CMA because latency of migration is very important rather
than eviction of background processes's workingset. In addition, it needs
less free pages for migration targets so it could avoid memory reclaiming
to get free pages, which is another factor increase latency.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/migrate_mode.h |    7 +++++++
 mm/migrate.c                 |   41 ++++++++++++++++++++++++++++++++++++++---
 mm/page_alloc.c              |    2 +-
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
index 8848cad..4eb1646 100644
--- a/include/linux/migrate_mode.h
+++ b/include/linux/migrate_mode.h
@@ -14,6 +14,13 @@
  */
 #define MIGRATE_SYNC		((__force migrate_mode_t)0x4)
 
+/*
+ * MIGRTATE_DISCARD will discard clean cache page instead of migration.
+ * MIGRATE_ASYNC, MIGRATE_SYNC_LIGHT, MIGRATE_SYNC shouldn't be used
+ * together with OR flag in current implementation.
+ */
+#define MIGRATE_DISCARD		((__force migrate_mode_t)0x8)
+
 typedef unsigned __bitwise__ migrate_mode_t;
 
 #endif		/* MIGRATE_MODE_H_INCLUDED */
diff --git a/mm/migrate.c b/mm/migrate.c
index 28d464b..2de7709 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -678,6 +678,19 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 	return rc;
 }
 
+static int discard_page(struct page *page)
+{
+	int ret = -EAGAIN;
+
+	struct address_space *mapping = page_mapping(page);
+	if (page_has_private(page))
+		if (!try_to_release_page(page, GFP_KERNEL))
+			return ret;
+	if (remove_mapping(mapping, page))
+		ret = 0;
+	return ret;
+}
+
 static int __unmap_and_move(struct page *page, struct page *newpage,
 			int force, bool offlining, migrate_mode_t mode)
 {
@@ -685,6 +698,9 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 	int remap_swapcache = 1;
 	struct mem_cgroup *mem;
 	struct anon_vma *anon_vma = NULL;
+	enum ttu_flags ttu_flags;
+	bool discard_mode = false;
+	bool file = false;
 
 	if (!trylock_page(page)) {
 		if (!force || (mode & MIGRATE_ASYNC))
@@ -799,12 +815,31 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		goto skip_unmap;
 	}
 
+	file = page_is_file_cache(page);
+	ttu_flags = TTU_IGNORE_ACCESS;
+retry:
+	if (!(mode & MIGRATE_DISCARD) || !file || PageDirty(page))
+		ttu_flags |= (TTU_MIGRATION | TTU_IGNORE_MLOCK);
+	else
+		discard_mode = true;
+
 	/* Establish migration ptes or remove ptes */
-	try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+	rc = try_to_unmap(page, ttu_flags);
 
 skip_unmap:
-	if (!page_mapped(page))
-		rc = move_to_new_page(newpage, page, remap_swapcache, mode);
+	if (rc == SWAP_SUCCESS) {
+		if (!discard_mode) {
+			rc = move_to_new_page(newpage, page,
+					remap_swapcache, mode);
+		} else {
+			rc = discard_page(page);
+			goto uncharge;
+		}
+	} else if (rc == SWAP_MLOCK && discard_mode) {
+		mode &= ~MIGRATE_DISCARD;
+		discard_mode = false;
+		goto retry;
+	}
 
 	if (rc && remap_swapcache)
 		remove_migration_ptes(page, page);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ba3100a..e14b960 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5670,7 +5670,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
 
 		ret = migrate_pages(&cc.migratepages,
 				    __alloc_contig_migrate_alloc,
-				    0, false, MIGRATE_SYNC);
+				    0, false, MIGRATE_SYNC|MIGRATE_DISCARD);
 	}
 
 	putback_lru_pages(&cc.migratepages);
-- 
1.7.9.5

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
> 

Bah, this was released while I was reviewing the older version. I did
not read this one as closely but I see the enum problems have gone away
at least. I'd still prefer if CMA had an additional helper to discard
some pages with shrink_page_list() and migrate the remaining pages with
migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
migrate mode at all.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 1/2] mm: change enum migrate_mode with bitwise type

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Wed, Sep 05 2012, Minchan Kim wrote:
> This patch changes migrate_mode type to bitwise type because
> next patch will add MIGRATE_DISCARD and it could be ORed with other
> attributes so it would be better to change it with bitwise type.
>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Suggested-by: Michal Nazarewicz <mina86@mina86.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

On Wed, Sep 05 2012, Minchan Kim wrote:
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

The principle is very good (I always intended for CMA to discard clean
pages but simply did not have time to implement it), and the code looks
reasonably good to me.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

Hi Mel,

On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
> On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> > This patch introudes MIGRATE_DISCARD mode in migration.
> > It drops *clean cache pages* instead of migration so that
> > migration latency could be reduced by avoiding (memcpy + page remapping).
> > It's useful for CMA because latency of migration is very important rather
> > than eviction of background processes's workingset. In addition, it needs
> > less free pages for migration targets so it could avoid memory reclaiming
> > to get free pages, which is another factor increase latency.
> > 
> 
> Bah, this was released while I was reviewing the older version. I did
> not read this one as closely but I see the enum problems have gone away
> at least. I'd still prefer if CMA had an additional helper to discard
> some pages with shrink_page_list() and migrate the remaining pages with
> migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
> migrate mode at all.

I am not convinced with your point. What's the benefit on separating
reclaim and migration? For just removing MIGRATE_DISCARD mode?
I don't think it's not bad because my implementation is very simple(maybe
it's much simpler than separating reclaim and migration) and
could be used by others like memory-hotplug in future.
If you're not strong against with me, I would like to insist on my implementation.

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

-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
> Hi Mel,
> 
> On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
> > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> > > This patch introudes MIGRATE_DISCARD mode in migration.
> > > It drops *clean cache pages* instead of migration so that
> > > migration latency could be reduced by avoiding (memcpy + page remapping).
> > > It's useful for CMA because latency of migration is very important rather
> > > than eviction of background processes's workingset. In addition, it needs
> > > less free pages for migration targets so it could avoid memory reclaiming
> > > to get free pages, which is another factor increase latency.
> > > 
> > 
> > Bah, this was released while I was reviewing the older version. I did
> > not read this one as closely but I see the enum problems have gone away
> > at least. I'd still prefer if CMA had an additional helper to discard
> > some pages with shrink_page_list() and migrate the remaining pages with
> > migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
> > migrate mode at all.
> 
> I am not convinced with your point. What's the benefit on separating
> reclaim and migration? For just removing MIGRATE_DISCARD mode?

Maintainability. There are reclaim functions and there are migration
functions. Your patch takes migrate_pages() and makes it partially a
reclaim function mixing up the responsibilities of migrate.c and vmscan.c.

> I don't think it's not bad because my implementation is very simple(maybe
> it's much simpler than separating reclaim and migration) and
> could be used by others like memory-hotplug in future.

They could also have used the helper function from CMA that takes a list
of pages, reclaims some and migrates other.

> If you're not strong against with me, I would like to insist on my implementation.
> 

I'm not very strongly against it but I'm also very unhappy.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
> On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
> > Hi Mel,
> > 
> > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
> > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> > > > This patch introudes MIGRATE_DISCARD mode in migration.
> > > > It drops *clean cache pages* instead of migration so that
> > > > migration latency could be reduced by avoiding (memcpy + page remapping).
> > > > It's useful for CMA because latency of migration is very important rather
> > > > than eviction of background processes's workingset. In addition, it needs
> > > > less free pages for migration targets so it could avoid memory reclaiming
> > > > to get free pages, which is another factor increase latency.
> > > > 
> > > 
> > > Bah, this was released while I was reviewing the older version. I did
> > > not read this one as closely but I see the enum problems have gone away
> > > at least. I'd still prefer if CMA had an additional helper to discard
> > > some pages with shrink_page_list() and migrate the remaining pages with
> > > migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
> > > migrate mode at all.
> > 
> > I am not convinced with your point. What's the benefit on separating
> > reclaim and migration? For just removing MIGRATE_DISCARD mode?
> 
> Maintainability. There are reclaim functions and there are migration
> functions. Your patch takes migrate_pages() and makes it partially a
> reclaim function mixing up the responsibilities of migrate.c and vmscan.c.
> 
> > I don't think it's not bad because my implementation is very simple(maybe
> > it's much simpler than separating reclaim and migration) and
> > could be used by others like memory-hotplug in future.
> 
> They could also have used the helper function from CMA that takes a list
> of pages, reclaims some and migrates other.
> 

I also do not accept that your approach is inherently simpler than what I
proposed to you. This is not tested at all but it should be functionally
similar to both your patches except that it keeps the responsibility for
reclaim in vmscan.c

Your diffstats are

8 files changed, 39 insertions(+), 36 deletions(-)
3 files changed, 46 insertions(+), 4 deletions(-)

Mine is

 3 files changed, 32 insertions(+), 5 deletions(-)

Fewer files changed and fewer lines inserted.

---8<---
mm: cma: Discard clean pages during contiguous allocation instead of migration

This patch drops clean cache pages instead of migration during
alloc_contig_range() to minimise allocation latency by reducing the amount
of migration is necessary. It's useful for CMA because latency of migration
is more important than evicting the background processes working set.

Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
---
 mm/internal.h   |    1 +
 mm/page_alloc.c |    2 ++
 mm/vmscan.c     |   34 +++++++++++++++++++++++++++++-----
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index b8c91b3..6d4bdf9 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -356,3 +356,4 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
         unsigned long, unsigned long);
 
 extern void set_pageblock_order(void);
+unsigned long reclaim_clean_pages_from_list(struct list_head *page_list);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c66fb87..977bdb2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
 			break;
 		}
 
+		reclaim_clean_pages_from_list(&cc.migratepages);
+
 		ret = migrate_pages(&cc.migratepages,
 				    __alloc_contig_migrate_alloc,
 				    0, false, MIGRATE_SYNC);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8d01243..ccf7bc2 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			goto keep;
 
 		VM_BUG_ON(PageActive(page));
-		VM_BUG_ON(page_zone(page) != zone);
+		VM_BUG_ON(zone && page_zone(page) != zone);
 
 		sc->nr_scanned++;
 
@@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				 * except we already have the page isolated
 				 * and know it's dirty
 				 */
-				inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
+				if (zone)
+					inc_zone_page_state(page,
+							NR_VMSCAN_IMMEDIATE);
 				SetPageReclaim(page);
 
 				goto keep_locked;
@@ -947,7 +949,7 @@ keep:
 	 * back off and wait for congestion to clear because further reclaim
 	 * will encounter the same problem
 	 */
-	if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
+	if (zone && nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
 		zone_set_flag(zone, ZONE_CONGESTED);
 
 	free_hot_cold_page_list(&free_pages, 1);
@@ -955,11 +957,33 @@ keep:
 	list_splice(&ret_pages, page_list);
 	count_vm_events(PGACTIVATE, pgactivate);
 	mem_cgroup_uncharge_end();
-	*ret_nr_dirty += nr_dirty;
-	*ret_nr_writeback += nr_writeback;
+	if (ret_nr_dirty)
+		*ret_nr_dirty += nr_dirty;
+	if (ret_nr_writeback)
+		*ret_nr_writeback += nr_writeback;
 	return nr_reclaimed;
 }
 
+unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
+{
+	struct scan_control sc = {
+		.gfp_mask = GFP_KERNEL,
+		.priority = DEF_PRIORITY,
+	};
+	unsigned long ret;
+	struct page *page, *next;
+	LIST_HEAD(clean_pages);
+
+	list_for_each_entry_safe(page, next, page_list, lru) {
+		if (page_is_file_cache(page) && !PageDirty(page))
+			list_move(&page->lru, &clean_pages);
+	}
+
+	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
+	list_splice(&clean_pages, page_list);
+	return ret;
+}
+
 /*
  * Attempt to remove the specified page from its LRU.  Only take this page
  * if it is of the appropriate PageActive status.  Pages which are being

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Kyungmin Park]

Hi Mel,

After apply your patch, It got the below message.
Please note that it's not the latest kernel. it's kernel v3.0.31 + CMA
+ your patch.
It seems it should not be active but it contains active field.

Thank you,
Kyungmin Park

[   79.160394] c1 BUG: Bad page state in process mediaserver  pfn:72b18
[   79.160424] c1 page:c1579300 count:0 mapcount:0 mapping:  (null) index:0x2
[   79.160454] c1 page flags: 0x20248(uptodate|active|arch_1|mappedtodisk)
[   79.160489] c1 Backtrace:
[   79.160512] c1 [<c005f1b0>] (dump_backtrace+0x0/0x114) from
[<c06516e0>] (dump_stack+0x20/0x24)
[   79.160550] c1  r7:00000000 r6:00000001 r5:c0ad75d0 r4:c1579300
[   79.160585] c1 [<c06516c0>] (dump_stack+0x0/0x24) from [<c014dd88>]
(bad_page+0xb4/0x114)
[   79.160625] c1 [<c014dcd4>] (bad_page+0x0/0x114) from [<c014f654>]
(free_pages_prepare+0x118/0x18c)
[   79.160664] c1  r7:00000000 r6:00000001 r5:c1579300 r4:c1579300
[   79.160700] c1 [<c014f53c>] (free_pages_prepare+0x0/0x18c) from [<c014f81c>]
(free_hot_cold_page+0x30/0x19c)
[   79.160742] c1  r9:ebf51be0 r8:00000001 r7:00000001 r6:c1579300 r5:c1579300
[   79.160777] c1 r4:ebf51c8c
[   79.160796] c1 [<c014f7ec>] (free_hot_cold_page+0x0/0x19c) from [<c014fbd4>]
(__pagevec_free+0x68/0xf4)
[   79.160842] c1 [<c014fb6c>] (__pagevec_free+0x0/0xf4) from [<c012e7a8>] (free
_page_list+0xc4/0xc8)
[   79.160885] c1 [<c012e6e4>] (free_page_list+0x0/0xc8) from [<c012ecb0>] (shri
nk_page_list+0x158/0x834)
[   79.160925] c1  r8:ebf51cac r7:ebf51d20 r6:c1579080 r5:ebf51cec r4:c1579098
[   79.160966] c1 [<c012eb58>] (shrink_page_list+0x0/0x834) from [<c012f488>] (r
eclaim_clean_pages_from_list+0xfc/0x128)
[   79.161016] c1 [<c012f38c>] (reclaim_clean_pages_from_list+0x0/0x128) from [<
c0150a0c>] (alloc_contig_range+0x20c/0x458)
[   79.161062] c1  r8:00072b00 r7:00072b20 r6:00072b24 r5:ebf51d78 r4:00000000
[   79.161103] c1 [<c0150800>] (alloc_contig_range+0x0/0x458) from [<c02e2e64>]
(__dma_alloc_from_contiguous+0xdc/0x170)
[   79.161153] c1 [<c02e2d88>] (__dma_alloc_from_contiguous+0x0/0x170)
from [<c02e2fd8>] (dma_alloc_from_contiguous+0xe0/0xf0)
[   79.161205] c1 [<c02e2ef8>] (dma_alloc_from_contiguous+0x0/0xf0) from [<c0064
ebc>] (__alloc_from_contiguous+0x40/0xc0)
[   79.161254] c1 [<c0064e7c>] (__alloc_from_contiguous+0x0/0xc0) from
[<c0065914>] (__dma_alloc+0x144/0x1a0)
[   79.161295] c1  r9:ebf50000 r8:c005b284 r7:000000d0 r6:00000000 r5:ebf51ed4
[   79.161331] c1 r4:ebf51ed4
[   79.161349] c1 [<c00657d0>] (__dma_alloc+0x0/0x1a0) from
[<c0065a10>] (dma_alloc_coherent+0x64/0x70)
[   79.161388] c1  r5:00024000 r4:ebf51ed4
[   79.161413] c1 [<c00659ac>] (dma_alloc_coherent+0x0/0x70) from
[<c007be48>] (secmem_ioctl+0x448/0x5dc)
[   79.161453] c1  r7:00000000 r6:ebf51ed0 r5:ebf50000 r4:417467f0
[   79.161488] c1 [<c007ba00>] (secmem_ioctl+0x0/0x5dc) from
[<c0171ab0>] (do_vfs_ioctl+0x90/0x5a8)
[   79.161526] c1  r7:00000018 r6:c0045306 r5:d1d63d80 r4:417467f0
[   79.161562] c1 [<c0171a20>] (do_vfs_ioctl+0x0/0x5a8) from [<c0172010>] (sys_i
octl+0x48/0x70)
[   79.161603] c1 [<c0171fc8>] (sys_ioctl+0x0/0x70) from [<c005b040>] (ret_fast_
syscall+0x0/0x30)
[   79.161640] c1  r7:00000036 r6:00000028 r5:00024000 r4:40e18750


On 9/6/12, Mel Gorman <mgorman@suse.de> wrote:
> On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
>> On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
>> > Hi Mel,
>> >
>> > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
>> > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
>> > > > This patch introudes MIGRATE_DISCARD mode in migration.
>> > > > It drops *clean cache pages* instead of migration so that
>> > > > migration latency could be reduced by avoiding (memcpy + page
>> > > > remapping).
>> > > > It's useful for CMA because latency of migration is very important
>> > > > rather
>> > > > than eviction of background processes's workingset. In addition, it
>> > > > needs
>> > > > less free pages for migration targets so it could avoid memory
>> > > > reclaiming
>> > > > to get free pages, which is another factor increase latency.
>> > > >
>> > >
>> > > Bah, this was released while I was reviewing the older version. I did
>> > > not read this one as closely but I see the enum problems have gone
>> > > away
>> > > at least. I'd still prefer if CMA had an additional helper to discard
>> > > some pages with shrink_page_list() and migrate the remaining pages
>> > > with
>> > > migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
>> > > migrate mode at all.
>> >
>> > I am not convinced with your point. What's the benefit on separating
>> > reclaim and migration? For just removing MIGRATE_DISCARD mode?
>>
>> Maintainability. There are reclaim functions and there are migration
>> functions. Your patch takes migrate_pages() and makes it partially a
>> reclaim function mixing up the responsibilities of migrate.c and
>> vmscan.c.
>>
>> > I don't think it's not bad because my implementation is very
>> > simple(maybe
>> > it's much simpler than separating reclaim and migration) and
>> > could be used by others like memory-hotplug in future.
>>
>> They could also have used the helper function from CMA that takes a list
>> of pages, reclaims some and migrates other.
>>
>
> I also do not accept that your approach is inherently simpler than what I
> proposed to you. This is not tested at all but it should be functionally
> similar to both your patches except that it keeps the responsibility for
> reclaim in vmscan.c
>
> Your diffstats are
>
> 8 files changed, 39 insertions(+), 36 deletions(-)
> 3 files changed, 46 insertions(+), 4 deletions(-)
>
> Mine is
>
>  3 files changed, 32 insertions(+), 5 deletions(-)
>
> Fewer files changed and fewer lines inserted.
>
> ---8<---
> mm: cma: Discard clean pages during contiguous allocation instead of
> migration
>
> This patch drops clean cache pages instead of migration during
> alloc_contig_range() to minimise allocation latency by reducing the amount
> of migration is necessary. It's useful for CMA because latency of migration
> is more important than evicting the background processes working set.
>
> Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
> ---
>  mm/internal.h   |    1 +
>  mm/page_alloc.c |    2 ++
>  mm/vmscan.c     |   34 +++++++++++++++++++++++++++++-----
>  3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index b8c91b3..6d4bdf9 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -356,3 +356,4 @@ extern unsigned long vm_mmap_pgoff(struct file *,
> unsigned long,
>          unsigned long, unsigned long);
>
>  extern void set_pageblock_order(void);
> +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c66fb87..977bdb2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned long
> start, unsigned long end)
>  			break;
>  		}
>
> +		reclaim_clean_pages_from_list(&cc.migratepages);
> +
>  		ret = migrate_pages(&cc.migratepages,
>  				    __alloc_contig_migrate_alloc,
>  				    0, false, MIGRATE_SYNC);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8d01243..ccf7bc2 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct list_head
> *page_list,
>  			goto keep;
>
>  		VM_BUG_ON(PageActive(page));
> -		VM_BUG_ON(page_zone(page) != zone);
> +		VM_BUG_ON(zone && page_zone(page) != zone);
>
>  		sc->nr_scanned++;
>
> @@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct list_head
> *page_list,
>  				 * except we already have the page isolated
>  				 * and know it's dirty
>  				 */
> -				inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
> +				if (zone)
> +					inc_zone_page_state(page,
> +							NR_VMSCAN_IMMEDIATE);
>  				SetPageReclaim(page);
>
>  				goto keep_locked;
> @@ -947,7 +949,7 @@ keep:
>  	 * back off and wait for congestion to clear because further reclaim
>  	 * will encounter the same problem
>  	 */
> -	if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
> +	if (zone && nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
>  		zone_set_flag(zone, ZONE_CONGESTED);
>
>  	free_hot_cold_page_list(&free_pages, 1);
> @@ -955,11 +957,33 @@ keep:
>  	list_splice(&ret_pages, page_list);
>  	count_vm_events(PGACTIVATE, pgactivate);
>  	mem_cgroup_uncharge_end();
> -	*ret_nr_dirty += nr_dirty;
> -	*ret_nr_writeback += nr_writeback;
> +	if (ret_nr_dirty)
> +		*ret_nr_dirty += nr_dirty;
> +	if (ret_nr_writeback)
> +		*ret_nr_writeback += nr_writeback;
>  	return nr_reclaimed;
>  }
>
> +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
> +{
> +	struct scan_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +		.priority = DEF_PRIORITY,
> +	};
> +	unsigned long ret;
> +	struct page *page, *next;
> +	LIST_HEAD(clean_pages);
> +
> +	list_for_each_entry_safe(page, next, page_list, lru) {
> +		if (page_is_file_cache(page) && !PageDirty(page))
> +			list_move(&page->lru, &clean_pages);
> +	}
> +
> +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
> +	list_splice(&clean_pages, page_list);
> +	return ret;
> +}
> +
>  /*
>   * Attempt to remove the specified page from its LRU.  Only take this page
>   * if it is of the appropriate PageActive status.  Pages which are being
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

On Thu, Sep 06, 2012 at 10:03:25AM +0100, Mel Gorman wrote:
> On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
> > On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
> > > Hi Mel,
> > > 
> > > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
> > > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> > > > > This patch introudes MIGRATE_DISCARD mode in migration.
> > > > > It drops *clean cache pages* instead of migration so that
> > > > > migration latency could be reduced by avoiding (memcpy + page remapping).
> > > > > It's useful for CMA because latency of migration is very important rather
> > > > > than eviction of background processes's workingset. In addition, it needs
> > > > > less free pages for migration targets so it could avoid memory reclaiming
> > > > > to get free pages, which is another factor increase latency.
> > > > > 
> > > > 
> > > > Bah, this was released while I was reviewing the older version. I did
> > > > not read this one as closely but I see the enum problems have gone away
> > > > at least. I'd still prefer if CMA had an additional helper to discard
> > > > some pages with shrink_page_list() and migrate the remaining pages with
> > > > migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
> > > > migrate mode at all.
> > > 
> > > I am not convinced with your point. What's the benefit on separating
> > > reclaim and migration? For just removing MIGRATE_DISCARD mode?
> > 
> > Maintainability. There are reclaim functions and there are migration
> > functions. Your patch takes migrate_pages() and makes it partially a
> > reclaim function mixing up the responsibilities of migrate.c and vmscan.c.
> > 
> > > I don't think it's not bad because my implementation is very simple(maybe
> > > it's much simpler than separating reclaim and migration) and
> > > could be used by others like memory-hotplug in future.
> > 
> > They could also have used the helper function from CMA that takes a list
> > of pages, reclaims some and migrates other.
> > 
> 
> I also do not accept that your approach is inherently simpler than what I
> proposed to you. This is not tested at all but it should be functionally
> similar to both your patches except that it keeps the responsibility for
> reclaim in vmscan.c
> 
> Your diffstats are
> 
> 8 files changed, 39 insertions(+), 36 deletions(-)
> 3 files changed, 46 insertions(+), 4 deletions(-)
> 
> Mine is
> 
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> Fewer files changed and fewer lines inserted.
> 
> ---8<---
> mm: cma: Discard clean pages during contiguous allocation instead of migration
> 
> This patch drops clean cache pages instead of migration during
> alloc_contig_range() to minimise allocation latency by reducing the amount
> of migration is necessary. It's useful for CMA because latency of migration
> is more important than evicting the background processes working set.
> 
> Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
> ---
>  mm/internal.h   |    1 +
>  mm/page_alloc.c |    2 ++
>  mm/vmscan.c     |   34 +++++++++++++++++++++++++++++-----
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index b8c91b3..6d4bdf9 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -356,3 +356,4 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
>          unsigned long, unsigned long);
>  
>  extern void set_pageblock_order(void);
> +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c66fb87..977bdb2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
>  			break;
>  		}
>  
> +		reclaim_clean_pages_from_list(&cc.migratepages);
> +
>  		ret = migrate_pages(&cc.migratepages,
>  				    __alloc_contig_migrate_alloc,
>  				    0, false, MIGRATE_SYNC);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8d01243..ccf7bc2 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			goto keep;
>  
>  		VM_BUG_ON(PageActive(page));
> -		VM_BUG_ON(page_zone(page) != zone);
> +		VM_BUG_ON(zone && page_zone(page) != zone);
>  
>  		sc->nr_scanned++;
>  
> @@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  				 * except we already have the page isolated
>  				 * and know it's dirty
>  				 */
> -				inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
> +				if (zone)
> +					inc_zone_page_state(page,
> +							NR_VMSCAN_IMMEDIATE);
>  				SetPageReclaim(page);
>  
>  				goto keep_locked;
> @@ -947,7 +949,7 @@ keep:
>  	 * back off and wait for congestion to clear because further reclaim
>  	 * will encounter the same problem
>  	 */
> -	if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
> +	if (zone && nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
>  		zone_set_flag(zone, ZONE_CONGESTED);
>  
>  	free_hot_cold_page_list(&free_pages, 1);
> @@ -955,11 +957,33 @@ keep:
>  	list_splice(&ret_pages, page_list);
>  	count_vm_events(PGACTIVATE, pgactivate);
>  	mem_cgroup_uncharge_end();
> -	*ret_nr_dirty += nr_dirty;
> -	*ret_nr_writeback += nr_writeback;
> +	if (ret_nr_dirty)
> +		*ret_nr_dirty += nr_dirty;
> +	if (ret_nr_writeback)
> +		*ret_nr_writeback += nr_writeback;
>  	return nr_reclaimed;
>  }
>  
> +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
> +{
> +	struct scan_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +		.priority = DEF_PRIORITY,
> +	};
> +	unsigned long ret;
> +	struct page *page, *next;
> +	LIST_HEAD(clean_pages);
> +
> +	list_for_each_entry_safe(page, next, page_list, lru) {
> +		if (page_is_file_cache(page) && !PageDirty(page))
> +			list_move(&page->lru, &clean_pages);
> +	}
> +
> +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
> +	list_splice(&clean_pages, page_list);
> +	return ret;
> +}
> +

It's different with my point.
My intention is to free mapped clean pages as well as not-mapped's one.

How about this?

>From 0f6986e943e55929b4d7b0220a1c24a6bae1a24d Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Fri, 7 Sep 2012 11:20:48 +0900
Subject: [PATCH] mm: cma: Discard clean pages during contiguous allocation
 instead of migration

This patch introudes MIGRATE_DISCARD mode in migration.
It drops *clean cache pages* instead of migration so that
migration latency could be reduced by avoiding (memcpy + page remapping).
It's useful for CMA because latency of migration is very important rather
than eviction of background processes's workingset. In addition, it needs
less free pages for migration targets so it could avoid memory reclaiming
to get free pages, which is another factor increase latency.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/internal.h   |    2 ++
 mm/page_alloc.c |    2 ++
 mm/vmscan.c     |   42 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 3314f79..be09a7e 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -355,3 +355,5 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
         unsigned long, unsigned long);
 
 extern void set_pageblock_order(void);
+unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+				struct list_head *page_list);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ba3100a..bf35e59 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5668,6 +5668,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
 			break;
 		}
 
+		reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
+
 		ret = migrate_pages(&cc.migratepages,
 				    __alloc_contig_migrate_alloc,
 				    0, false, MIGRATE_SYNC);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8d01243..525355e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -674,8 +674,10 @@ static enum page_references page_check_references(struct page *page,
 static unsigned long shrink_page_list(struct list_head *page_list,
 				      struct zone *zone,
 				      struct scan_control *sc,
+				      enum ttu_flags ttu_flags,
 				      unsigned long *ret_nr_dirty,
-				      unsigned long *ret_nr_writeback)
+				      unsigned long *ret_nr_writeback,
+				      bool force_reclaim)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
@@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 
 	mem_cgroup_uncharge_start();
 	while (!list_empty(page_list)) {
-		enum page_references references;
 		struct address_space *mapping;
 		struct page *page;
 		int may_enter_fs;
+		enum page_references references = PAGEREF_RECLAIM;
 
 		cond_resched();
 
@@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			wait_on_page_writeback(page);
 		}
 
-		references = page_check_references(page, sc);
+		if (!force_reclaim)
+			references = page_check_references(page, sc);
+
 		switch (references) {
 		case PAGEREF_ACTIVATE:
 			goto activate_locked;
@@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 * processes. Try to unmap it here.
 		 */
 		if (page_mapped(page) && mapping) {
-			switch (try_to_unmap(page, TTU_UNMAP)) {
+			switch (try_to_unmap(page, ttu_flags)) {
 			case SWAP_FAIL:
 				goto activate_locked;
 			case SWAP_AGAIN:
@@ -960,6 +964,32 @@ keep:
 	return nr_reclaimed;
 }
 
+unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+					struct list_head *page_list)
+{
+	struct scan_control sc = {
+		.gfp_mask = GFP_KERNEL,
+		.priority = DEF_PRIORITY,
+		.may_unmap = 1,
+	};
+	unsigned long ret, dummy1, dummy2;
+	struct page *page, *next;
+	LIST_HEAD(clean_pages);
+
+	list_for_each_entry_safe(page, next, page_list, lru) {
+		if (page_is_file_cache(page) && !PageDirty(page)) {
+			ClearPageActive(page);
+			list_move(&page->lru, &clean_pages);
+		}
+	}
+
+	ret = shrink_page_list(&clean_pages, zone, &sc,
+				TTU_UNMAP|TTU_IGNORE_ACCESS,
+				&dummy1, &dummy2, true);
+	list_splice(&clean_pages, page_list);
+	return ret;
+}
+
 /*
  * Attempt to remove the specified page from its LRU.  Only take this page
  * if it is of the appropriate PageActive status.  Pages which are being
@@ -1278,8 +1308,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	if (nr_taken == 0)
 		return 0;
 
-	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
-						&nr_dirty, &nr_writeback);
+	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
+					&nr_dirty, &nr_writeback, false);
 
 	spin_lock_irq(&zone->lru_lock);
 
-- 
1.7.9.5

-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

On Fri, Sep 07, 2012 at 09:57:12AM +0900, Kyungmin Park wrote:
> Hi Mel,
> 
> After apply your patch, It got the below message.
> Please note that it's not the latest kernel. it's kernel v3.0.31 + CMA
> + your patch.
> It seems it should not be active but it contains active field.

Yeb. At the moment, shrink_page_list shouldn't handle active pages
so we should clear PG_active bit in reclaim_clean_pages_from_list.

-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Kyungmin Park]

Hi Minchan,

I tested Mel patch again with ClearPageActive(page). but after some
testing, it's stall and can't return from
reclaim_clean_pages_from_list(&cc.migratepages).

Maybe it's related with unmap feature from yours?
stall is not happened from your codes until now.

I'll test it more and report any issue if happened.

Thank you,
Kyungmin Park

On 9/7/12, Minchan Kim <minchan@kernel.org> wrote:
> On Thu, Sep 06, 2012 at 10:03:25AM +0100, Mel Gorman wrote:
>> On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
>> > On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
>> > > Hi Mel,
>> > >
>> > > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
>> > > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
>> > > > > This patch introudes MIGRATE_DISCARD mode in migration.
>> > > > > It drops *clean cache pages* instead of migration so that
>> > > > > migration latency could be reduced by avoiding (memcpy + page
>> > > > > remapping).
>> > > > > It's useful for CMA because latency of migration is very important
>> > > > > rather
>> > > > > than eviction of background processes's workingset. In addition,
>> > > > > it needs
>> > > > > less free pages for migration targets so it could avoid memory
>> > > > > reclaiming
>> > > > > to get free pages, which is another factor increase latency.
>> > > > >
>> > > >
>> > > > Bah, this was released while I was reviewing the older version. I
>> > > > did
>> > > > not read this one as closely but I see the enum problems have gone
>> > > > away
>> > > > at least. I'd still prefer if CMA had an additional helper to
>> > > > discard
>> > > > some pages with shrink_page_list() and migrate the remaining pages
>> > > > with
>> > > > migrate_pages(). That would remove the need to add a
>> > > > MIGRATE_DISCARD
>> > > > migrate mode at all.
>> > >
>> > > I am not convinced with your point. What's the benefit on separating
>> > > reclaim and migration? For just removing MIGRATE_DISCARD mode?
>> >
>> > Maintainability. There are reclaim functions and there are migration
>> > functions. Your patch takes migrate_pages() and makes it partially a
>> > reclaim function mixing up the responsibilities of migrate.c and
>> > vmscan.c.
>> >
>> > > I don't think it's not bad because my implementation is very
>> > > simple(maybe
>> > > it's much simpler than separating reclaim and migration) and
>> > > could be used by others like memory-hotplug in future.
>> >
>> > They could also have used the helper function from CMA that takes a
>> > list
>> > of pages, reclaims some and migrates other.
>> >
>>
>> I also do not accept that your approach is inherently simpler than what I
>> proposed to you. This is not tested at all but it should be functionally
>> similar to both your patches except that it keeps the responsibility for
>> reclaim in vmscan.c
>>
>> Your diffstats are
>>
>> 8 files changed, 39 insertions(+), 36 deletions(-)
>> 3 files changed, 46 insertions(+), 4 deletions(-)
>>
>> Mine is
>>
>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>
>> Fewer files changed and fewer lines inserted.
>>
>> ---8<---
>> mm: cma: Discard clean pages during contiguous allocation instead of
>> migration
>>
>> This patch drops clean cache pages instead of migration during
>> alloc_contig_range() to minimise allocation latency by reducing the
>> amount
>> of migration is necessary. It's useful for CMA because latency of
>> migration
>> is more important than evicting the background processes working set.
>>
>> Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
>> ---
>>  mm/internal.h   |    1 +
>>  mm/page_alloc.c |    2 ++
>>  mm/vmscan.c     |   34 +++++++++++++++++++++++++++++-----
>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index b8c91b3..6d4bdf9 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -356,3 +356,4 @@ extern unsigned long vm_mmap_pgoff(struct file *,
>> unsigned long,
>>          unsigned long, unsigned long);
>>
>>  extern void set_pageblock_order(void);
>> +unsigned long reclaim_clean_pages_from_list(struct list_head
>> *page_list);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index c66fb87..977bdb2 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned
>> long start, unsigned long end)
>>  			break;
>>  		}
>>
>> +		reclaim_clean_pages_from_list(&cc.migratepages);
>> +
>>  		ret = migrate_pages(&cc.migratepages,
>>  				    __alloc_contig_migrate_alloc,
>>  				    0, false, MIGRATE_SYNC);
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 8d01243..ccf7bc2 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct list_head
>> *page_list,
>>  			goto keep;
>>
>>  		VM_BUG_ON(PageActive(page));
>> -		VM_BUG_ON(page_zone(page) != zone);
>> +		VM_BUG_ON(zone && page_zone(page) != zone);
>>
>>  		sc->nr_scanned++;
>>
>> @@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct list_head
>> *page_list,
>>  				 * except we already have the page isolated
>>  				 * and know it's dirty
>>  				 */
>> -				inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
>> +				if (zone)
>> +					inc_zone_page_state(page,
>> +							NR_VMSCAN_IMMEDIATE);
>>  				SetPageReclaim(page);
>>
>>  				goto keep_locked;
>> @@ -947,7 +949,7 @@ keep:
>>  	 * back off and wait for congestion to clear because further reclaim
>>  	 * will encounter the same problem
>>  	 */
>> -	if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
>> +	if (zone && nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
>>  		zone_set_flag(zone, ZONE_CONGESTED);
>>
>>  	free_hot_cold_page_list(&free_pages, 1);
>> @@ -955,11 +957,33 @@ keep:
>>  	list_splice(&ret_pages, page_list);
>>  	count_vm_events(PGACTIVATE, pgactivate);
>>  	mem_cgroup_uncharge_end();
>> -	*ret_nr_dirty += nr_dirty;
>> -	*ret_nr_writeback += nr_writeback;
>> +	if (ret_nr_dirty)
>> +		*ret_nr_dirty += nr_dirty;
>> +	if (ret_nr_writeback)
>> +		*ret_nr_writeback += nr_writeback;
>>  	return nr_reclaimed;
>>  }
>>
>> +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
>> +{
>> +	struct scan_control sc = {
>> +		.gfp_mask = GFP_KERNEL,
>> +		.priority = DEF_PRIORITY,
>> +	};
>> +	unsigned long ret;
>> +	struct page *page, *next;
>> +	LIST_HEAD(clean_pages);
>> +
>> +	list_for_each_entry_safe(page, next, page_list, lru) {
>> +		if (page_is_file_cache(page) && !PageDirty(page))
>> +			list_move(&page->lru, &clean_pages);
>> +	}
>> +
>> +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
>> +	list_splice(&clean_pages, page_list);
>> +	return ret;
>> +}
>> +
>
> It's different with my point.
> My intention is to free mapped clean pages as well as not-mapped's one.
>
> How about this?
>
> From 0f6986e943e55929b4d7b0220a1c24a6bae1a24d Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Fri, 7 Sep 2012 11:20:48 +0900
> Subject: [PATCH] mm: cma: Discard clean pages during contiguous allocation
>  instead of migration
>
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Rik van Riel <riel@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/internal.h   |    2 ++
>  mm/page_alloc.c |    2 ++
>  mm/vmscan.c     |   42 ++++++++++++++++++++++++++++++++++++------
>  3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 3314f79..be09a7e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -355,3 +355,5 @@ extern unsigned long vm_mmap_pgoff(struct file *,
> unsigned long,
>          unsigned long, unsigned long);
>
>  extern void set_pageblock_order(void);
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +				struct list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ba3100a..bf35e59 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5668,6 +5668,8 @@ static int __alloc_contig_migrate_range(unsigned long
> start, unsigned long end)
>  			break;
>  		}
>
> +		reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
> +
>  		ret = migrate_pages(&cc.migratepages,
>  				    __alloc_contig_migrate_alloc,
>  				    0, false, MIGRATE_SYNC);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8d01243..525355e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -674,8 +674,10 @@ static enum page_references
> page_check_references(struct page *page,
>  static unsigned long shrink_page_list(struct list_head *page_list,
>  				      struct zone *zone,
>  				      struct scan_control *sc,
> +				      enum ttu_flags ttu_flags,
>  				      unsigned long *ret_nr_dirty,
> -				      unsigned long *ret_nr_writeback)
> +				      unsigned long *ret_nr_writeback,
> +				      bool force_reclaim)
>  {
>  	LIST_HEAD(ret_pages);
>  	LIST_HEAD(free_pages);
> @@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct list_head
> *page_list,
>
>  	mem_cgroup_uncharge_start();
>  	while (!list_empty(page_list)) {
> -		enum page_references references;
>  		struct address_space *mapping;
>  		struct page *page;
>  		int may_enter_fs;
> +		enum page_references references = PAGEREF_RECLAIM;
>
>  		cond_resched();
>
> @@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct list_head
> *page_list,
>  			wait_on_page_writeback(page);
>  		}
>
> -		references = page_check_references(page, sc);
> +		if (!force_reclaim)
> +			references = page_check_references(page, sc);
> +
>  		switch (references) {
>  		case PAGEREF_ACTIVATE:
>  			goto activate_locked;
> @@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct list_head
> *page_list,
>  		 * processes. Try to unmap it here.
>  		 */
>  		if (page_mapped(page) && mapping) {
> -			switch (try_to_unmap(page, TTU_UNMAP)) {
> +			switch (try_to_unmap(page, ttu_flags)) {
>  			case SWAP_FAIL:
>  				goto activate_locked;
>  			case SWAP_AGAIN:
> @@ -960,6 +964,32 @@ keep:
>  	return nr_reclaimed;
>  }
>
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +					struct list_head *page_list)
> +{
> +	struct scan_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +		.priority = DEF_PRIORITY,
> +		.may_unmap = 1,
> +	};
> +	unsigned long ret, dummy1, dummy2;
> +	struct page *page, *next;
> +	LIST_HEAD(clean_pages);
> +
> +	list_for_each_entry_safe(page, next, page_list, lru) {
> +		if (page_is_file_cache(page) && !PageDirty(page)) {
> +			ClearPageActive(page);
> +			list_move(&page->lru, &clean_pages);
> +		}
> +	}
> +
> +	ret = shrink_page_list(&clean_pages, zone, &sc,
> +				TTU_UNMAP|TTU_IGNORE_ACCESS,
> +				&dummy1, &dummy2, true);
> +	list_splice(&clean_pages, page_list);
> +	return ret;
> +}
> +
>  /*
>   * Attempt to remove the specified page from its LRU.  Only take this page
>   * if it is of the appropriate PageActive status.  Pages which are being
> @@ -1278,8 +1308,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct
> lruvec *lruvec,
>  	if (nr_taken == 0)
>  		return 0;
>
> -	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
> -						&nr_dirty, &nr_writeback);
> +	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
> +					&nr_dirty, &nr_writeback, false);
>
>  	spin_lock_irq(&zone->lru_lock);
>
> --
> 1.7.9.5
>
> --
> Kind regards,
> Minchan Kim
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Kyungmin Park]

On 9/7/12, Kyungmin Park <kmpark@infradead.org> wrote:
> Hi Minchan,
>
> I tested Mel patch again with ClearPageActive(page). but after some
> testing, it's stall and can't return from
> reclaim_clean_pages_from_list(&cc.migratepages).
>
> Maybe it's related with unmap feature from yours?
> stall is not happened from your codes until now.
>
> I'll test it more and report any issue if happened.
Updated. it's hang also. there are other issues.
>
> Thank you,
> Kyungmin Park
>
> On 9/7/12, Minchan Kim <minchan@kernel.org> wrote:
>> On Thu, Sep 06, 2012 at 10:03:25AM +0100, Mel Gorman wrote:
>>> On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
>>> > On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
>>> > > Hi Mel,
>>> > >
>>> > > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
>>> > > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
>>> > > > > This patch introudes MIGRATE_DISCARD mode in migration.
>>> > > > > It drops *clean cache pages* instead of migration so that
>>> > > > > migration latency could be reduced by avoiding (memcpy + page
>>> > > > > remapping).
>>> > > > > It's useful for CMA because latency of migration is very
>>> > > > > important
>>> > > > > rather
>>> > > > > than eviction of background processes's workingset. In addition,
>>> > > > > it needs
>>> > > > > less free pages for migration targets so it could avoid memory
>>> > > > > reclaiming
>>> > > > > to get free pages, which is another factor increase latency.
>>> > > > >
>>> > > >
>>> > > > Bah, this was released while I was reviewing the older version. I
>>> > > > did
>>> > > > not read this one as closely but I see the enum problems have gone
>>> > > > away
>>> > > > at least. I'd still prefer if CMA had an additional helper to
>>> > > > discard
>>> > > > some pages with shrink_page_list() and migrate the remaining pages
>>> > > > with
>>> > > > migrate_pages(). That would remove the need to add a
>>> > > > MIGRATE_DISCARD
>>> > > > migrate mode at all.
>>> > >
>>> > > I am not convinced with your point. What's the benefit on separating
>>> > > reclaim and migration? For just removing MIGRATE_DISCARD mode?
>>> >
>>> > Maintainability. There are reclaim functions and there are migration
>>> > functions. Your patch takes migrate_pages() and makes it partially a
>>> > reclaim function mixing up the responsibilities of migrate.c and
>>> > vmscan.c.
>>> >
>>> > > I don't think it's not bad because my implementation is very
>>> > > simple(maybe
>>> > > it's much simpler than separating reclaim and migration) and
>>> > > could be used by others like memory-hotplug in future.
>>> >
>>> > They could also have used the helper function from CMA that takes a
>>> > list
>>> > of pages, reclaims some and migrates other.
>>> >
>>>
>>> I also do not accept that your approach is inherently simpler than what
>>> I
>>> proposed to you. This is not tested at all but it should be functionally
>>> similar to both your patches except that it keeps the responsibility for
>>> reclaim in vmscan.c
>>>
>>> Your diffstats are
>>>
>>> 8 files changed, 39 insertions(+), 36 deletions(-)
>>> 3 files changed, 46 insertions(+), 4 deletions(-)
>>>
>>> Mine is
>>>
>>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>>
>>> Fewer files changed and fewer lines inserted.
>>>
>>> ---8<---
>>> mm: cma: Discard clean pages during contiguous allocation instead of
>>> migration
>>>
>>> This patch drops clean cache pages instead of migration during
>>> alloc_contig_range() to minimise allocation latency by reducing the
>>> amount
>>> of migration is necessary. It's useful for CMA because latency of
>>> migration
>>> is more important than evicting the background processes working set.
>>>
>>> Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
>>> ---
>>>  mm/internal.h   |    1 +
>>>  mm/page_alloc.c |    2 ++
>>>  mm/vmscan.c     |   34 +++++++++++++++++++++++++++++-----
>>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index b8c91b3..6d4bdf9 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -356,3 +356,4 @@ extern unsigned long vm_mmap_pgoff(struct file *,
>>> unsigned long,
>>>          unsigned long, unsigned long);
>>>
>>>  extern void set_pageblock_order(void);
>>> +unsigned long reclaim_clean_pages_from_list(struct list_head
>>> *page_list);
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index c66fb87..977bdb2 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned
>>> long start, unsigned long end)
>>>  			break;
>>>  		}
>>>
>>> +		reclaim_clean_pages_from_list(&cc.migratepages);
>>> +
>>>  		ret = migrate_pages(&cc.migratepages,
>>>  				    __alloc_contig_migrate_alloc,
>>>  				    0, false, MIGRATE_SYNC);
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 8d01243..ccf7bc2 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct
>>> list_head
>>> *page_list,
>>>  			goto keep;
>>>
>>>  		VM_BUG_ON(PageActive(page));
>>> -		VM_BUG_ON(page_zone(page) != zone);
>>> +		VM_BUG_ON(zone && page_zone(page) != zone);
>>>
>>>  		sc->nr_scanned++;
>>>
>>> @@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct
>>> list_head
>>> *page_list,
>>>  				 * except we already have the page isolated
>>>  				 * and know it's dirty
>>>  				 */
>>> -				inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
>>> +				if (zone)
>>> +					inc_zone_page_state(page,
>>> +							NR_VMSCAN_IMMEDIATE);
>>>  				SetPageReclaim(page);
>>>
>>>  				goto keep_locked;
>>> @@ -947,7 +949,7 @@ keep:
>>>  	 * back off and wait for congestion to clear because further reclaim
>>>  	 * will encounter the same problem
>>>  	 */
>>> -	if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
>>> +	if (zone && nr_dirty && nr_dirty == nr_congested &&
>>> global_reclaim(sc))
>>>  		zone_set_flag(zone, ZONE_CONGESTED);
>>>
>>>  	free_hot_cold_page_list(&free_pages, 1);
>>> @@ -955,11 +957,33 @@ keep:
>>>  	list_splice(&ret_pages, page_list);
>>>  	count_vm_events(PGACTIVATE, pgactivate);
>>>  	mem_cgroup_uncharge_end();
>>> -	*ret_nr_dirty += nr_dirty;
>>> -	*ret_nr_writeback += nr_writeback;
>>> +	if (ret_nr_dirty)
>>> +		*ret_nr_dirty += nr_dirty;
>>> +	if (ret_nr_writeback)
>>> +		*ret_nr_writeback += nr_writeback;
>>>  	return nr_reclaimed;
>>>  }
>>>
>>> +unsigned long reclaim_clean_pages_from_list(struct list_head
>>> *page_list)
>>> +{
>>> +	struct scan_control sc = {
>>> +		.gfp_mask = GFP_KERNEL,
>>> +		.priority = DEF_PRIORITY,
>>> +	};
>>> +	unsigned long ret;
>>> +	struct page *page, *next;
>>> +	LIST_HEAD(clean_pages);
>>> +
>>> +	list_for_each_entry_safe(page, next, page_list, lru) {
>>> +		if (page_is_file_cache(page) && !PageDirty(page))
>>> +			list_move(&page->lru, &clean_pages);
>>> +	}
>>> +
>>> +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
>>> +	list_splice(&clean_pages, page_list);
>>> +	return ret;
>>> +}
>>> +
>>
>> It's different with my point.
>> My intention is to free mapped clean pages as well as not-mapped's one.
>>
>> How about this?
>>
>> From 0f6986e943e55929b4d7b0220a1c24a6bae1a24d Mon Sep 17 00:00:00 2001
>> From: Minchan Kim <minchan@kernel.org>
>> Date: Fri, 7 Sep 2012 11:20:48 +0900
>> Subject: [PATCH] mm: cma: Discard clean pages during contiguous
>> allocation
>>  instead of migration
>>
>> This patch introudes MIGRATE_DISCARD mode in migration.
>> It drops *clean cache pages* instead of migration so that
>> migration latency could be reduced by avoiding (memcpy + page remapping).
>> It's useful for CMA because latency of migration is very important rather
>> than eviction of background processes's workingset. In addition, it needs
>> less free pages for migration targets so it could avoid memory reclaiming
>> to get free pages, which is another factor increase latency.
>>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Michal Nazarewicz <mina86@mina86.com>
>> Cc: Rik van Riel <riel@redhat.com>
>> Signed-off-by: Mel Gorman <mgorman@suse.de>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>> ---
>>  mm/internal.h   |    2 ++
>>  mm/page_alloc.c |    2 ++
>>  mm/vmscan.c     |   42 ++++++++++++++++++++++++++++++++++++------
>>  3 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 3314f79..be09a7e 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -355,3 +355,5 @@ extern unsigned long vm_mmap_pgoff(struct file *,
>> unsigned long,
>>          unsigned long, unsigned long);
>>
>>  extern void set_pageblock_order(void);
>> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
>> +				struct list_head *page_list);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index ba3100a..bf35e59 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5668,6 +5668,8 @@ static int __alloc_contig_migrate_range(unsigned
>> long
>> start, unsigned long end)
>>  			break;
>>  		}
>>
>> +		reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
>> +
>>  		ret = migrate_pages(&cc.migratepages,
>>  				    __alloc_contig_migrate_alloc,
>>  				    0, false, MIGRATE_SYNC);
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 8d01243..525355e 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -674,8 +674,10 @@ static enum page_references
>> page_check_references(struct page *page,
>>  static unsigned long shrink_page_list(struct list_head *page_list,
>>  				      struct zone *zone,
>>  				      struct scan_control *sc,
>> +				      enum ttu_flags ttu_flags,
>>  				      unsigned long *ret_nr_dirty,
>> -				      unsigned long *ret_nr_writeback)
>> +				      unsigned long *ret_nr_writeback,
>> +				      bool force_reclaim)
>>  {
>>  	LIST_HEAD(ret_pages);
>>  	LIST_HEAD(free_pages);
>> @@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct
>> list_head
>> *page_list,
>>
>>  	mem_cgroup_uncharge_start();
>>  	while (!list_empty(page_list)) {
>> -		enum page_references references;
>>  		struct address_space *mapping;
>>  		struct page *page;
>>  		int may_enter_fs;
>> +		enum page_references references = PAGEREF_RECLAIM;
>>
>>  		cond_resched();
>>
>> @@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct
>> list_head
>> *page_list,
>>  			wait_on_page_writeback(page);
>>  		}
>>
>> -		references = page_check_references(page, sc);
>> +		if (!force_reclaim)
>> +			references = page_check_references(page, sc);
>> +
>>  		switch (references) {
>>  		case PAGEREF_ACTIVATE:
>>  			goto activate_locked;
>> @@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct
>> list_head
>> *page_list,
>>  		 * processes. Try to unmap it here.
>>  		 */
>>  		if (page_mapped(page) && mapping) {
>> -			switch (try_to_unmap(page, TTU_UNMAP)) {
>> +			switch (try_to_unmap(page, ttu_flags)) {
>>  			case SWAP_FAIL:
>>  				goto activate_locked;
>>  			case SWAP_AGAIN:
>> @@ -960,6 +964,32 @@ keep:
>>  	return nr_reclaimed;
>>  }
>>
>> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
>> +					struct list_head *page_list)
>> +{
>> +	struct scan_control sc = {
>> +		.gfp_mask = GFP_KERNEL,
>> +		.priority = DEF_PRIORITY,
>> +		.may_unmap = 1,
>> +	};
>> +	unsigned long ret, dummy1, dummy2;
>> +	struct page *page, *next;
>> +	LIST_HEAD(clean_pages);
>> +
>> +	list_for_each_entry_safe(page, next, page_list, lru) {
>> +		if (page_is_file_cache(page) && !PageDirty(page)) {
>> +			ClearPageActive(page);
>> +			list_move(&page->lru, &clean_pages);
>> +		}
>> +	}
>> +
>> +	ret = shrink_page_list(&clean_pages, zone, &sc,
>> +				TTU_UNMAP|TTU_IGNORE_ACCESS,
>> +				&dummy1, &dummy2, true);
>> +	list_splice(&clean_pages, page_list);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * Attempt to remove the specified page from its LRU.  Only take this
>> page
>>   * if it is of the appropriate PageActive status.  Pages which are being
>> @@ -1278,8 +1308,8 @@ shrink_inactive_list(unsigned long nr_to_scan,
>> struct
>> lruvec *lruvec,
>>  	if (nr_taken == 0)
>>  		return 0;
>>
>> -	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
>> -						&nr_dirty, &nr_writeback);
>> +	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
>> +					&nr_dirty, &nr_writeback, false);
>>
>>  	spin_lock_irq(&zone->lru_lock);
>>
>> --
>> 1.7.9.5
>>
>> --
>> Kind regards,
>> Minchan Kim
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Fri, Sep 07, 2012 at 11:26:01AM +0900, Minchan Kim wrote:
> On Fri, Sep 07, 2012 at 09:57:12AM +0900, Kyungmin Park wrote:
> > Hi Mel,
> > 
> > After apply your patch, It got the below message.
> > Please note that it's not the latest kernel. it's kernel v3.0.31 + CMA
> > + your patch.
> > It seems it should not be active but it contains active field.
> 
> Yeb. At the moment, shrink_page_list shouldn't handle active pages
> so we should clear PG_active bit in reclaim_clean_pages_from_list.
> 

Yep, that was an obvious thing I missed. It was a very fast prototype to
illustrate my point. It should be possible to fix.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

Hi Kyungmin,

On Fri, Sep 07, 2012 at 04:31:17PM +0900, Kyungmin Park wrote:
> On 9/7/12, Kyungmin Park <kmpark@infradead.org> wrote:
> > Hi Minchan,
> >
> > I tested Mel patch again with ClearPageActive(page). but after some
> > testing, it's stall and can't return from
> > reclaim_clean_pages_from_list(&cc.migratepages).
> >
> > Maybe it's related with unmap feature from yours?
> > stall is not happened from your codes until now.
> >
> > I'll test it more and report any issue if happened.
> Updated. it's hang also. there are other issues.

It was silly mistake in my patch and I suspect it fixes your issue
because I guess you already tried below patch when you compiled and saw
warning message.
Anyway, if you see hang still after applying below patch,
please enable CONFIG_DEBUG_VM and retest, if you find something, report it.
I hope CONFIG_DEBUG_VM catch something.

Thanks.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6668115..51d3f66 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5705,7 +5705,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned
                        break;
                }
 
-               reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
+               reclaim_clean_pages_from_list(cc.zone, &cc.migratepages);
 
                ret = migrate_pages(&cc.migratepages,
                                    __alloc_contig_migrate_alloc,
(END)


-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Fri, Sep 07, 2012 at 11:24:34AM +0900, Minchan Kim wrote:
> > +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
> > +{
> > +	struct scan_control sc = {
> > +		.gfp_mask = GFP_KERNEL,
> > +		.priority = DEF_PRIORITY,
> > +	};
> > +	unsigned long ret;
> > +	struct page *page, *next;
> > +	LIST_HEAD(clean_pages);
> > +
> > +	list_for_each_entry_safe(page, next, page_list, lru) {
> > +		if (page_is_file_cache(page) && !PageDirty(page))
> > +			list_move(&page->lru, &clean_pages);
> > +	}
> > +
> > +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
> > +	list_splice(&clean_pages, page_list);
> > +	return ret;
> > +}
> > +
> 
> It's different with my point.
> My intention is to free mapped clean pages as well as not-mapped's one.
> 

Then just set may_unmap == 1?

struct scan_control sc = {
	.gfp_mask = GFP_KERNEL,
	.may_unmap = 1,
	.priority = DEF_PRIORITY
};

> How about this?
> 
> From 0f6986e943e55929b4d7b0220a1c24a6bae1a24d Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Fri, 7 Sep 2012 11:20:48 +0900
> Subject: [PATCH] mm: cma: Discard clean pages during contiguous allocation
>  instead of migration
> 
> This patch introudes MIGRATE_DISCARD mode in migration.

This line is now redundant.

> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs

processes working set

> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
> 
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Rik van Riel <riel@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/internal.h   |    2 ++
>  mm/page_alloc.c |    2 ++
>  mm/vmscan.c     |   42 ++++++++++++++++++++++++++++++++++++------
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 3314f79..be09a7e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -355,3 +355,5 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
>          unsigned long, unsigned long);
>  
>  extern void set_pageblock_order(void);
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +				struct list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ba3100a..bf35e59 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5668,6 +5668,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
>  			break;
>  		}
>  
> +		reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
> +

The signature is zone, list_head but you pass in list_head, zone. I
expect that generates fun compile warnings and crashes at runtime.

The reason that I did not pass in zone to reclaim_clean_pages_from_list()
in my version is because I did not want to force the list to all be from the
same zone. That will just happen to be true but shrink_page_list() really
does not care about the zone from the perspective of this new function. It's
used for a debugging check (disable it if !zone) and setting ZONE_CONGESTED
which will never be important as far as reclaim_clean_pages_from_list()
is concerned. It will never call pageout().

It's up to you if you really want to pass zone in but I would not bother
if I was you :)

>  		ret = migrate_pages(&cc.migratepages,
>  				    __alloc_contig_migrate_alloc,
>  				    0, false, MIGRATE_SYNC);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8d01243..525355e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -674,8 +674,10 @@ static enum page_references page_check_references(struct page *page,
>  static unsigned long shrink_page_list(struct list_head *page_list,
>  				      struct zone *zone,
>  				      struct scan_control *sc,
> +				      enum ttu_flags ttu_flags,
>  				      unsigned long *ret_nr_dirty,
> -				      unsigned long *ret_nr_writeback)
> +				      unsigned long *ret_nr_writeback,
> +				      bool force_reclaim)
>  {
>  	LIST_HEAD(ret_pages);
>  	LIST_HEAD(free_pages);
> @@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  
>  	mem_cgroup_uncharge_start();
>  	while (!list_empty(page_list)) {
> -		enum page_references references;
>  		struct address_space *mapping;
>  		struct page *page;
>  		int may_enter_fs;
> +		enum page_references references = PAGEREF_RECLAIM;
>  
>  		cond_resched();
>  
> @@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			wait_on_page_writeback(page);
>  		}
>  
> -		references = page_check_references(page, sc);
> +		if (!force_reclaim)
> +			references = page_check_references(page, sc);
> +
>  		switch (references) {
>  		case PAGEREF_ACTIVATE:
>  			goto activate_locked;

Good point.

> @@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 * processes. Try to unmap it here.
>  		 */
>  		if (page_mapped(page) && mapping) {
> -			switch (try_to_unmap(page, TTU_UNMAP)) {
> +			switch (try_to_unmap(page, ttu_flags)) {
>  			case SWAP_FAIL:
>  				goto activate_locked;
>  			case SWAP_AGAIN:

Another good point.

> @@ -960,6 +964,32 @@ keep:
>  	return nr_reclaimed;
>  }
>  
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +					struct list_head *page_list)
> +{
> +	struct scan_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +		.priority = DEF_PRIORITY,
> +		.may_unmap = 1,
> +	};

Yep.

> +	unsigned long ret, dummy1, dummy2;
> +	struct page *page, *next;
> +	LIST_HEAD(clean_pages);
> +
> +	list_for_each_entry_safe(page, next, page_list, lru) {
> +		if (page_is_file_cache(page) && !PageDirty(page)) {
> +			ClearPageActive(page);

Yep.

> +			list_move(&page->lru, &clean_pages);
> +		}
> +	}
> +
> +	ret = shrink_page_list(&clean_pages, zone, &sc,
> +				TTU_UNMAP|TTU_IGNORE_ACCESS,
> +				&dummy1, &dummy2, true);
> +	list_splice(&clean_pages, page_list);
> +	return ret;
> +}
> +
>  /*
>   * Attempt to remove the specified page from its LRU.  Only take this page
>   * if it is of the appropriate PageActive status.  Pages which are being
> @@ -1278,8 +1308,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	if (nr_taken == 0)
>  		return 0;
>  
> -	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
> -						&nr_dirty, &nr_writeback);
> +	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
> +					&nr_dirty, &nr_writeback, false);
>  
>  	spin_lock_irq(&zone->lru_lock);
>  

So other than the mix up of order parameters I think this should work.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Kyungmin Park]

On 9/7/12, Minchan Kim <minchan@kernel.org> wrote:
> Hi Kyungmin,
>
> On Fri, Sep 07, 2012 at 04:31:17PM +0900, Kyungmin Park wrote:
>> On 9/7/12, Kyungmin Park <kmpark@infradead.org> wrote:
>> > Hi Minchan,
>> >
>> > I tested Mel patch again with ClearPageActive(page). but after some
>> > testing, it's stall and can't return from
>> > reclaim_clean_pages_from_list(&cc.migratepages).
>> >
>> > Maybe it's related with unmap feature from yours?
>> > stall is not happened from your codes until now.
>> >
>> > I'll test it more and report any issue if happened.
>> Updated. it's hang also. there are other issues.
>
> It was silly mistake in my patch and I suspect it fixes your issue
> because I guess you already tried below patch when you compiled and saw
> warning message.
> Anyway, if you see hang still after applying below patch,
> please enable CONFIG_DEBUG_VM and retest, if you find something, report it.
> I hope CONFIG_DEBUG_VM catch something.
>
> Thanks.
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6668115..51d3f66 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5705,7 +5705,7 @@ static int __alloc_contig_migrate_range(unsigned long
> start, unsigned
>                         break;
>                 }
>
> -               reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
> +               reclaim_clean_pages_from_list(cc.zone, &cc.migratepages);
Of course, I tested it after local fix. and got the results as above.

Thank you,
Kyungmin Park
>                 ret = migrate_pages(&cc.migratepages,
>                                     __alloc_contig_migrate_alloc,
> (END)
>
>
> --
> Kind regards,
> Minchan Kim
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Fri, Sep 07, 2012 at 09:21:45AM +0100, Mel Gorman wrote:
> 
> So other than the mix up of order parameters I think this should work.
> 

But I'd be wrong, isolated page accounting is not fixed up so it will
eventually hang on too_many_isolated. It turns out it is necessary
to pass in zone after all. The following patch passed a high order
allocation stress test. To actually exercise the path I had compaction
call reclaim_clean_pages_from_list() in a separate debugging patch.

Minchan, can you test your CMA allocation latency test with this patch?
If the figures are satisfactory could you add them to the changelog and
consider replacing the MIGRATE_DISCARD pair of patches with this version
please?

---8<---
From: Minchan Kim <minchan@kernel.org>
Subject: [PATCH] mm: cma: Discard clean pages during contiguous allocation
 instead of migration

This patch drops clean cache pages instead of migration during
alloc_contig_range() to minimise allocation latency by reducing the amount
of migration is necessary. It's useful for CMA because latency of migration
is more important than evicting the background processes working set.
In addition, as pages are reclaimed then fewer free pages for migration
targets are required so it avoids memory reclaiming to get free pages,
which is a contributory factor to increased latency.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/internal.h   |    2 ++
 mm/page_alloc.c |    2 ++
 mm/vmscan.c     |   43 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index b8c91b3..ec2f304 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -356,3 +356,5 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
         unsigned long, unsigned long);
 
 extern void set_pageblock_order(void);
+unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+					    struct list_head *page_list);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c66fb87..977bdb2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
 			break;
 		}
 
+		reclaim_clean_pages_from_list(&cc.migratepages);
+
 		ret = migrate_pages(&cc.migratepages,
 				    __alloc_contig_migrate_alloc,
 				    0, false, MIGRATE_SYNC);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8d01243..795d963 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -674,8 +674,10 @@ static enum page_references page_check_references(struct page *page,
 static unsigned long shrink_page_list(struct list_head *page_list,
 				      struct zone *zone,
 				      struct scan_control *sc,
+				      enum ttu_flags ttu_flags,
 				      unsigned long *ret_nr_dirty,
-				      unsigned long *ret_nr_writeback)
+				      unsigned long *ret_nr_writeback,
+				      bool force_reclaim)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
@@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 
 	mem_cgroup_uncharge_start();
 	while (!list_empty(page_list)) {
-		enum page_references references;
 		struct address_space *mapping;
 		struct page *page;
 		int may_enter_fs;
+		enum page_references references = PAGEREF_RECLAIM;
 
 		cond_resched();
 
@@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			wait_on_page_writeback(page);
 		}
 
-		references = page_check_references(page, sc);
+		if (!force_reclaim)
+			references = page_check_references(page, sc);
+
 		switch (references) {
 		case PAGEREF_ACTIVATE:
 			goto activate_locked;
@@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 * processes. Try to unmap it here.
 		 */
 		if (page_mapped(page) && mapping) {
-			switch (try_to_unmap(page, TTU_UNMAP)) {
+			switch (try_to_unmap(page, ttu_flags)) {
 			case SWAP_FAIL:
 				goto activate_locked;
 			case SWAP_AGAIN:
@@ -960,6 +964,33 @@ keep:
 	return nr_reclaimed;
 }
 
+unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+					    struct list_head *page_list)
+{
+	struct scan_control sc = {
+		.gfp_mask = GFP_KERNEL,
+		.priority = DEF_PRIORITY,
+		.may_unmap = 1,
+	};
+	unsigned long ret, dummy1, dummy2;
+	struct page *page, *next;
+	LIST_HEAD(clean_pages);
+
+	list_for_each_entry_safe(page, next, page_list, lru) {
+		if (page_is_file_cache(page) && !PageDirty(page)) {
+			ClearPageActive(page);
+			list_move(&page->lru, &clean_pages);
+		}
+	}
+
+	ret = shrink_page_list(&clean_pages, NULL, &sc,
+				TTU_UNMAP|TTU_IGNORE_ACCESS,
+				&dummy1, &dummy2, true);
+	list_splice(&clean_pages, page_list);
+	__mod_zone_page_state(zone, NR_ISOLATED_FILE, -ret);
+	return ret;
+}
+
 /*
  * Attempt to remove the specified page from its LRU.  Only take this page
  * if it is of the appropriate PageActive status.  Pages which are being
@@ -1278,8 +1309,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	if (nr_taken == 0)
 		return 0;
 
-	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
-						&nr_dirty, &nr_writeback);
+	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
+					&nr_dirty, &nr_writeback, false);
 
 	spin_lock_irq(&zone->lru_lock);
 

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Minchan Kim]

On Fri, Sep 07, 2012 at 10:32:03AM +0100, Mel Gorman wrote:
> On Fri, Sep 07, 2012 at 09:21:45AM +0100, Mel Gorman wrote:
> > 
> > So other than the mix up of order parameters I think this should work.
> > 
> 
> But I'd be wrong, isolated page accounting is not fixed up so it will
> eventually hang on too_many_isolated. It turns out it is necessary

Good spot.

> to pass in zone after all. The following patch passed a high order
> allocation stress test. To actually exercise the path I had compaction
> call reclaim_clean_pages_from_list() in a separate debugging patch.
> 
> Minchan, can you test your CMA allocation latency test with this patch?
> If the figures are satisfactory could you add them to the changelog and
> consider replacing the MIGRATE_DISCARD pair of patches with this version
> please?

Of course, I will do next week.
Thanks!

--
Kind Regards,
Minchan Kim

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[lihanhui]

在 2012-9-5，下午4:11，Minchan Kim <minchan@kernel.org> 写道：

> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
> 
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> include/linux/migrate_mode.h |    7 +++++++
> mm/migrate.c                 |   41 ++++++++++++++++++++++++++++++++++++++---
> mm/page_alloc.c              |    2 +-
> 3 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
> index 8848cad..4eb1646 100644
> --- a/include/linux/migrate_mode.h
> +++ b/include/linux/migrate_mode.h
> @@ -14,6 +14,13 @@
>  */
> #define MIGRATE_SYNC		((__force migrate_mode_t)0x4)
> 
> +/*
> + * MIGRTATE_DISCARD will discard clean cache page instead of migration.
> + * MIGRATE_ASYNC, MIGRATE_SYNC_LIGHT, MIGRATE_SYNC shouldn't be used
> + * together with OR flag in current implementation.
> + */
> +#define MIGRATE_DISCARD		((__force migrate_mode_t)0x8)
> +
> typedef unsigned __bitwise__ migrate_mode_t;
> 
> #endif		/* MIGRATE_MODE_H_INCLUDED */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 28d464b..2de7709 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -678,6 +678,19 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> 	return rc;
> }
> 
> +static int discard_page(struct page *page)
> +{
> +	int ret = -EAGAIN;
> +
> +	struct address_space *mapping = page_mapping(page);
> +	if (page_has_private(page))
> +		if (!try_to_release_page(page, GFP_KERNEL))
> +			return ret;
> +	if (remove_mapping(mapping, page))
> +		ret = 0;
> +	return ret;
> +}
> +
> static int __unmap_and_move(struct page *page, struct page *newpage,
> 			int force, bool offlining, migrate_mode_t mode)
> {
> @@ -685,6 +698,9 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> 	int remap_swapcache = 1;
> 	struct mem_cgroup *mem;
> 	struct anon_vma *anon_vma = NULL;
> +	enum ttu_flags ttu_flags;
> +	bool discard_mode = false;
> +	bool file = false;
> 
> 	if (!trylock_page(page)) {
> 		if (!force || (mode & MIGRATE_ASYNC))
> @@ -799,12 +815,31 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> 		goto skip_unmap;
> 	}
> 
> +	file = page_is_file_cache(page);
> +	ttu_flags = TTU_IGNORE_ACCESS;
> +retry:
> +	if (!(mode & MIGRATE_DISCARD) || !file || PageDirty(page))
> +		ttu_flags |= (TTU_MIGRATION | TTU_IGNORE_MLOCK);
> +	else
> +		discard_mode = true;
> +
> 	/* Establish migration ptes or remove ptes */
> -	try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> +	rc = try_to_unmap(page, ttu_flags);
> 
> skip_unmap:
> -	if (!page_mapped(page))
> -		rc = move_to_new_page(newpage, page, remap_swapcache, mode);
> +	if (rc == SWAP_SUCCESS) {
> +		if (!discard_mode) {
> +			rc = move_to_new_page(newpage, page,
> +					remap_swapcache, mode);
> +		} else {
> +			rc = discard_page(page);
> +			goto uncharge;
> +		}
> +	} else if (rc == SWAP_MLOCK && discard_mode) {
> +		mode &= ~MIGRATE_DISCARD;
> +		discard_mode = false;
> +		goto retry;
> +	}
> 
> 	if (rc && remap_swapcache)
> 		remove_migration_ptes(page, page);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ba3100a..e14b960 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5670,7 +5670,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> 
> 		ret = migrate_pages(&cc.migratepages,
> 				    __alloc_contig_migrate_alloc,
> -				    0, false, MIGRATE_SYNC);
> +				    0, false, MIGRATE_SYNC|MIGRATE_DISCARD);
> 	}
> 
> 	putback_lru_pages(&cc.migratepages);
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/