[PATCH] Check PageActive when evictable page and unevicetable page race happen

[PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

In putback_lru_page, unevictable page can be changed into evictable
's one while we move it among lru. So we have checked it again and
rescued it. But we don't check PageActive, again. It could add
active page into inactive list so we can see the BUG in isolate_lru_pages.
(But I didn't see any report because I think it's very subtle)

It could happen in race that zap_pte_range's mark_page_accessed and
putback_lru_page. It's subtle but could be possible.

Note:
While I review the code, I found it. So it's not real report.

Cc: Rik van Riel <riel@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/vmscan.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b3a569f..c0cd1aa 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
 void putback_lru_page(struct page *page)
 {
 	int lru;
-	int active = !!TestClearPageActive(page);
+	int active;
 	int was_unevictable = PageUnevictable(page);
 
 	VM_BUG_ON(PageLRU(page));
@@ -571,6 +571,7 @@ redo:
 	ClearPageUnevictable(page);
 
 	if (page_evictable(page, NULL)) {
+		active = !!TestClearPageActive(page);
 		/*
 		 * For evictable pages, we can use the cache.
 		 * In event of a race, worst case is we end up with an
@@ -584,6 +585,7 @@ redo:
 		 * Put unevictable pages directly on zone's unevictable
 		 * list.
 		 */
+		ClearPageActive(page);
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-- 
1.7.1

[PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

In putback_lru_page, unevictable page can be changed into evictable
's one while we move it among lru. So we have checked it again and
rescued it. But we don't check PageActive, again. It could add
active page into inactive list so we can see the BUG in isolate_lru_pages.
(But I didn't see any report because I think it's very subtle)

It could happen in race that zap_pte_range's mark_page_accessed and
putback_lru_page. It's subtle but could be possible.

Note:
While I review the code, I found it. So it's not real report.

Cc: Rik van Riel <riel@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/vmscan.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b3a569f..c0cd1aa 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
 void putback_lru_page(struct page *page)
 {
 	int lru;
-	int active = !!TestClearPageActive(page);
+	int active;
 	int was_unevictable = PageUnevictable(page);
 
 	VM_BUG_ON(PageLRU(page));
@@ -571,6 +571,7 @@ redo:
 	ClearPageUnevictable(page);
 
 	if (page_evictable(page, NULL)) {
+		active = !!TestClearPageActive(page);
 		/*
 		 * For evictable pages, we can use the cache.
 		 * In event of a race, worst case is we end up with an
@@ -584,6 +585,7 @@ redo:
 		 * Put unevictable pages directly on zone's unevictable
 		 * list.
 		 */
+		ClearPageActive(page);
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-- 
1.7.1

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

On Sun, Apr 24, 2011 at 09:25:51AM +0900, Minchan Kim wrote:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)

As I look the code further, that's because lru_cache_add_lru always 
cleans up PageActive regardless of LRU list. 
If active page goes to inactive list, we shouldn't meet the BUG 
but it's apparently wrong. 

-- 
Kind regards,
Minchan Kim

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

On Sun, Apr 24, 2011 at 09:25:51AM +0900, Minchan Kim wrote:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)

As I look the code further, that's because lru_cache_add_lru always 
cleans up PageActive regardless of LRU list. 
If active page goes to inactive list, we shouldn't meet the BUG 
but it's apparently wrong. 

-- 
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[KOSAKI Motohiro]

2011/4/24 Minchan Kim <minchan.kim@gmail.com>:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)
>
> It could happen in race that zap_pte_range's mark_page_accessed and
> putback_lru_page. It's subtle but could be possible.
>
> Note:
> While I review the code, I found it. So it's not real report.
>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/vmscan.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b3a569f..c0cd1aa 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
>  void putback_lru_page(struct page *page)
>  {
>        int lru;
> -       int active = !!TestClearPageActive(page);
> +       int active;
>        int was_unevictable = PageUnevictable(page);
>
>        VM_BUG_ON(PageLRU(page));
> @@ -571,6 +571,7 @@ redo:
>        ClearPageUnevictable(page);
>
>        if (page_evictable(page, NULL)) {
> +               active = !!TestClearPageActive(page);
>                /*
>                 * For evictable pages, we can use the cache.
>                 * In event of a race, worst case is we end up with an
> @@ -584,6 +585,7 @@ redo:
>                 * Put unevictable pages directly on zone's unevictable
>                 * list.
>                 */
> +               ClearPageActive(page);
>                lru = LRU_UNEVICTABLE;
>                add_page_to_unevictable_list(page);

I think we forgot 'goto redo' case. following patch is better?

------------------------------------------------
        if (page_evictable(page, NULL)) {
                /*
                 * For evictable pages, we can use the cache.
                 * In event of a race, worst case is we end up with an
                 * unevictable page on [in]active list.
                 * We know how to handle that.
                 */
                lru = active + page_lru_base_type(page);
+              if (active)
+                   SetPageActive(page);
                lru_cache_add_lru(page, lru);

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[KOSAKI Motohiro]

2011/4/24 Minchan Kim <minchan.kim@gmail.com>:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)
>
> It could happen in race that zap_pte_range's mark_page_accessed and
> putback_lru_page. It's subtle but could be possible.
>
> Note:
> While I review the code, I found it. So it's not real report.
>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/vmscan.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b3a569f..c0cd1aa 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
>  void putback_lru_page(struct page *page)
>  {
>        int lru;
> -       int active = !!TestClearPageActive(page);
> +       int active;
>        int was_unevictable = PageUnevictable(page);
>
>        VM_BUG_ON(PageLRU(page));
> @@ -571,6 +571,7 @@ redo:
>        ClearPageUnevictable(page);
>
>        if (page_evictable(page, NULL)) {
> +               active = !!TestClearPageActive(page);
>                /*
>                 * For evictable pages, we can use the cache.
>                 * In event of a race, worst case is we end up with an
> @@ -584,6 +585,7 @@ redo:
>                 * Put unevictable pages directly on zone's unevictable
>                 * list.
>                 */
> +               ClearPageActive(page);
>                lru = LRU_UNEVICTABLE;
>                add_page_to_unevictable_list(page);

I think we forgot 'goto redo' case. following patch is better?

------------------------------------------------
        if (page_evictable(page, NULL)) {
                /*
                 * For evictable pages, we can use the cache.
                 * In event of a race, worst case is we end up with an
                 * unevictable page on [in]active list.
                 * We know how to handle that.
                 */
                lru = active + page_lru_base_type(page);
+              if (active)
+                   SetPageActive(page);
                lru_cache_add_lru(page, lru);

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

On Sun, Apr 24, 2011 at 9:25 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)
>
> It could happen in race that zap_pte_range's mark_page_accessed and
> putback_lru_page. It's subtle but could be possible.

Please Ignore this. I was confused.
The race never happens.


-- 
Kind regards,
Minchan Kim

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

On Sun, Apr 24, 2011 at 9:25 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> In putback_lru_page, unevictable page can be changed into evictable
> 's one while we move it among lru. So we have checked it again and
> rescued it. But we don't check PageActive, again. It could add
> active page into inactive list so we can see the BUG in isolate_lru_pages.
> (But I didn't see any report because I think it's very subtle)
>
> It could happen in race that zap_pte_range's mark_page_accessed and
> putback_lru_page. It's subtle but could be possible.

Please Ignore this. I was confused.
The race never happens.


-- 
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

Hi KOSAKI,

On Sun, Apr 24, 2011 at 12:02:57PM +0900, KOSAKI Motohiro wrote:
> 2011/4/24 Minchan Kim <minchan.kim@gmail.com>:
> > In putback_lru_page, unevictable page can be changed into evictable
> > 's one while we move it among lru. So we have checked it again and
> > rescued it. But we don't check PageActive, again. It could add
> > active page into inactive list so we can see the BUG in isolate_lru_pages.
> > (But I didn't see any report because I think it's very subtle)
> >
> > It could happen in race that zap_pte_range's mark_page_accessed and
> > putback_lru_page. It's subtle but could be possible.
> >
> > Note:
> > While I review the code, I found it. So it's not real report.
> >
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/vmscan.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index b3a569f..c0cd1aa 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
> >  void putback_lru_page(struct page *page)
> >  {
> >        int lru;
> > -       int active = !!TestClearPageActive(page);
> > +       int active;
> >        int was_unevictable = PageUnevictable(page);
> >
> >        VM_BUG_ON(PageLRU(page));
> > @@ -571,6 +571,7 @@ redo:
> >        ClearPageUnevictable(page);
> >
> >        if (page_evictable(page, NULL)) {
> > +               active = !!TestClearPageActive(page);
> >                /*
> >                 * For evictable pages, we can use the cache.
> >                 * In event of a race, worst case is we end up with an
> > @@ -584,6 +585,7 @@ redo:
> >                 * Put unevictable pages directly on zone's unevictable
> >                 * list.
> >                 */
> > +               ClearPageActive(page);
> >                lru = LRU_UNEVICTABLE;
> >                add_page_to_unevictable_list(page);
> 
> I think we forgot 'goto redo' case. following patch is better?
> 
> ------------------------------------------------
>         if (page_evictable(page, NULL)) {
>                 /*
>                  * For evictable pages, we can use the cache.
>                  * In event of a race, worst case is we end up with an
>                  * unevictable page on [in]active list.
>                  * We know how to handle that.
>                  */
>                 lru = active + page_lru_base_type(page);
> +              if (active)
> +                   SetPageActive(page);
>                 lru_cache_add_lru(page, lru);

PageActive is reset by lru_cache_add_lru so it's meaningless.
BTW, please ignore this patch. :)
 
I think LRU status of isolated page cannot be changed.
Thanks for the review. 


-- 
Kind regards,
Minchan Kim

Re: [PATCH] Check PageActive when evictable page and unevicetable page race happen

[Minchan Kim]

Hi KOSAKI,

On Sun, Apr 24, 2011 at 12:02:57PM +0900, KOSAKI Motohiro wrote:
> 2011/4/24 Minchan Kim <minchan.kim@gmail.com>:
> > In putback_lru_page, unevictable page can be changed into evictable
> > 's one while we move it among lru. So we have checked it again and
> > rescued it. But we don't check PageActive, again. It could add
> > active page into inactive list so we can see the BUG in isolate_lru_pages.
> > (But I didn't see any report because I think it's very subtle)
> >
> > It could happen in race that zap_pte_range's mark_page_accessed and
> > putback_lru_page. It's subtle but could be possible.
> >
> > Note:
> > While I review the code, I found it. So it's not real report.
> >
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/vmscan.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index b3a569f..c0cd1aa 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -562,7 +562,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
> >  void putback_lru_page(struct page *page)
> >  {
> >        int lru;
> > -       int active = !!TestClearPageActive(page);
> > +       int active;
> >        int was_unevictable = PageUnevictable(page);
> >
> >        VM_BUG_ON(PageLRU(page));
> > @@ -571,6 +571,7 @@ redo:
> >        ClearPageUnevictable(page);
> >
> >        if (page_evictable(page, NULL)) {
> > +               active = !!TestClearPageActive(page);
> >                /*
> >                 * For evictable pages, we can use the cache.
> >                 * In event of a race, worst case is we end up with an
> > @@ -584,6 +585,7 @@ redo:
> >                 * Put unevictable pages directly on zone's unevictable
> >                 * list.
> >                 */
> > +               ClearPageActive(page);
> >                lru = LRU_UNEVICTABLE;
> >                add_page_to_unevictable_list(page);
> 
> I think we forgot 'goto redo' case. following patch is better?
> 
> ------------------------------------------------
>         if (page_evictable(page, NULL)) {
>                 /*
>                  * For evictable pages, we can use the cache.
>                  * In event of a race, worst case is we end up with an
>                  * unevictable page on [in]active list.
>                  * We know how to handle that.
>                  */
>                 lru = active + page_lru_base_type(page);
> +              if (active)
> +                   SetPageActive(page);
>                 lru_cache_add_lru(page, lru);

PageActive is reset by lru_cache_add_lru so it's meaningless.
BTW, please ignore this patch. :)
 
I think LRU status of isolated page cannot be changed.
Thanks for the review. 


-- 
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>