[PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

When racing between putback_lru_page and shmem_unlock happens,
progrom execution order is as follows, but clear_bit in processor #1
could be reordered right before spin_unlock of processor #1.
Then, the page would be stranded on the unevictable list.

spin_lock
SetPageLRU
spin_unlock
                                clear_bit(AS_UNEVICTABLE)
                                spin_lock
                                if PageLRU()
                                        if !test_bit(AS_UNEVICTABLE)
                                        	move evictable list
smp_mb
if !test_bit(AS_UNEVICTABLE)
        move evictable list
                                spin_unlock

But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
so this problem never happens. But it's a unexpected side effect and we should
solve this problem properly.

This patch adds a barrier after mapping_clear_unevictable.

side-note: I didn't meet this problem but just found during review.

Cc: Johannes Weiner <jweiner@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/shmem.c  |    1 +
 mm/vmscan.c |   11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2d35772..22cb349 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
 		user_shm_unlock(inode->i_size, user);
 		info->flags &= ~VM_LOCKED;
 		mapping_clear_unevictable(file->f_mapping);
+		smp_mb__after_clear_bit();
 		scan_mapping_unevictable_pages(file->f_mapping);
 	}
 	retval = 0;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 23256e8..4480f67 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -634,13 +634,14 @@ redo:
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-		 * When racing with an mlock clearing (page is
-		 * unlocked), make sure that if the other thread does
-		 * not observe our setting of PG_lru and fails
-		 * isolation, we see PG_mlocked cleared below and move
+		 * When racing with an mlock or AS_UNEVICTABLE clearing
+		 * (page is unlocked) make sure that if the other thread
+		 * does not observe our setting of PG_lru and fails
+		 * isolation/check_move_unevictable_page,
+		 * we see PG_mlocked/AS_UNEVICTABLE cleared below and move
 		 * the page back to the evictable list.
 		 *
-		 * The other side is TestClearPageMlocked().
+		 * The other side is TestClearPageMlocked() or shmem_lock().
 		 */
 		smp_mb();
 	}
-- 
1.7.4.1

[PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

When racing between putback_lru_page and shmem_unlock happens,
progrom execution order is as follows, but clear_bit in processor #1
could be reordered right before spin_unlock of processor #1.
Then, the page would be stranded on the unevictable list.

spin_lock
SetPageLRU
spin_unlock
                                clear_bit(AS_UNEVICTABLE)
                                spin_lock
                                if PageLRU()
                                        if !test_bit(AS_UNEVICTABLE)
                                        	move evictable list
smp_mb
if !test_bit(AS_UNEVICTABLE)
        move evictable list
                                spin_unlock

But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
so this problem never happens. But it's a unexpected side effect and we should
solve this problem properly.

This patch adds a barrier after mapping_clear_unevictable.

side-note: I didn't meet this problem but just found during review.

Cc: Johannes Weiner <jweiner@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/shmem.c  |    1 +
 mm/vmscan.c |   11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2d35772..22cb349 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
 		user_shm_unlock(inode->i_size, user);
 		info->flags &= ~VM_LOCKED;
 		mapping_clear_unevictable(file->f_mapping);
+		smp_mb__after_clear_bit();
 		scan_mapping_unevictable_pages(file->f_mapping);
 	}
 	retval = 0;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 23256e8..4480f67 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -634,13 +634,14 @@ redo:
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-		 * When racing with an mlock clearing (page is
-		 * unlocked), make sure that if the other thread does
-		 * not observe our setting of PG_lru and fails
-		 * isolation, we see PG_mlocked cleared below and move
+		 * When racing with an mlock or AS_UNEVICTABLE clearing
+		 * (page is unlocked) make sure that if the other thread
+		 * does not observe our setting of PG_lru and fails
+		 * isolation/check_move_unevictable_page,
+		 * we see PG_mlocked/AS_UNEVICTABLE cleared below and move
 		 * the page back to the evictable list.
 		 *
-		 * The other side is TestClearPageMlocked().
+		 * The other side is TestClearPageMlocked() or shmem_lock().
 		 */
 		smp_mb();
 	}
-- 
1.7.4.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] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

(2011/09/28 10:45), Minchan Kim wrote:
> When racing between putback_lru_page and shmem_unlock happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
> 
> spin_lock
> SetPageLRU
> spin_unlock
>                                 clear_bit(AS_UNEVICTABLE)
>                                 spin_lock
>                                 if PageLRU()
>                                         if !test_bit(AS_UNEVICTABLE)
>                                         	move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>         move evictable list
>                                 spin_unlock
> 
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.

Do we still need this after Hannes removes scan_mapping_unevictable_pages?


> 
> This patch adds a barrier after mapping_clear_unevictable.
> 
> side-note: I didn't meet this problem but just found during review.

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

(2011/09/28 10:45), Minchan Kim wrote:
> When racing between putback_lru_page and shmem_unlock happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
> 
> spin_lock
> SetPageLRU
> spin_unlock
>                                 clear_bit(AS_UNEVICTABLE)
>                                 spin_lock
>                                 if PageLRU()
>                                         if !test_bit(AS_UNEVICTABLE)
>                                         	move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>         move evictable list
>                                 spin_unlock
> 
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.

Do we still need this after Hannes removes scan_mapping_unevictable_pages?


> 
> This patch adds a barrier after mapping_clear_unevictable.
> 
> side-note: I didn't meet this problem but just found during review.


--
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] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 11:21:58AM +0900, KOSAKI Motohiro wrote:
> (2011/09/28 10:45), Minchan Kim wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.
> > 
> > spin_lock
> > SetPageLRU
> > spin_unlock
> >                                 clear_bit(AS_UNEVICTABLE)
> >                                 spin_lock
> >                                 if PageLRU()
> >                                         if !test_bit(AS_UNEVICTABLE)
> >                                         	move evictable list
> > smp_mb
> > if !test_bit(AS_UNEVICTABLE)
> >         move evictable list
> >                                 spin_unlock
> > 
> > But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> > it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> > so this problem never happens. But it's a unexpected side effect and we should
> > solve this problem properly.
> 
> Do we still need this after Hannes removes scan_mapping_unevictable_pages?
 
Hi KOSAKI,

What Hannes removes is scan_zone_unevictable_pages not scan_mapping_unevictable_pages.

-- 
Kinds regards,
Minchan Kim

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 11:21:58AM +0900, KOSAKI Motohiro wrote:
> (2011/09/28 10:45), Minchan Kim wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.
> > 
> > spin_lock
> > SetPageLRU
> > spin_unlock
> >                                 clear_bit(AS_UNEVICTABLE)
> >                                 spin_lock
> >                                 if PageLRU()
> >                                         if !test_bit(AS_UNEVICTABLE)
> >                                         	move evictable list
> > smp_mb
> > if !test_bit(AS_UNEVICTABLE)
> >         move evictable list
> >                                 spin_unlock
> > 
> > But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> > it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> > so this problem never happens. But it's a unexpected side effect and we should
> > solve this problem properly.
> 
> Do we still need this after Hannes removes scan_mapping_unevictable_pages?
 
Hi KOSAKI,

What Hannes removes is scan_zone_unevictable_pages not scan_mapping_unevictable_pages.

-- 
Kinds 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] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

(2011/09/28 11:25), Minchan Kim wrote:
> On Wed, Sep 28, 2011 at 11:21:58AM +0900, KOSAKI Motohiro wrote:
>> (2011/09/28 10:45), Minchan Kim wrote:
>>> When racing between putback_lru_page and shmem_unlock happens,
>>> progrom execution order is as follows, but clear_bit in processor #1
>>> could be reordered right before spin_unlock of processor #1.
>>> Then, the page would be stranded on the unevictable list.
>>>
>>> spin_lock
>>> SetPageLRU
>>> spin_unlock
>>>                                 clear_bit(AS_UNEVICTABLE)
>>>                                 spin_lock
>>>                                 if PageLRU()
>>>                                         if !test_bit(AS_UNEVICTABLE)
>>>                                         	move evictable list
>>> smp_mb
>>> if !test_bit(AS_UNEVICTABLE)
>>>         move evictable list
>>>                                 spin_unlock
>>>
>>> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
>>> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
>>> so this problem never happens. But it's a unexpected side effect and we should
>>> solve this problem properly.
>>
>> Do we still need this after Hannes removes scan_mapping_unevictable_pages?
>  
> Hi KOSAKI,
> 
> What Hannes removes is scan_zone_unevictable_pages not scan_mapping_unevictable_pages.
> 

Oops, you are right.

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

(2011/09/28 11:25), Minchan Kim wrote:
> On Wed, Sep 28, 2011 at 11:21:58AM +0900, KOSAKI Motohiro wrote:
>> (2011/09/28 10:45), Minchan Kim wrote:
>>> When racing between putback_lru_page and shmem_unlock happens,
>>> progrom execution order is as follows, but clear_bit in processor #1
>>> could be reordered right before spin_unlock of processor #1.
>>> Then, the page would be stranded on the unevictable list.
>>>
>>> spin_lock
>>> SetPageLRU
>>> spin_unlock
>>>                                 clear_bit(AS_UNEVICTABLE)
>>>                                 spin_lock
>>>                                 if PageLRU()
>>>                                         if !test_bit(AS_UNEVICTABLE)
>>>                                         	move evictable list
>>> smp_mb
>>> if !test_bit(AS_UNEVICTABLE)
>>>         move evictable list
>>>                                 spin_unlock
>>>
>>> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
>>> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
>>> so this problem never happens. But it's a unexpected side effect and we should
>>> solve this problem properly.
>>
>> Do we still need this after Hannes removes scan_mapping_unevictable_pages?
>  
> Hi KOSAKI,
> 
> What Hannes removes is scan_zone_unevictable_pages not scan_mapping_unevictable_pages.
> 

Oops, you are right.



--
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] vmscan: add barrier to prevent evictable page in unevictable list

[Johannes Weiner]

On Wed, Sep 28, 2011 at 10:45:30AM +0900, Minchan Kim wrote:
> When racing between putback_lru_page and shmem_unlock happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
> 
> spin_lock
> SetPageLRU
> spin_unlock
>                                 clear_bit(AS_UNEVICTABLE)
>                                 spin_lock
>                                 if PageLRU()
>                                         if !test_bit(AS_UNEVICTABLE)
>                                         	move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>         move evictable list
>                                 spin_unlock
> 
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.
> 
> This patch adds a barrier after mapping_clear_unevictable.
> 
> side-note: I didn't meet this problem but just found during review.
> 
> Cc: Johannes Weiner <jweiner@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/shmem.c  |    1 +
>  mm/vmscan.c |   11 ++++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 2d35772..22cb349 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
>  		user_shm_unlock(inode->i_size, user);
>  		info->flags &= ~VM_LOCKED;
>  		mapping_clear_unevictable(file->f_mapping);
> +		smp_mb__after_clear_bit();
>  		scan_mapping_unevictable_pages(file->f_mapping);

I always get nervous when I see undocumented barriers.  Maybe add a
teensy tiny comment here?

	/*
	 * Ensure that a racing putback_lru_page() can see
	 * the pages of this mapping are evictable when we
	 * skip them due to !PageLRU during the scan.
	 */

Or something like that.  Otherwise, nice catch :-)

Acked-by: Johannes Weiner <jweiner@redhat.com>

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Johannes Weiner]

On Wed, Sep 28, 2011 at 10:45:30AM +0900, Minchan Kim wrote:
> When racing between putback_lru_page and shmem_unlock happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
> 
> spin_lock
> SetPageLRU
> spin_unlock
>                                 clear_bit(AS_UNEVICTABLE)
>                                 spin_lock
>                                 if PageLRU()
>                                         if !test_bit(AS_UNEVICTABLE)
>                                         	move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>         move evictable list
>                                 spin_unlock
> 
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.
> 
> This patch adds a barrier after mapping_clear_unevictable.
> 
> side-note: I didn't meet this problem but just found during review.
> 
> Cc: Johannes Weiner <jweiner@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/shmem.c  |    1 +
>  mm/vmscan.c |   11 ++++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 2d35772..22cb349 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
>  		user_shm_unlock(inode->i_size, user);
>  		info->flags &= ~VM_LOCKED;
>  		mapping_clear_unevictable(file->f_mapping);
> +		smp_mb__after_clear_bit();
>  		scan_mapping_unevictable_pages(file->f_mapping);

I always get nervous when I see undocumented barriers.  Maybe add a
teensy tiny comment here?

	/*
	 * Ensure that a racing putback_lru_page() can see
	 * the pages of this mapping are evictable when we
	 * skip them due to !PageLRU during the scan.
	 */

Or something like that.  Otherwise, nice catch :-)

Acked-by: Johannes Weiner <jweiner@redhat.com>

--
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] vmscan: add barrier to prevent evictable page in unevictable list

[Lin Ming]

On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> When racing between putback_lru_page and shmem_unlock happens,

s/shmem_unlock/shmem_lock/

> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Lin Ming]

On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> When racing between putback_lru_page and shmem_unlock happens,

s/shmem_unlock/shmem_lock/

> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.

--
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] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 10:14:52AM +0200, Johannes Weiner wrote:
> On Wed, Sep 28, 2011 at 10:45:30AM +0900, Minchan Kim wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.
> > 
> > spin_lock
> > SetPageLRU
> > spin_unlock
> >                                 clear_bit(AS_UNEVICTABLE)
> >                                 spin_lock
> >                                 if PageLRU()
> >                                         if !test_bit(AS_UNEVICTABLE)
> >                                         	move evictable list
> > smp_mb
> > if !test_bit(AS_UNEVICTABLE)
> >         move evictable list
> >                                 spin_unlock
> > 
> > But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> > it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> > so this problem never happens. But it's a unexpected side effect and we should
> > solve this problem properly.
> > 
> > This patch adds a barrier after mapping_clear_unevictable.
> > 
> > side-note: I didn't meet this problem but just found during review.
> > 
> > Cc: Johannes Weiner <jweiner@redhat.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Mel Gorman <mel@csn.ul.ie>
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/shmem.c  |    1 +
> >  mm/vmscan.c |   11 ++++++-----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 2d35772..22cb349 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
> >  		user_shm_unlock(inode->i_size, user);
> >  		info->flags &= ~VM_LOCKED;
> >  		mapping_clear_unevictable(file->f_mapping);
> > +		smp_mb__after_clear_bit();
> >  		scan_mapping_unevictable_pages(file->f_mapping);
> 
> I always get nervous when I see undocumented barriers.  Maybe add a
> teensy tiny comment here?

Agree. I will try it.

> 
> 	/*
> 	 * Ensure that a racing putback_lru_page() can see
> 	 * the pages of this mapping are evictable when we
> 	 * skip them due to !PageLRU during the scan.
> 	 */
> 
> Or something like that.  Otherwise, nice catch :-)
> 
> Acked-by: Johannes Weiner <jweiner@redhat.com>

Thanks!

-- 
Kind regards,
Minchan Kim

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 10:14:52AM +0200, Johannes Weiner wrote:
> On Wed, Sep 28, 2011 at 10:45:30AM +0900, Minchan Kim wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.
> > 
> > spin_lock
> > SetPageLRU
> > spin_unlock
> >                                 clear_bit(AS_UNEVICTABLE)
> >                                 spin_lock
> >                                 if PageLRU()
> >                                         if !test_bit(AS_UNEVICTABLE)
> >                                         	move evictable list
> > smp_mb
> > if !test_bit(AS_UNEVICTABLE)
> >         move evictable list
> >                                 spin_unlock
> > 
> > But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> > it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> > so this problem never happens. But it's a unexpected side effect and we should
> > solve this problem properly.
> > 
> > This patch adds a barrier after mapping_clear_unevictable.
> > 
> > side-note: I didn't meet this problem but just found during review.
> > 
> > Cc: Johannes Weiner <jweiner@redhat.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Mel Gorman <mel@csn.ul.ie>
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/shmem.c  |    1 +
> >  mm/vmscan.c |   11 ++++++-----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 2d35772..22cb349 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1068,6 +1068,7 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
> >  		user_shm_unlock(inode->i_size, user);
> >  		info->flags &= ~VM_LOCKED;
> >  		mapping_clear_unevictable(file->f_mapping);
> > +		smp_mb__after_clear_bit();
> >  		scan_mapping_unevictable_pages(file->f_mapping);
> 
> I always get nervous when I see undocumented barriers.  Maybe add a
> teensy tiny comment here?

Agree. I will try it.

> 
> 	/*
> 	 * Ensure that a racing putback_lru_page() can see
> 	 * the pages of this mapping are evictable when we
> 	 * skip them due to !PageLRU during the scan.
> 	 */
> 
> Or something like that.  Otherwise, nice catch :-)
> 
> Acked-by: Johannes Weiner <jweiner@redhat.com>

Thanks!

-- 
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] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 11:04:05PM +0800, Lin Ming wrote:
> On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> 
> s/shmem_unlock/shmem_lock/

I did it intentionally for represent shmem_lock with user = 1, lock = 0.
If you think it makes others confusing, I will change in next version.
Thanks.

> 
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.

-- 
Kind regards,
Minchan Kim

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

On Wed, Sep 28, 2011 at 11:04:05PM +0800, Lin Ming wrote:
> On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > When racing between putback_lru_page and shmem_unlock happens,
> 
> s/shmem_unlock/shmem_lock/

I did it intentionally for represent shmem_lock with user = 1, lock = 0.
If you think it makes others confusing, I will change in next version.
Thanks.

> 
> > progrom execution order is as follows, but clear_bit in processor #1
> > could be reordered right before spin_unlock of processor #1.
> > Then, the page would be stranded on the unevictable list.

-- 
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] vmscan: add barrier to prevent evictable page in unevictable list

[Lin Ming]

On Thu, Sep 29, 2011 at 2:05 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> On Wed, Sep 28, 2011 at 11:04:05PM +0800, Lin Ming wrote:
>> On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > When racing between putback_lru_page and shmem_unlock happens,
>>
>> s/shmem_unlock/shmem_lock/
>
> I did it intentionally for represent shmem_lock with user = 1, lock = 0.
> If you think it makes others confusing, I will change in next version.
> Thanks.

I was confused. Now I understand.

>
>>
>> > progrom execution order is as follows, but clear_bit in processor #1
>> > could be reordered right before spin_unlock of processor #1.
>> > Then, the page would be stranded on the unevictable list.
>
> --
> Kind regards,
> Minchan Kim

Re: [PATCH] vmscan: add barrier to prevent evictable page in unevictable list

[Lin Ming]

On Thu, Sep 29, 2011 at 2:05 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> On Wed, Sep 28, 2011 at 11:04:05PM +0800, Lin Ming wrote:
>> On Wed, Sep 28, 2011 at 9:45 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > When racing between putback_lru_page and shmem_unlock happens,
>>
>> s/shmem_unlock/shmem_lock/
>
> I did it intentionally for represent shmem_lock with user = 1, lock = 0.
> If you think it makes others confusing, I will change in next version.
> Thanks.

I was confused. Now I understand.

>
>>
>> > progrom execution order is as follows, but clear_bit in processor #1
>> > could be reordered right before spin_unlock of processor #1.
>> > Then, the page would be stranded on the unevictable list.
>
> --
> 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>

[PATCH v2] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

When racing between putback_lru_page and shmem_lock with lock=0 happens,
progrom execution order is as follows, but clear_bit in processor #1
could be reordered right before spin_unlock of processor #1.
Then, the page would be stranded on the unevictable list.

spin_lock
SetPageLRU
spin_unlock
                                clear_bit(AS_UNEVICTABLE)
                                spin_lock
                                if PageLRU()
                                        if !test_bit(AS_UNEVICTABLE)
                                        	move evictable list
smp_mb
if !test_bit(AS_UNEVICTABLE)
        move evictable list
                                spin_unlock

But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
so this problem never happens. But it's a unexpected side effect and we should
solve this problem properly.

This patch adds a barrier after mapping_clear_unevictable.

side-note: I didn't meet this problem but just found during review.

Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Acked-by: Johannes Weiner <jweiner@redhat.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/shmem.c  |    6 ++++++
 mm/vmscan.c |   11 ++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2d35772..fa4fa6c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1068,6 +1068,12 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
 		user_shm_unlock(inode->i_size, user);
 		info->flags &= ~VM_LOCKED;
 		mapping_clear_unevictable(file->f_mapping);
+		/*
+		 * Ensure that a racing putback_lru_page() can see
+		 * the pages of this mapping are evictable when we
+		 * skip them due to !PageLRU during the scan.
+		 */
+		smp_mb__after_clear_bit();
 		scan_mapping_unevictable_pages(file->f_mapping);
 	}
 	retval = 0;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 23256e8..4480f67 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -634,13 +634,14 @@ redo:
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-		 * When racing with an mlock clearing (page is
-		 * unlocked), make sure that if the other thread does
-		 * not observe our setting of PG_lru and fails
-		 * isolation, we see PG_mlocked cleared below and move
+		 * When racing with an mlock or AS_UNEVICTABLE clearing
+		 * (page is unlocked) make sure that if the other thread
+		 * does not observe our setting of PG_lru and fails
+		 * isolation/check_move_unevictable_page,
+		 * we see PG_mlocked/AS_UNEVICTABLE cleared below and move
 		 * the page back to the evictable list.
 		 *
-		 * The other side is TestClearPageMlocked().
+		 * The other side is TestClearPageMlocked() or shmem_lock().
 		 */
 		smp_mb();
 	}
-- 
1.7.4.1

[PATCH v2] vmscan: add barrier to prevent evictable page in unevictable list

[Minchan Kim]

When racing between putback_lru_page and shmem_lock with lock=0 happens,
progrom execution order is as follows, but clear_bit in processor #1
could be reordered right before spin_unlock of processor #1.
Then, the page would be stranded on the unevictable list.

spin_lock
SetPageLRU
spin_unlock
                                clear_bit(AS_UNEVICTABLE)
                                spin_lock
                                if PageLRU()
                                        if !test_bit(AS_UNEVICTABLE)
                                        	move evictable list
smp_mb
if !test_bit(AS_UNEVICTABLE)
        move evictable list
                                spin_unlock

But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
so this problem never happens. But it's a unexpected side effect and we should
solve this problem properly.

This patch adds a barrier after mapping_clear_unevictable.

side-note: I didn't meet this problem but just found during review.

Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Acked-by: Johannes Weiner <jweiner@redhat.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/shmem.c  |    6 ++++++
 mm/vmscan.c |   11 ++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2d35772..fa4fa6c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1068,6 +1068,12 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
 		user_shm_unlock(inode->i_size, user);
 		info->flags &= ~VM_LOCKED;
 		mapping_clear_unevictable(file->f_mapping);
+		/*
+		 * Ensure that a racing putback_lru_page() can see
+		 * the pages of this mapping are evictable when we
+		 * skip them due to !PageLRU during the scan.
+		 */
+		smp_mb__after_clear_bit();
 		scan_mapping_unevictable_pages(file->f_mapping);
 	}
 	retval = 0;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 23256e8..4480f67 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -634,13 +634,14 @@ redo:
 		lru = LRU_UNEVICTABLE;
 		add_page_to_unevictable_list(page);
 		/*
-		 * When racing with an mlock clearing (page is
-		 * unlocked), make sure that if the other thread does
-		 * not observe our setting of PG_lru and fails
-		 * isolation, we see PG_mlocked cleared below and move
+		 * When racing with an mlock or AS_UNEVICTABLE clearing
+		 * (page is unlocked) make sure that if the other thread
+		 * does not observe our setting of PG_lru and fails
+		 * isolation/check_move_unevictable_page,
+		 * we see PG_mlocked/AS_UNEVICTABLE cleared below and move
 		 * the page back to the evictable list.
 		 *
-		 * The other side is TestClearPageMlocked().
+		 * The other side is TestClearPageMlocked() or shmem_lock().
 		 */
 		smp_mb();
 	}
-- 
1.7.4.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 v2] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

2011/9/29 Minchan Kim <minchan.kim@gmail.com>:
> When racing between putback_lru_page and shmem_lock with lock=0 happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
>
> spin_lock
> SetPageLRU
> spin_unlock
>                                clear_bit(AS_UNEVICTABLE)
>                                spin_lock
>                                if PageLRU()
>                                        if !test_bit(AS_UNEVICTABLE)
>                                                move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>        move evictable list
>                                spin_unlock
>
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.
>
> This patch adds a barrier after mapping_clear_unevictable.
>
> side-note: I didn't meet this problem but just found during review.
>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Acked-by: Johannes Weiner <jweiner@redhat.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>

  Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH v2] vmscan: add barrier to prevent evictable page in unevictable list

[KOSAKI Motohiro]

2011/9/29 Minchan Kim <minchan.kim@gmail.com>:
> When racing between putback_lru_page and shmem_lock with lock=0 happens,
> progrom execution order is as follows, but clear_bit in processor #1
> could be reordered right before spin_unlock of processor #1.
> Then, the page would be stranded on the unevictable list.
>
> spin_lock
> SetPageLRU
> spin_unlock
>                                clear_bit(AS_UNEVICTABLE)
>                                spin_lock
>                                if PageLRU()
>                                        if !test_bit(AS_UNEVICTABLE)
>                                                move evictable list
> smp_mb
> if !test_bit(AS_UNEVICTABLE)
>        move evictable list
>                                spin_unlock
>
> But, pagevec_lookup in scan_mapping_unevictable_pages has rcu_read_[un]lock so
> it could protect reordering before reaching test_bit(AS_UNEVICTABLE) on processor #1
> so this problem never happens. But it's a unexpected side effect and we should
> solve this problem properly.
>
> This patch adds a barrier after mapping_clear_unevictable.
>
> side-note: I didn't meet this problem but just found during review.
>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Acked-by: Johannes Weiner <jweiner@redhat.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>

  Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

--
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>