[PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nikita Danilov]

set PG_reclaimed bit on pages that are under writeback when shrink_list()
looks at them: these pages are at end of the inactive list, and it only makes
sense to reclaim them as soon as possible when writeout finishes.

Signed-off-by: Nikita Danilov <nikita@clusterfs.com>


 mm/vmscan.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

diff -puN mm/vmscan.c~SetPageReclaimed-inactive-tail mm/vmscan.c
--- bk-linux/mm/vmscan.c~SetPageReclaimed-inactive-tail	2005-04-17 17:52:53.000000000 +0400
+++ bk-linux-nikita/mm/vmscan.c	2005-04-17 17:52:53.000000000 +0400
@@ -556,8 +556,11 @@ static int shrink_list(struct list_head 
 		if (page_mapped(page) || PageSwapCache(page))
 			sc->nr_scanned++;
 
-		if (PageWriteback(page))
+		if (PageWriteback(page)) {
+			if (!PageReclaim(page))
+				SetPageReclaim(page);
 			goto keep_locked;
+		}
 
 		referenced = page_referenced(page, 1, sc->priority <= 0);
 		/* In active use or really unfreeable?  Activate it. */

_
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nick Piggin]

On Sun, 2005-04-17 at 21:38 +0400, Nikita Danilov wrote:
> set PG_reclaimed bit on pages that are under writeback when shrink_list()
> looks at them: these pages are at end of the inactive list, and it only makes
> sense to reclaim them as soon as possible when writeout finishes.
> 

I agree it makes sense, but this is racy I think. It will leave
PG_reclaim set in some cases and hit bad_page. The trivial fix is
to remove the PG_reclaim check from bad_page. It looks a bit more
tricky to do it "nicely".


-- 
SUSE Labs, Novell Inc.


--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nikita Danilov]

Nick Piggin writes:
 > On Sun, 2005-04-17 at 21:38 +0400, Nikita Danilov wrote:
 > > set PG_reclaimed bit on pages that are under writeback when shrink_list()
 > > looks at them: these pages are at end of the inactive list, and it only makes
 > > sense to reclaim them as soon as possible when writeout finishes.
 > > 
 > 
 > I agree it makes sense, but this is racy I think. It will leave
 > PG_reclaim set in some cases and hit bad_page. The trivial fix is

Good catch, thanks. By the way, bad_page() doesn't clear PG_reclaim that
free_pages_check() checks for. Is there some deep meaning in this?

 > to remove the PG_reclaim check from bad_page. It looks a bit more
 > tricky to do it "nicely".
 > 

I think "trivial fix" makes more sense, if we redefine PG_reclaim to
mean "page has been seen at end of the inactive list". This bit will be
set at the very beginning of shrink_list() (every for locked pages), and
cleared either by end-io handler, or when page is moved to the active
list. Idea being that if page made its way to end of the inactive list,
but VM failed to reclaim it due to some race, page should be reclaimed
as soon as possible (to get better LRU approximation).

Does this make sense?

 > 
 > -- 
 > SUSE Labs, Novell Inc.

Nikita.
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Andrew Morton]

Nikita Danilov <nikita@clusterfs.com> wrote:
>
> 
> set PG_reclaimed bit on pages that are under writeback when shrink_list()
> looks at them: these pages are at end of the inactive list, and it only makes
> sense to reclaim them as soon as possible when writeout finishes.
> 

Makes sense, I guess.  It would be nice to know how many pages actually get
this treatment, and under what situations.

To address the race which Nick identified I think we can do it this way?

--- 25/mm/vmscan.c~mm-shrink_list-set-pg_reclaimed	2005-04-25 21:26:28.853691816 -0700
+++ 25-akpm/mm/vmscan.c	2005-04-25 21:27:28.180672744 -0700
@@ -401,8 +401,18 @@ static int shrink_list(struct list_head 
 		if (page_mapped(page) || PageSwapCache(page))
 			sc->nr_scanned++;
 
-		if (PageWriteback(page))
+		if (PageWriteback(page)) {
+			if (!PageReclaim(page)) {
+				SetPageReclaim(page);
+				if (!PageWriteback(page)) {
+					/*
+					 * oops, the writeout just completed.
+					 */
+					ClearPageReclaim(page);
+				}
+			}
 			goto keep_locked;
+		}
 
 		referenced = page_referenced(page, 1, sc->priority <= 0);
 		/* In active use or really unfreeable?  Activate it. */
_

--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nick Piggin]

On Mon, 2005-04-25 at 21:29 -0700, Andrew Morton wrote:
> Nikita Danilov <nikita@clusterfs.com> wrote:
> >
> > 
> > set PG_reclaimed bit on pages that are under writeback when shrink_list()
> > looks at them: these pages are at end of the inactive list, and it only makes
> > sense to reclaim them as soon as possible when writeout finishes.
> > 
> 
> Makes sense, I guess.  It would be nice to know how many pages actually get
> this treatment, and under what situations.
> 
> To address the race which Nick identified I think we can do it this way?
> 

I did the same patch a while back and I had a feeling this didn't work
either. I can't immediately see a race... but don't put this one in
2.6.12!

I agree it makes sense.

-- 
SUSE Labs, Novell Inc.


--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nick Piggin]

On Tue, 2005-04-26 at 15:27 +1000, Nick Piggin wrote:
> On Mon, 2005-04-25 at 21:29 -0700, Andrew Morton wrote:
> > Nikita Danilov <nikita@clusterfs.com> wrote:
> > >
> > > 
> > > set PG_reclaimed bit on pages that are under writeback when shrink_list()
> > > looks at them: these pages are at end of the inactive list, and it only makes
> > > sense to reclaim them as soon as possible when writeout finishes.
> > > 
> > 
> > Makes sense, I guess.  It would be nice to know how many pages actually get
> > this treatment, and under what situations.
> > 
> > To address the race which Nick identified I think we can do it this way?
> > 
> 
> I did the same patch a while back and I had a feeling this didn't work
> either. I can't immediately see a race...


Somewhere, a light-bulb flickers...

shrink_list():                | end_page_writeback():
------------------------------|---------------------------------------                              
if (PageWriteback(page)) {    |
  if (!PageReclaim(page)) {   |
                              | if (!TestClearPageReclaim(page)) {
    SetPageReclaim(page);     |
    if (!PageWriteback(page)) |
      ClearPageReclaim(page); |
                              |   if (!test_clear_page_writeback(page))
                              |     BUG();

          |
          |
          V
PageReclaim && !PageWriteback == BUG


And yes, IIRC I actually did hit this race when testing - probably this
*exact* code (granted it took a while :P)

-- 
SUSE Labs, Novell Inc.


--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nikita Danilov]

Andrew Morton writes:

[...]

 > 
 > To address the race which Nick identified I think we can do it this way?

I think that instead of fixing that race we'd better to make it valid:
let's redefine PG_reclaim to mean

       "page has been seen on the tail of the inactive list, but VM
       failed to reclaim it right away either because it was dirty, or
       there was some race. Reclaim this page as soon as possible."

Nikita.

set PG_reclaimed bit on pages that are under writeback when shrink_list()
looks at them: these pages are at end of the inactive list, and it only makes
sense to reclaim them as soon as possible when writeout finishes.

Signed-off-by: Nikita Danilov <nikita@clusterfs.com>


 mm/filemap.c    |   10 +++++----
 mm/page_alloc.c |    3 +-
 mm/swap.c       |   12 +----------
 mm/vmscan.c     |   60 +++++++++++++++++++++++++++++++++++++++++---------------
 4 files changed, 54 insertions(+), 31 deletions(-)

diff -puN mm/vmscan.c~SetPageReclaimed-inactive-tail mm/vmscan.c
--- bk-linux/mm/vmscan.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
+++ bk-linux-nikita/mm/vmscan.c	2005-04-22 12:11:31.000000000 +0400
@@ -41,13 +41,18 @@
 
 /* possible outcome of pageout() */
 typedef enum {
-	/* failed to write page out, page is locked */
+	/* failed to write page out, page is unlocked */
 	PAGE_KEEP,
+	/*
+	 * failed to write page out _now_ owing to some temporary condition,
+	 * page is locked
+	 */
+	PAGE_RACE,
 	/* move page to the active list, page is locked */
 	PAGE_ACTIVATE,
 	/* page has been sent to the disk successfully, page is unlocked */
 	PAGE_SUCCESS,
-	/* page was queued for asynchronous pageout */
+	/* page was queued for asynchronous pageout, page is locked */
 	PAGE_ASYNC,
 	/* page is clean and locked */
 	PAGE_CLEAN,
@@ -285,6 +290,12 @@ static int may_write_to_queue(struct bac
 	return 0;
 }
 
+static inline void set_page_reclaim(struct page *page)
+{
+	if (!PageReclaim(page))
+		SetPageReclaim(page);
+}
+
 /*
  * We detected a synchronous write error writing a page out.  Probably
  * -ENOSPC.  We need to propagate that into the address_space for a subsequent
@@ -464,7 +475,7 @@ static pageout_t pageout(struct page *pa
 	 * swapfile.c:page_queue_congested().
 	 */
 	if (!is_page_cache_freeable(page))
-		return PAGE_KEEP;
+		return PAGE_RACE;
 	if (!mapping) {
 		/*
 		 * Some data journaling orphaned pages can have
@@ -482,7 +493,7 @@ static pageout_t pageout(struct page *pa
 	if (mapping->a_ops->writepage == NULL)
 		return PAGE_ACTIVATE;
 	if (!may_write_to_queue(mapping->backing_dev_info))
-		return PAGE_KEEP;
+		return PAGE_RACE;
 	/*
 	 * Don't call ->writepage when page is met for the first time during
 	 * scanning. Reasons:
@@ -570,8 +581,10 @@ static int shrink_list(struct list_head 
 		page = lru_to_page(page_list);
 		list_del(&page->lru);
 
-		if (TestSetPageLocked(page))
+		if (TestSetPageLocked(page)) {
+			set_page_reclaim(page);
 			goto keep;
+		}
 
 		BUG_ON(PageActive(page));
 
@@ -581,7 +594,7 @@ static int shrink_list(struct list_head 
 			sc->nr_scanned++;
 
 		if (PageWriteback(page))
-			goto keep_locked;
+			goto keep_reclaim;
 
 		inuse = page_mapping_inuse(page);
 		referenced = page_referenced(page, 1, sc->priority <= 0,
@@ -614,7 +627,7 @@ static int shrink_list(struct list_head 
 			case SWAP_FAIL:
 				goto activate_locked;
 			case SWAP_AGAIN:
-				goto keep_locked;
+				goto keep_reclaim;
 			case SWAP_SUCCESS:
 				; /* try to free the page below */
 			}
@@ -624,14 +637,16 @@ static int shrink_list(struct list_head 
 			if (referenced)
 				goto keep_locked;
 			if (!may_enter_fs)
-				goto keep_locked;
+				goto keep_reclaim;
 			if (laptop_mode && !sc->may_writepage)
-				goto keep_locked;
+				goto keep_reclaim;
 
 			/* Page is dirty, try to write it out here */
 			switch(pageout(page, mapping, sc)) {
 			case PAGE_KEEP:
 				goto keep_locked;
+			case PAGE_RACE:
+				goto keep_reclaim;
 			case PAGE_ACTIVATE:
 				goto activate_locked;
 			case PAGE_ASYNC:
@@ -685,7 +700,7 @@ static int shrink_list(struct list_head 
 		}
 
 		if (!mapping)
-			goto keep_locked;	/* truncate got there first */
+			goto keep_reclaim;	/* truncate got there first */
 
 		write_lock_irq(&mapping->tree_lock);
 
@@ -696,7 +711,7 @@ static int shrink_list(struct list_head 
 		 */
 		if (page_count(page) != 2 || PageDirty(page)) {
 			write_unlock_irq(&mapping->tree_lock);
-			goto keep_locked;
+			goto keep_reclaim;
 		}
 
 #ifdef CONFIG_SWAP
@@ -724,6 +739,14 @@ free_it:
 activate_locked:
 		SetPageActive(page);
 		pgactivate++;
+		goto keep_locked;
+keep_reclaim:
+		/*
+		 * the page reached the end of the inactive list, it should be
+		 * reclaimed as soon as possible to maintain LRU
+		 * approximation.
+		 */
+		set_page_reclaim(page);
 keep_locked:
 		unlock_page(page);
 keep:
@@ -1420,21 +1443,26 @@ static void __kpgout(void)
 		page = lru_to_page(&todo);
 		list_del(&page->lru);
 
-		if (TestSetPageLocked(page))
+		if (TestSetPageLocked(page)) {
+			set_page_reclaim(page);
 			outcome = PAGE_SUCCESS;
-		else if (PageWriteback(page))
-			outcome = PAGE_KEEP;
-		else if (PageDirty(page))
+		} else if (PageWriteback(page)) {
+			outcome = PAGE_RACE;
+		} else if (PageDirty(page)) {
 			outcome = pageout(page,
 					  page_mapping(page), NULL);
-		else
+		} else
 			outcome = PAGE_KEEP;
 
+		if (outcome == PAGE_RACE)
+ 			set_page_reclaim(page);
+
 		switch (outcome) {
 		case PAGE_ASYNC:
 			BUG();
 		case PAGE_ACTIVATE:
 			SetPageActive(page);
+		case PAGE_RACE:
 		case PAGE_KEEP:
 		case PAGE_CLEAN:
 			unlock_page(page);
diff -puN mm/page_alloc.c~SetPageReclaimed-inactive-tail mm/page_alloc.c
--- bk-linux/mm/page_alloc.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
+++ bk-linux-nikita/mm/page_alloc.c	2005-04-22 12:09:59.000000000 +0400
@@ -319,13 +319,14 @@ static inline void free_pages_check(cons
 			1 << PG_private |
 			1 << PG_locked	|
 			1 << PG_active	|
-			1 << PG_reclaim	|
 			1 << PG_slab	|
 			1 << PG_swapcache |
 			1 << PG_writeback )))
 		bad_page(function, page);
 	if (PageDirty(page))
 		ClearPageDirty(page);
+	if (PageReclaim(page))
+		ClearPageReclaim(page);
 }
 
 /*
diff -puN mm/swap.c~SetPageReclaimed-inactive-tail mm/swap.c
--- bk-linux/mm/swap.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
+++ bk-linux-nikita/mm/swap.c	2005-04-22 12:09:59.000000000 +0400
@@ -62,12 +62,7 @@ EXPORT_SYMBOL(put_page);
  * We don't expect many pages to come through here, so don't bother batching
  * things up.
  *
- * To avoid placing the page at the tail of the LRU while PG_writeback is still
- * set, this function will clear PG_writeback before performing the page
- * motion.  Do that inside the lru lock because once PG_writeback is cleared
- * we may not touch the page.
- *
- * Returns zero if it cleared PG_writeback.
+ * Returns zero if page was moved.
  */
 int rotate_reclaimable_page(struct page *page)
 {
@@ -86,12 +81,9 @@ int rotate_reclaimable_page(struct page 
 	zone = page_zone(page);
 	spin_lock_irqsave(&zone->lru_lock, flags);
 	if (PageLRU(page) && !PageActive(page)) {
-		list_del(&page->lru);
-		list_add_tail(&page->lru, &zone->inactive_list);
+		list_move_tail(&page->lru, &zone->inactive_list);
 		inc_page_state(pgrotated);
 	}
-	if (!test_clear_page_writeback(page))
-		BUG();
 	spin_unlock_irqrestore(&zone->lru_lock, flags);
 	return 0;
 }
diff -puN mm/filemap.c~SetPageReclaimed-inactive-tail mm/filemap.c
--- bk-linux/mm/filemap.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
+++ bk-linux-nikita/mm/filemap.c	2005-04-22 12:09:59.000000000 +0400
@@ -445,6 +445,8 @@ void fastcall unlock_page(struct page *p
 	if (!TestClearPageLocked(page))
 		BUG();
 	smp_mb__after_clear_bit();
+	if (unlikely(TestClearPageReclaim(page)))
+		rotate_reclaimable_page(page);
 	wake_up_page(page, PG_locked);
 }
 EXPORT_SYMBOL(unlock_page);
@@ -454,10 +456,10 @@ EXPORT_SYMBOL(unlock_page);
  */
 void end_page_writeback(struct page *page)
 {
-	if (!TestClearPageReclaim(page) || rotate_reclaimable_page(page)) {
-		if (!test_clear_page_writeback(page))
-			BUG();
-	}
+	if (unlikely(TestClearPageReclaim(page)))
+		rotate_reclaimable_page(page);
+	if (!test_clear_page_writeback(page))
+		BUG();
 	smp_mb__after_clear_bit();
 	wake_up_page(page, PG_writeback);
 }

_
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Andrew Morton]

Nikita Danilov <nikita@clusterfs.com> wrote:
>
>  > 
>   > To address the race which Nick identified I think we can do it this way?
> 
>  I think that instead of fixing that race we'd better to make it valid:
>  let's redefine PG_reclaim to mean
> 
>         "page has been seen on the tail of the inactive list, but VM
>         failed to reclaim it right away either because it was dirty, or
>         there was some race. Reclaim this page as soon as possible."
> 
>  Nikita.
> 
>  set PG_reclaimed bit on pages that are under writeback when shrink_list()
>  looks at them: these pages are at end of the inactive list, and it only makes
>  sense to reclaim them as soon as possible when writeout finishes.

Seems a bit too complex to me.  See if you can get it back to a three-liner ;)

--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH]: VM 8/8 shrink_list(): set PG_reclaimed

[Nick Piggin]

On Tue, 2005-04-26 at 14:16 +0400, Nikita Danilov wrote:
> Andrew Morton writes:
> 
> [...]
> 
>  > 
>  > To address the race which Nick identified I think we can do it this way?
> 
> I think that instead of fixing that race we'd better to make it valid:
> let's redefine PG_reclaim to mean
> 
>        "page has been seen on the tail of the inactive list, but VM
>        failed to reclaim it right away either because it was dirty, or
>        there was some race. Reclaim this page as soon as possible."
> 
> Nikita.
> 
> set PG_reclaimed bit on pages that are under writeback when shrink_list()
> looks at them: these pages are at end of the inactive list, and it only makes
> sense to reclaim them as soon as possible when writeout finishes.
> 
> Signed-off-by: Nikita Danilov <nikita@clusterfs.com>
> 
> 
>  mm/filemap.c    |   10 +++++----
>  mm/page_alloc.c |    3 +-
>  mm/swap.c       |   12 +----------
>  mm/vmscan.c     |   60 +++++++++++++++++++++++++++++++++++++++++---------------
>  4 files changed, 54 insertions(+), 31 deletions(-)
> 
> diff -puN mm/vmscan.c~SetPageReclaimed-inactive-tail mm/vmscan.c
> --- bk-linux/mm/vmscan.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
> +++ bk-linux-nikita/mm/vmscan.c	2005-04-22 12:11:31.000000000 +0400

[...]

> diff -puN mm/page_alloc.c~SetPageReclaimed-inactive-tail mm/page_alloc.c
> --- bk-linux/mm/page_alloc.c~SetPageReclaimed-inactive-tail	2005-04-22 12:09:59.000000000 +0400
> +++ bk-linux-nikita/mm/page_alloc.c	2005-04-22 12:09:59.000000000 +0400
> @@ -319,13 +319,14 @@ static inline void free_pages_check(cons
>  			1 << PG_private |
>  			1 << PG_locked	|
>  			1 << PG_active	|
> -			1 << PG_reclaim	|
>  			1 << PG_slab	|
>  			1 << PG_swapcache |
>  			1 << PG_writeback )))
>  		bad_page(function, page);
>  	if (PageDirty(page))
>  		ClearPageDirty(page);
> +	if (PageReclaim(page))
> +		ClearPageReclaim(page);
>  }
>  
>  /*

A bit ugly for maybe little improvement. I agree it makes fine
of sense in theory, but possibly not worthwhile if it doesn't
actually help (but I'm not saying it doesn't).

-- 
SUSE Labs, Novell Inc.


--
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:"aart@kvack.org"> aart@kvack.org </a>