f/madivse(DONTNEED) support

Recently, there are reported problem about thrashing.
(http://marc.info/?l=rsync&m=128885034930933&w=2)
It happens by backup workloads(ex, nightly rsync). That's because the workload makes just use-once pages and touches pages twice. It promotes the page into active list so that it results in working set page eviction.

Some app developer want to support POSIX_FADV_NOREUSE. But other OSes don't support it, either.
(http://marc.info/?l=linux-mm&m=128928979512086&w=2)

By other approach, app developers use POSIX_FADV_DONTNEED. But it has a problem. If kernel meets page is writing during invalidate_mapping_pages, it can't work.
It is very hard for application programmer to use it. Because they always have to sync data before calling fadivse(..POSIX_FADV_DONTNEED) to make sure the pages could be discardable. At last, they can't use deferred write of kernel so that they could see performance loss.
(http://insights.oetiker.ch/linux/fadvise.html)

In fact, invalidation is very big hint to reclaimer. It means we don't use the page any more. So let's move the writing page into inactive list's head.

Why I need the page to head, Dirty/Writeback page would be flushed sooner or later. It can prevent writeout of pageout which is less effective than flusher's writeout.

Originally, I reused lru_demote of Peter with some change so added his Signed-off-by.

Reported-by Ben Gamari <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Recently, there are reported problem about thrashing.
(http://marc.info/?l=rsync&m=128885034930933&w=2)
It happens by backup workloads(ex, nightly rsync). That's because the workload makes just use-once pages and touches pages twice. It promotes the page into active list so that it results in working set page eviction.

Some app developer want to support POSIX_FADV_NOREUSE. But other OSes don't support it, either.
(http://marc.info/?l=linux-mm&m=128928979512086&w=2)

By other approach, app developers use POSIX_FADV_DONTNEED. But it has a problem. If kernel meets page is writing during invalidate_mapping_pages, it can't work.
It is very hard for application programmer to use it. Because they always have to sync data before calling fadivse(..POSIX_FADV_DONTNEED) to make sure the pages could be discardable. At last, they can't use deferred write of kernel so that they could see performance loss.
(http://insights.oetiker.ch/linux/fadvise.html)

In fact, invalidation is very big hint to reclaimer. It means we don't use the page any more. So let's move the writing page into inactive list's head.

Why I need the page to head, Dirty/Writeback page would be flushed sooner or later. It can prevent writeout of pageout which is less effective than flusher's writeout.

Originally, I reused lru_demote of Peter with some change so added his Signed-off-by.

Reported-by Ben Gamari <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Adnrew. Before applying this series, please drop below two patches.
mm-deactivate-invalidated-pages.patch mm-deactivate-invalidated-pages-fix.patch

Changelog since v2:
- mapped page leaves alone - suggested by Mel
- pass part related PG_reclaim in next patch.

Changelog since v1:
- modify description
- correct typo
- add some comment

---
 include/linux/swap.h |    1 +
 mm/swap.c            |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/truncate.c        |   16 +++++++--
 3 files changed, 93 insertions(+), 4 deletions(-)

include/linux/swap.h +0 -0

index eba53e7..84375e4 100644

include/linux/swap.h +14 -0

+{
+	int lru, file;
+	unsigned long vm_flags;
+
+	if (!PageLRU(page) || !PageActive(page))
+		return;
+
+	/* Some processes are using the page */
+	if (page_mapped(page))
+		return;
+
+	file = page_is_file_cache(page);
+	lru = page_lru_base_type(page);
+	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);

include/linux/swap.h +24 -0

+{
+	int i;
+	struct zone *zone = NULL;
+
+	for (i = 0; i < pagevec_count(pvec); i++) {
+		struct page *page = pvec->pages[i];
+		struct zone *pagezone = page_zone(page);
+
+		if (pagezone != zone) {
+			if (zone)
+				spin_unlock_irq(&zone->lru_lock);
+			zone = pagezone;
+			spin_lock_irq(&zone->lru_lock);
+		}
+		__lru_deactivate(page, zone);
+	}
+	if (zone)
+		spin_unlock_irq(&zone->lru_lock);
+
+	release_pages(pvec->pages, pvec->nr, pvec->cold);
+	pagevec_reinit(pvec);
+}
+
+/*
  * Drain pages out of the cpu's pagevecs.
  * Either "cpu" is the current CPU, and preemption has already been
  * disabled; or "cpu" is being hot-unplugged, and is already dead.

@@ -292,6 +352,26 @@ static void drain_cpu_pagevecs(int cpu)
 		pagevec_move_tail(pvec);
 		local_irq_restore(flags);
 	}

include/linux/swap.h +7 -0

+	if (likely(get_page_unless_zero(page))) {
+		struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
+
+		if (!pagevec_add(pvec, page))
+			__pagevec_lru_deactivate(pvec);
+		put_cpu_var(lru_deactivate_pvecs);
+	}
 }
 
 void lru_add_drain(void)

Quoted 64 lines · Minchan Kim

Recently, there are reported problem about thrashing.
(http://marc.info/?l=rsync&m=128885034930933&w=2)
It happens by backup workloads(ex, nightly rsync). That's because the workload makes just use-once pages and touches pages twice. It promotes the page into active list so that it results in working set page eviction.

Some app developer want to support POSIX_FADV_NOREUSE. But other OSes don't support it, either.
(http://marc.info/?l=linux-mm&m=128928979512086&w=2)

By other approach, app developers use POSIX_FADV_DONTNEED. But it has a problem. If kernel meets page is writing during invalidate_mapping_pages, it can't work.
It is very hard for application programmer to use it. Because they always have to sync data before calling fadivse(..POSIX_FADV_DONTNEED) to make sure the pages could be discardable. At last, they can't use deferred write of kernel so that they could see performance loss.
(http://insights.oetiker.ch/linux/fadvise.html)

In fact, invalidation is very big hint to reclaimer. It means we don't use the page any more. So let's move the writing page into inactive list's head.

Why I need the page to head, Dirty/Writeback page would be flushed sooner or later. It can prevent writeout of pageout which is less effective than flusher's writeout.

Originally, I reused lru_demote of Peter with some change so added his Signed-off-by.

Reported-by Ben Gamari <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Adnrew. Before applying this series, please drop below two patches. mm-deactivate-invalidated-pages.patch mm-deactivate-invalidated-pages-fix.patch

Changelog since v2:
 - mapped page leaves alone - suggested by Mel
 - pass part related PG_reclaim in next patch.

Changelog since v1:
 - modify description
 - correct typo
 - add some comment
---
 include/linux/swap.h |    1 +
 mm/swap.c            |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++ mm/truncate.c        |   16 +++++++--

 3 files changed, 93 insertions(+), 4 deletions(-)

include/linux/swap.h +0 -0

index eba53e7..84375e4 100644

[...]

include/linux/swap.h void add_page_to_unevictable_list(struct page *page) · line 277

Quoted 9 lines

include/linux/swap.h +5 -0

@@ -267,6 +270,63 @@ void add_page_to_unevictable_list(struct page *page)
  }
 
  /*
+ * This function is used by invalidate_mapping_pages.
+ * If the page can't be invalidated, this function moves the page
+ * into inative list's head. Because the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective
+ * than reclaimer's random writeout.

The wording is a bit confusing, I find.  It sounds a bit like the flusher's chance is increased by moving it to the inactive list in the first place, but the key is that it is moved to the head instead of, what one would expect, the tail of the list.  How about:

       If the page can not be invalidated, it is moved to the inactive list to speed up its reclaim.  It is moved to the head of the list, rather than the tail, to give the flusher threads some time to write it out, as this is much more effective than the single-page writeout from reclaim.

include/linux/swap.h void add_page_to_unevictable_list(struct page *page) · line 278

Quoted 1 line

include/linux/swap.h +1 -0

+static void __lru_deactivate(struct page *page, struct zone *zone)

Do you insist on the underscores? :)

Quoted 22 lines

include/linux/swap.h +21 -0

+{
+     int lru, file;
+     unsigned long vm_flags;
+
+     if (!PageLRU(page) || !PageActive(page))
+             return;
+
+     /* Some processes are using the page */
+     if (page_mapped(page))
+             return;
+
+     file = page_is_file_cache(page);
+     lru = page_lru_base_type(page);
+     del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
+     ClearPageActive(page);
+     ClearPageReferenced(page);
+     add_page_to_lru_list(zone, page, lru);
+     __count_vm_event(PGDEACTIVATE);
+
+     update_page_reclaim_stat(zone, page, file, 0);
+}

include/linux/swap.h void add_page_to_unevictable_list(struct page *page) · line 302

Quoted 3 lines

include/linux/swap.h +3 -0

+
+/*
+ * This function must be called with preemption disable.

Why is that?  Unless I missed something, the only thing that needs protection is the per-cpu pagevec reference the only user of this function passes in.  But this should be the caller's concern and is not really a requirement of this function per-se, is it?

include/linux/swap.h void add_page_to_unevictable_list(struct page *page) · line 303

Quoted 2 lines

include/linux/swap.h +1 -0

+static void __pagevec_lru_deactivate(struct pagevec *pvec)

More underscores!

Quoted 32 lines

include/linux/swap.h +24 -0

+{
+     int i;
+     struct zone *zone = NULL;
+
+     for (i = 0; i < pagevec_count(pvec); i++) {
+             struct page *page = pvec->pages[i];
+             struct zone *pagezone = page_zone(page);
+
+             if (pagezone != zone) {
+                     if (zone)
+                             spin_unlock_irq(&zone->lru_lock);
+                     zone = pagezone;
+                     spin_lock_irq(&zone->lru_lock);
+             }
+             __lru_deactivate(page, zone);
+     }
+     if (zone)
+             spin_unlock_irq(&zone->lru_lock);
+
+     release_pages(pvec->pages, pvec->nr, pvec->cold);
+     pagevec_reinit(pvec);
+}
+
+/*
   * Drain pages out of the cpu's pagevecs.
   * Either "cpu" is the current CPU, and preemption has already been
   * disabled; or "cpu" is being hot-unplugged, and is already dead.

@@ -292,6 +352,26 @@ static void drain_cpu_pagevecs(int cpu)
               pagevec_move_tail(pvec);
               local_irq_restore(flags);
       }

include/linux/swap.h static void drain_cpu_pagevecs(int cpu) · line 364

Quoted 10 lines

include/linux/swap.h +10 -0

+
+     pvec = &per_cpu(lru_deactivate_pvecs, cpu);
+     if (pagevec_count(pvec))
+             __pagevec_lru_deactivate(pvec);
+}
+
+/*
+ * Forcefully deactivate a page.
+ * This function is used for reclaiming the page ASAP when the page
+ * can't be invalidated by Dirty/Writeback.

How about:

/**
 * lru_deactivate_page - forcefully deactivate a page
 * @page: page to deactivate
 *
 * This function hints the VM that @page is a good reclaim candidate,
 * for example if its invalidation fails due to the page being dirty
 * or under writeback.
 */

include/linux/swap.h static void drain_cpu_pagevecs(int cpu) · line 365

Quoted 1 line

include/linux/swap.h +1 -0

+void lru_deactivate_page(struct page *page)

I would love that lru_ prefix for most of the API in this file.  In fact, the file should probably be called lru.c.  But for now, can you keep the naming consistent and call it deactivate_page?

Quoted 11 lines

include/linux/swap.h +7 -0

+     if (likely(get_page_unless_zero(page))) {
+             struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
+
+             if (!pagevec_add(pvec, page))
+                     __pagevec_lru_deactivate(pvec);
+             put_cpu_var(lru_deactivate_pvecs);
+     }
  }
 
  void lru_add_drain(void)

[...]

include/linux/swap.h unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 367

Quoted 11 lines

include/linux/swap.h +5 -2

@@ -359,8 +360,15 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
                       if (lock_failed)
                               continue;
 
-                     ret += invalidate_inode_page(page);
-
+                     ret = invalidate_inode_page(page);
+                     /*
+                      * If the page was dirty or under writeback we cannot
+                      * invalidate it now.  Move it to the head of the
+                      * inactive LRU for using deferred writeback of flusher.

This would also be less confusing if it would say

       Move it to the head of the inactive LRU (rather than the tail) for using [...]

But I am not sure that this detail is interesting at this point.  It would be more interesting to name the reasons for why the page is moved to the inactive list in the first place:

       If the page is dirty or under writeback, we can not invalidate it now.  But we assume that attempted invalidation is a hint that the page is no longer of interest and try to speed up its reclaim.

include/linux/swap.h unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 374

Quoted 7 lines

include/linux/swap.h +4 -0

+                      */
+                     if (!ret)
+                             lru_deactivate_page(page);
+                     count += ret;
                       unlock_page(page);
                       if (next > end)
                               break;

       Hannes

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +23 -13

index 19e0812..936b281 100644
@@ -275,28 +275,50 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);
-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-
-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;
+
+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.

mm/swap.c +2 -0

+		update_page_reclaim_stat(zone, page, file, 0);
+	}
 }
 
 /*

Signature · 1 line

1.7.0.4

Quoted 128 lines · Minchan Kim

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +27 -13

index 19e0812..936b281 100644
@@ -275,28 +275,50 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);
-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-
-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;
+
+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+
+		file = page_is_file_cache(page);

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 319

Quoted 6 lines

mm/swap.c +6 -0

+		lru = page_lru_base_type(page);
+		del_page_from_lru_list(zone, page, lru + active);
+		ClearPageActive(page);
+		ClearPageReferenced(page);
+		add_page_to_lru_list(zone, page, lru);
+		__count_vm_event(PGDEACTIVATE);

You update PGDEACTIVATE whether the page was active or not.

Quoted 129 lines · Mel Gorman

Quoted 128 lines · Minchan Kim

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +27 -13

index 19e0812..936b281 100644
@@ -275,28 +275,50 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);
-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-
-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;
+
+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+
+		file = page_is_file_cache(page);

Quoted 8 lines

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 320

Quoted 6 lines

mm/swap.c +6 -0

+		lru = page_lru_base_type(page);
+		del_page_from_lru_list(zone, page, lru + active);
+		ClearPageActive(page);
+		ClearPageReferenced(page);
+		add_page_to_lru_list(zone, page, lru);
+		__count_vm_event(PGDEACTIVATE);

You update PGDEACTIVATE whether the page was active or not.

My fault.
Resend.

Subject: [PATCH v3 2/3] Reclaim invalidated page ASAP

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   49 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 47 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +15 -6

index 19e0812..1f1f435 100644
@@ -275,28 +275,51 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);

mm/swap.c +10 -0

+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+
+		file = page_is_file_cache(page);
+		lru = page_lru_base_type(page);

Quoted 259 lines · Minchan Kim

Quoted 129 lines · Mel Gorman

Quoted 128 lines · Minchan Kim

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +27 -13

index 19e0812..936b281 100644
@@ -275,28 +275,50 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);
-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-
-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;
+
+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+
+		file = page_is_file_cache(page);

Quoted 8 lines

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 320

Quoted 6 lines

mm/swap.c +6 -0

+		lru = page_lru_base_type(page);
+		del_page_from_lru_list(zone, page, lru + active);
+		ClearPageActive(page);
+		ClearPageReferenced(page);
+		add_page_to_lru_list(zone, page, lru);
+		__count_vm_event(PGDEACTIVATE);

You update PGDEACTIVATE whether the page was active or not.

My fault.
Resend.

Subject: [PATCH v3 2/3] Reclaim invalidated page ASAP

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Changelog since v2:
- put ClearPageReclaim in set_page_dirty - suggested by Wu.

Changelog since v1:
- make the invalidated page reclaim asap - suggested by Andrew.

---
 mm/page-writeback.c |   12 +++++++++++-
 mm/swap.c           |   49 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 47 insertions(+), 14 deletions(-)

mm/page-writeback.c +11 -1

index fc93802..88587a5 100644
@@ -1250,6 +1250,17 @@ int set_page_dirty(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 
+	/*
+	 * readahead/lru_deactivate_page could remain
+	 * PG_readahead/PG_reclaim due to race with end_page_writeback
+	 * About readahead, if the page is written, the flags would be
+	 * reset. So no problem.
+	 * About lru_deactivate_page, if the page is redirty, the flag
+	 * will be reset. So no problem. but if the page is used by readahead
+	 * it will confuse readahead and  make it restart the size rampup
+	 * process. But it's a trivial problem.
+	 */
+	ClearPageReclaim(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

@@ -1307,7 +1318,6 @@ int clear_page_dirty_for_io(struct page *page)
 
 	BUG_ON(!PageLocked(page));
 
-	ClearPageReclaim(page);
 	if (mapping && mapping_cap_account_dirty(mapping)) {
 		/*
 		 * Yes, Virginia, this is indeed insane.

mm/swap.c +15 -12

index 19e0812..1f1f435 100644
@@ -275,28 +275,51 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.
  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);
-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 304

Quoted 4 lines

mm/swap.c +3 -1

-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;
+

I should have said this the last time but if you do another revision, make active a "bool". There is a very slow migration of int to bool in cases it makes sense. It's not urgent though.

Quoted 11 lines

mm/swap.c +10 -0

+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+
+		file = page_is_file_cache(page);
+		lru = page_lru_base_type(page);

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 324

Quoted 10 lines

mm/swap.c +8 -0

+		del_page_from_lru_list(zone, page, lru + active);
+		ClearPageActive(page);
+		ClearPageReferenced(page);
+		add_page_to_lru_list(zone, page, lru);
+		if (active)
+			__count_vm_event(PGDEACTIVATE);
+		update_page_reclaim_stat(zone, page, file, 0);
+	}
 }
 

Whether you update active's type or not;

Acked-by Mel Gorman <[email protected]>

Signature · 4 lines

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

mm/swap.c +9 -0

index 19e0812..1f1f435 100644
@@ -275,28 +275,51 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none

mm/swap.c +1 -4

-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);

mm/swap.c +8 -0

+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+

Quoted 21 lines · Minchan Kim

mm/swap.c +13 -0

index 19e0812..1f1f435 100644
@@ -275,28 +275,51 @@ void add_page_to_unevictable_list(struct page *page)
  * into inative list's head. Because the VM expects the page would
  * be writeout by flusher. The flusher's writeout is much effective
  * than reclaimer's random writeout.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim
+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be writeout by flusher. The flusher's writeout is much effective than
+ * reclaimer's random writeout.

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 295

Quoted 6 lines

mm/swap.c +1 -1

  */
 static void __lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
-	unsigned long vm_flags;
+	int active = 0;

vm_flags is never used in this series.

Quoted 14 lines

mm/swap.c +1 -8

-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
-
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
-
-	file = page_is_file_cache(page);
-	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
-	ClearPageActive(page);
-	ClearPageReferenced(page);

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 303

Quoted 6 lines

mm/swap.c +2 -4

-	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
-
-	update_page_reclaim_stat(zone, page, file, 0);
+	if (PageActive(page))
+		active = 1;

active = PageActive(page)

Quoted 9 lines

mm/swap.c +8 -0

+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window
+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 322

Quoted 10 lines

mm/swap.c +10 -0

+		file = page_is_file_cache(page);
+		lru = page_lru_base_type(page);
+		del_page_from_lru_list(zone, page, lru + active);
+		ClearPageActive(page);
+		ClearPageReferenced(page);
+		add_page_to_lru_list(zone, page, lru);
+		if (active)
+			__count_vm_event(PGDEACTIVATE);
+		update_page_reclaim_stat(zone, page, file, 0);
+	}

If we lose the race with writeback, the completion handler won't see PG_reclaim, won't move the page, and we have an unwanted clean cache page on the active list. Given the pagevec caching of those pages it could be rather likely that IO completes before the above executes.

Shouldn't this be

	if (PageWriteback() || PageDirty()) {
		SetPageReclaim()
		move_to_inactive_head()
	} else {
		move_to_inactive_tail()
	}

instead?

Hannes

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Quoted 23 lines

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Quoted 7 lines

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

I still dislike this one. I doubt this trick makes much benefit in real world workload.

Quoted 31 lines · KOSAKI Motohiro

Quoted 23 lines

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Quoted 7 lines

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Quoted 3 lines

I still dislike this one. I doubt this trick makes much benefit in real world workload.

I would agree except as said elsewhere, it's a chicken and egg problem. We don't have a real world test because fadvise is not useful in its current iteration. I'm hoping that there will be a test comparing

rsync on vanilla kernel
rsync on patched kernel
rsync+patch on vanilla kernel
rsync+patch on patched kernel

Are the results of such a test likely to happen?

Signature · 4 lines

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

I would agree except as said elsewhere, it's a chicken and egg problem. We don't have a real world test because fadvise is not useful in its current iteration. I'm hoping that there will be a test comparing

rsync on vanilla kernel
rsync on patched kernel
rsync+patch on vanilla kernel
rsync+patch on patched kernel

Are the results of such a test likely to happen?

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Changelog since v2:
- remove unnecessary description
Changelog since v1:
- change word from promote to activate
- add activate argument to zap_pte_range and family function

---
 include/linux/mm.h |    4 ++--
 mm/madvise.c       |    4 ++--
 mm/memory.c        |   38 +++++++++++++++++++++++---------------
 mm/mmap.c          |    4 ++--
 4 files changed, 29 insertions(+), 21 deletions(-)

include/linux/mm.h +0 -0

index e097df6..6032881 100644

Quoted 30 lines · Minchan Kim

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Changelog since v2:
 - remove unnecessary description
Changelog since v1:
 - change word from promote to activate
 - add activate argument to zap_pte_range and family function

---
 include/linux/mm.h |    4 ++--
 mm/madvise.c       |    4 ++--
 mm/memory.c        |   38 +++++++++++++++++++++++--------------- mm/mmap.c          |    4 ++--

 4 files changed, 29 insertions(+), 21 deletions(-)

include/linux/mm.h +0 -0

index e097df6..6032881 100644

include/linux/mm.h struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, · line 782

Quoted 5 lines

include/linux/mm.h +1 -1

@@ -779,11 +779,11 @@ struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
  int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
               unsigned long size);
  unsigned long zap_page_range(struct vm_area_struct *vma, unsigned long address,
-             unsigned long size, struct zap_details *);
+             unsigned long size, struct zap_details *, bool activate);

I would prefer naming the parameter 'ignore_references' or something similar, so that it reflects the immediate effect on the zappers' behaviour, not what mark_page_accessed() might end up doing.

Other than that, the patch looks good to me.

       Hannes

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Changelog since v2:
- remove unnecessary description
Changelog since v1:
- change word from promote to activate
- add activate argument to zap_pte_range and family function

---
 include/linux/mm.h |    4 ++--
 mm/madvise.c       |    4 ++--
 mm/memory.c        |   38 +++++++++++++++++++++++---------------
 mm/mmap.c          |    4 ++--
 4 files changed, 29 insertions(+), 21 deletions(-)

Quoted 18 lines · Minchan Kim

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Changelog since v2:
 - remove unnecessary description
Changelog since v1:
 - change word from promote to activate
 - add activate argument to zap_pte_range and family function

Quoted 6 lines

---
 include/linux/mm.h |    4 ++--
 mm/madvise.c       |    4 ++--
 mm/memory.c        |   38 +++++++++++++++++++++++--------------- mm/mmap.c          |    4 ++--

 4 files changed, 29 insertions(+), 21 deletions(-)

Everyone else seems pretty happy with this, and I've not checked at all whether it achieves your purpose; but personally I'd much prefer a smaller patch which adds your "activate" or "ignore_references" flag to struct zap_details, instead of passing this exceptional arg down lots of levels.  That's precisely the purpose of zap_details, to gather together a few things that aren't needed in the common case (though I admit the NULL details defaulting may be ugly).

Hugh