[PATCH]mm/compation.c: checking page in lru twice

[PATCH]mm/compation.c: checking page in lru twice

[Figo.zhang]

in isolate_migratepages() have check page in LRU twice, the next one
at _isolate_lru_page(). 

Signed-off-by: Figo.zhang <figo1802@gmail.com> 
---

mm/compaction.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 021a296..ac605cb 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -321,9 +321,6 @@ static unsigned long isolate_migratepages(struct zone *zone,
 			continue;
 		}
 
-		if (!PageLRU(page))
-			continue;
-
 		/*
 		 * PageLRU is set, and lru_lock excludes isolation,
 		 * splitting and collapsing (collapsing has already

[PATCH]mm/compation.c: checking page in lru twice

[Figo.zhang]

in isolate_migratepages() have check page in LRU twice, the next one
at _isolate_lru_page(). 

Signed-off-by: Figo.zhang <figo1802@gmail.com> 
---

mm/compaction.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 021a296..ac605cb 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -321,9 +321,6 @@ static unsigned long isolate_migratepages(struct zone *zone,
 			continue;
 		}
 
-		if (!PageLRU(page))
-			continue;
-
 		/*
 		 * PageLRU is set, and lru_lock excludes isolation,
 		 * splitting and collapsing (collapsing has already


--
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]mm/compation.c: checking page in lru twice

[Mel Gorman]

On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> 
> in isolate_migratepages() have check page in LRU twice, the next one
> at _isolate_lru_page(). 
> 
> Signed-off-by: Figo.zhang <figo1802@gmail.com> 

Not checking for PageLRU means that PageTransHuge() gets called
for each page. While the scanner is active and the lock released,
a transparent hugepage can be created and potentially we test
PageTransHuge() on a tail page. This will trigger a BUG if
CONFIG_DEBUG_VM is set.

Nacked-by: Mel Gorman <mel@csn.ul.ie>

> 
> mm/compaction.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 021a296..ac605cb 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -321,9 +321,6 @@ static unsigned long isolate_migratepages(struct zone *zone,
>  			continue;
>  		}
>  
> -		if (!PageLRU(page))
> -			continue;
> -
>  		/*
>  		 * PageLRU is set, and lru_lock excludes isolation,
>  		 * splitting and collapsing (collapsing has already
> 
> 

-- 
Mel Gorman
SUSE Labs

Re: [PATCH]mm/compation.c: checking page in lru twice

[Mel Gorman]

On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> 
> in isolate_migratepages() have check page in LRU twice, the next one
> at _isolate_lru_page(). 
> 
> Signed-off-by: Figo.zhang <figo1802@gmail.com> 

Not checking for PageLRU means that PageTransHuge() gets called
for each page. While the scanner is active and the lock released,
a transparent hugepage can be created and potentially we test
PageTransHuge() on a tail page. This will trigger a BUG if
CONFIG_DEBUG_VM is set.

Nacked-by: Mel Gorman <mel@csn.ul.ie>

> 
> mm/compaction.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 021a296..ac605cb 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -321,9 +321,6 @@ static unsigned long isolate_migratepages(struct zone *zone,
>  			continue;
>  		}
>  
> -		if (!PageLRU(page))
> -			continue;
> -
>  		/*
>  		 * PageLRU is set, and lru_lock excludes isolation,
>  		 * splitting and collapsing (collapsing has already
> 
> 

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
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]mm/compation.c: checking page in lru twice

[Andrea Arcangeli]

On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> 
> in isolate_migratepages() have check page in LRU twice, the next one
> at _isolate_lru_page(). 

hugetlbfs or any other compound page won't have PageLRU set and they
may go away from under us leading to compound_order not being reliable
if we remove the PageLRU check before compound_order. So we need to
verify the page is in LRU before running compound_order safely. And if
we hold the lru_lock, the page won't be isolated under us, and we know
it's not going to get splitted either.

We might use compound_trans_order but that's only reliable if run on
the head page so it's not so reliable, and so far it's only used by
memory-failure to "diminish" the risk of races in reading the compound
order, but it's not the best having to use compound_trans_order (and
memory-failure remains unsafe w.r.t to hugetlbfs being released during
hwpoisoning, so compound_trans_order might have to be improved for
it).

Re: [PATCH]mm/compation.c: checking page in lru twice

[Andrea Arcangeli]

On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> 
> in isolate_migratepages() have check page in LRU twice, the next one
> at _isolate_lru_page(). 

hugetlbfs or any other compound page won't have PageLRU set and they
may go away from under us leading to compound_order not being reliable
if we remove the PageLRU check before compound_order. So we need to
verify the page is in LRU before running compound_order safely. And if
we hold the lru_lock, the page won't be isolated under us, and we know
it's not going to get splitted either.

We might use compound_trans_order but that's only reliable if run on
the head page so it's not so reliable, and so far it's only used by
memory-failure to "diminish" the risk of races in reading the compound
order, but it's not the best having to use compound_trans_order (and
memory-failure remains unsafe w.r.t to hugetlbfs being released during
hwpoisoning, so compound_trans_order might have to be improved for
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/ .
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]mm/compation.c: checking page in lru twice

[Andrea Arcangeli]

On Fri, May 06, 2011 at 02:09:55PM +0100, Mel Gorman wrote:
> On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> > 
> > in isolate_migratepages() have check page in LRU twice, the next one
> > at _isolate_lru_page(). 
> > 
> > Signed-off-by: Figo.zhang <figo1802@gmail.com> 
> 
> Not checking for PageLRU means that PageTransHuge() gets called
> for each page. While the scanner is active and the lock released,
> a transparent hugepage can be created and potentially we test
> PageTransHuge() on a tail page. This will trigger a BUG if
> CONFIG_DEBUG_VM is set.

Agreed. The compound_order also would become unsafe even if it was
initially an head page (if it's a compound page not in lru). And
compound_trans_order isn't a solution either because we need to be
head for it to be safe like you said, better not having to use
compound_trans_order.

Re: [PATCH]mm/compation.c: checking page in lru twice

[Andrea Arcangeli]

On Fri, May 06, 2011 at 02:09:55PM +0100, Mel Gorman wrote:
> On Fri, May 06, 2011 at 07:32:46PM +0800, Figo.zhang wrote:
> > 
> > in isolate_migratepages() have check page in LRU twice, the next one
> > at _isolate_lru_page(). 
> > 
> > Signed-off-by: Figo.zhang <figo1802@gmail.com> 
> 
> Not checking for PageLRU means that PageTransHuge() gets called
> for each page. While the scanner is active and the lock released,
> a transparent hugepage can be created and potentially we test
> PageTransHuge() on a tail page. This will trigger a BUG if
> CONFIG_DEBUG_VM is set.

Agreed. The compound_order also would become unsafe even if it was
initially an head page (if it's a compound page not in lru). And
compound_trans_order isn't a solution either because we need to be
head for it to be safe like you said, better not having to use
compound_trans_order.

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