[PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

Changelog since V1
  o Remove unnecessary branch

When a page is freed with the PG_mlocked set, it is considered an unexpected
but recoverable situation. A counter records how often this event happens
but it is easy to miss that this event has occured at all. This patch warns
once when PG_mlocked is set to prompt debuggers to check the counter to
see how often it is happening.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 mm/page_alloc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index caa9268..97c8ecf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -495,8 +495,14 @@ static inline void free_page_mlock(struct page *page)
 static void free_page_mlock(struct page *page) { }
 #endif
 
-static inline int free_pages_check(struct page *page)
+static inline int free_pages_check(struct page *page, int wasMlocked)
 {
+	WARN_ONCE(wasMlocked, KERN_WARNING
+		"Page flag mlocked set for process %s at pfn:%05lx\n"
+		"page:%p flags:0x%lX\n",
+		current->comm, page_to_pfn(page),
+		page, page->flags|__PG_MLOCKED);
+
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |
 		(atomic_read(&page->_count) != 0) |
@@ -562,7 +568,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	kmemcheck_free_shadow(page, order);
 
 	for (i = 0 ; i < (1 << order) ; ++i)
-		bad += free_pages_check(page + i);
+		bad += free_pages_check(page + i, wasMlocked);
 	if (bad)
 		return;
 
@@ -1027,7 +1033,7 @@ static void free_hot_cold_page(struct page *page, int cold)
 
 	if (PageAnon(page))
 		page->mapping = NULL;
-	if (free_pages_check(page))
+	if (free_pages_check(page, wasMlocked))
 		return;
 
 	if (!PageHighMem(page)) {

[PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

Changelog since V1
  o Remove unnecessary branch

When a page is freed with the PG_mlocked set, it is considered an unexpected
but recoverable situation. A counter records how often this event happens
but it is easy to miss that this event has occured at all. This patch warns
once when PG_mlocked is set to prompt debuggers to check the counter to
see how often it is happening.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 mm/page_alloc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index caa9268..97c8ecf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -495,8 +495,14 @@ static inline void free_page_mlock(struct page *page)
 static void free_page_mlock(struct page *page) { }
 #endif
 
-static inline int free_pages_check(struct page *page)
+static inline int free_pages_check(struct page *page, int wasMlocked)
 {
+	WARN_ONCE(wasMlocked, KERN_WARNING
+		"Page flag mlocked set for process %s at pfn:%05lx\n"
+		"page:%p flags:0x%lX\n",
+		current->comm, page_to_pfn(page),
+		page, page->flags|__PG_MLOCKED);
+
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |
 		(atomic_read(&page->_count) != 0) |
@@ -562,7 +568,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	kmemcheck_free_shadow(page, order);
 
 	for (i = 0 ; i < (1 << order) ; ++i)
-		bad += free_pages_check(page + i);
+		bad += free_pages_check(page + i, wasMlocked);
 	if (bad)
 		return;
 
@@ -1027,7 +1033,7 @@ static void free_hot_cold_page(struct page *page, int cold)
 
 	if (PageAnon(page))
 		page->mapping = NULL;
-	if (free_pages_check(page))
+	if (free_pages_check(page, wasMlocked))
 		return;
 
 	if (!PageHighMem(page)) {

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

On Wed, 15 Jul 2009, Mel Gorman wrote:

> -static inline int free_pages_check(struct page *page)
> +static inline int free_pages_check(struct page *page, int wasMlocked)
>  {
> +	WARN_ONCE(wasMlocked, KERN_WARNING
> +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> +		"page:%p flags:0x%lX\n",
> +		current->comm, page_to_pfn(page),
> +		page, page->flags|__PG_MLOCKED);
> +
>  	if (unlikely(page_mapcount(page) |

There is already a free_page_mlocked() that is only called if the mlock
bit is set. Move it into there to avoid having to run two checks in the
hot codee path?

Also __free_pages_ok() now has a TestClearMlocked in the hot code path.
Would it be possible to get rid of the unconditional use of an atomic
operation? Just check the bit and clear it later in free_page_mlocked()?

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

On Wed, 15 Jul 2009, Mel Gorman wrote:

> -static inline int free_pages_check(struct page *page)
> +static inline int free_pages_check(struct page *page, int wasMlocked)
>  {
> +	WARN_ONCE(wasMlocked, KERN_WARNING
> +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> +		"page:%p flags:0x%lX\n",
> +		current->comm, page_to_pfn(page),
> +		page, page->flags|__PG_MLOCKED);
> +
>  	if (unlikely(page_mapcount(page) |

There is already a free_page_mlocked() that is only called if the mlock
bit is set. Move it into there to avoid having to run two checks in the
hot codee path?

Also __free_pages_ok() now has a TestClearMlocked in the hot code path.
Would it be possible to get rid of the unconditional use of an atomic
operation? Just check the bit and clear it later in free_page_mlocked()?

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

Hello Christoph,

On Wed, Jul 15, 2009 at 10:31:54AM -0400, Christoph Lameter wrote:
> On Wed, 15 Jul 2009, Mel Gorman wrote:
> 
> > -static inline int free_pages_check(struct page *page)
> > +static inline int free_pages_check(struct page *page, int wasMlocked)
> >  {
> > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:0x%lX\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> > +
> >  	if (unlikely(page_mapcount(page) |
> 
> There is already a free_page_mlocked() that is only called if the mlock
> bit is set. Move it into there to avoid having to run two checks in the
> hot codee path?
> 
> Also __free_pages_ok() now has a TestClearMlocked in the hot code path.
> Would it be possible to get rid of the unconditional use of an atomic
> operation? Just check the bit and clear it later in free_page_mlocked()?

That was initially done, but free_pages_check() checks for that flag
and did bad_page() on those mlocked ones.  Now, one idea was to not
check the mlocked flag at all in free_pages_check() as we handle it
differently anyway.  But I think we might still want to check for it
in tail-pages of higher order blocks.

And if you move that warning after free_pages_check(), the interesting
bits for the warning have been wiped already.

But we can get rid of the locked test-and-clear despite all the other
issues, patch below.

	Hannes

>From eee677ddea61b1331a3bd8e402a0d02437fe872a Mon Sep 17 00:00:00 2001
From: Johannes Weiner <hannes@cmpxchg.org>
Date: Wed, 15 Jul 2009 23:40:28 +0200
Subject: [patch] mm: non-atomic test-clear of PG_mlocked on free

By the time PG_mlocked is cleared in the page freeing path, nobody
else is looking at our page->flags anymore.

It is thus safe to make the test-and-clear non-atomic and thereby
removing an unnecessary and expensive operation from a hotpath.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/page-flags.h |   12 +++++++++---
 mm/page_alloc.c            |    4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e2e5ce5..10e6011 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -158,6 +158,9 @@ static inline int TestSetPage##uname(struct page *page)			\
 static inline int TestClearPage##uname(struct page *page)		\
 		{ return test_and_clear_bit(PG_##lname, &page->flags); }
 
+#define __TESTCLEARFLAG(uname, lname)					\
+static inline int __TestClearPage##uname(struct page *page)		\
+		{ return __test_and_clear_bit(PG_##lname, &page->flags); }
 
 #define PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname)		\
 	SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname)
@@ -184,6 +187,9 @@ static inline void __ClearPage##uname(struct page *page) {  }
 #define TESTCLEARFLAG_FALSE(uname)					\
 static inline int TestClearPage##uname(struct page *page) { return 0; }
 
+#define __TESTCLEARFLAG_FALSE(uname)					\
+static inline int __TestClearPage##uname(struct page *page) { return 0; }
+
 struct page;	/* forward declaration */
 
 TESTPAGEFLAG(Locked, locked) TESTSETFLAG(Locked, locked)
@@ -250,11 +256,11 @@ PAGEFLAG(Unevictable, unevictable) __CLEARPAGEFLAG(Unevictable, unevictable)
 #ifdef CONFIG_HAVE_MLOCKED_PAGE_BIT
 #define MLOCK_PAGES 1
 PAGEFLAG(Mlocked, mlocked) __CLEARPAGEFLAG(Mlocked, mlocked)
-	TESTSCFLAG(Mlocked, mlocked)
+	TESTSCFLAG(Mlocked, mlocked) __TESTCLEARFLAG(Mlocked, mlocked)
 #else
 #define MLOCK_PAGES 0
-PAGEFLAG_FALSE(Mlocked)
-	SETPAGEFLAG_NOOP(Mlocked) TESTCLEARFLAG_FALSE(Mlocked)
+PAGEFLAG_FALSE(Mlocked) SETPAGEFLAG_NOOP(Mlocked)
+	TESTCLEARFLAG_FALSE(Mlocked) __TESTCLEARFLAG_FALSE(Mlocked)
 #endif
 
 #ifdef CONFIG_IA64_UNCACHED_ALLOCATOR
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index caa9268..b0c8758 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -557,7 +557,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	unsigned long flags;
 	int i;
 	int bad = 0;
-	int wasMlocked = TestClearPageMlocked(page);
+	int wasMlocked = __TestClearPageMlocked(page);
 
 	kmemcheck_free_shadow(page, order);
 
@@ -1021,7 +1021,7 @@ static void free_hot_cold_page(struct page *page, int cold)
 	struct zone *zone = page_zone(page);
 	struct per_cpu_pages *pcp;
 	unsigned long flags;
-	int wasMlocked = TestClearPageMlocked(page);
+	int wasMlocked = __TestClearPageMlocked(page);
 
 	kmemcheck_free_shadow(page, 0);
 
-- 
1.6.3

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

Hello Christoph,

On Wed, Jul 15, 2009 at 10:31:54AM -0400, Christoph Lameter wrote:
> On Wed, 15 Jul 2009, Mel Gorman wrote:
> 
> > -static inline int free_pages_check(struct page *page)
> > +static inline int free_pages_check(struct page *page, int wasMlocked)
> >  {
> > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:0x%lX\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> > +
> >  	if (unlikely(page_mapcount(page) |
> 
> There is already a free_page_mlocked() that is only called if the mlock
> bit is set. Move it into there to avoid having to run two checks in the
> hot codee path?
> 
> Also __free_pages_ok() now has a TestClearMlocked in the hot code path.
> Would it be possible to get rid of the unconditional use of an atomic
> operation? Just check the bit and clear it later in free_page_mlocked()?

That was initially done, but free_pages_check() checks for that flag
and did bad_page() on those mlocked ones.  Now, one idea was to not
check the mlocked flag at all in free_pages_check() as we handle it
differently anyway.  But I think we might still want to check for it
in tail-pages of higher order blocks.

And if you move that warning after free_pages_check(), the interesting
bits for the warning have been wiped already.

But we can get rid of the locked test-and-clear despite all the other
issues, patch below.

	Hannes

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[KOSAKI Motohiro]

> From eee677ddea61b1331a3bd8e402a0d02437fe872a Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Wed, 15 Jul 2009 23:40:28 +0200
> Subject: [patch] mm: non-atomic test-clear of PG_mlocked on free
> 
> By the time PG_mlocked is cleared in the page freeing path, nobody
> else is looking at our page->flags anymore.
> 
> It is thus safe to make the test-and-clear non-atomic and thereby
> removing an unnecessary and expensive operation from a hotpath.

I like this patch. but can you please separate two following patches?
  - introduce __TESTCLEARFLAG()
  - non-atomic test-clear of PG_mlocked on free



> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  include/linux/page-flags.h |   12 +++++++++---
>  mm/page_alloc.c            |    4 ++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index e2e5ce5..10e6011 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -158,6 +158,9 @@ static inline int TestSetPage##uname(struct page *page)			\
>  static inline int TestClearPage##uname(struct page *page)		\
>  		{ return test_and_clear_bit(PG_##lname, &page->flags); }
>  
> +#define __TESTCLEARFLAG(uname, lname)					\
> +static inline int __TestClearPage##uname(struct page *page)		\
> +		{ return __test_and_clear_bit(PG_##lname, &page->flags); }
>  
>  #define PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname)		\
>  	SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname)
> @@ -184,6 +187,9 @@ static inline void __ClearPage##uname(struct page *page) {  }
>  #define TESTCLEARFLAG_FALSE(uname)					\
>  static inline int TestClearPage##uname(struct page *page) { return 0; }
>  
> +#define __TESTCLEARFLAG_FALSE(uname)					\
> +static inline int __TestClearPage##uname(struct page *page) { return 0; }
> +
>  struct page;	/* forward declaration */
>  
>  TESTPAGEFLAG(Locked, locked) TESTSETFLAG(Locked, locked)
> @@ -250,11 +256,11 @@ PAGEFLAG(Unevictable, unevictable) __CLEARPAGEFLAG(Unevictable, unevictable)
>  #ifdef CONFIG_HAVE_MLOCKED_PAGE_BIT
>  #define MLOCK_PAGES 1
>  PAGEFLAG(Mlocked, mlocked) __CLEARPAGEFLAG(Mlocked, mlocked)
> -	TESTSCFLAG(Mlocked, mlocked)
> +	TESTSCFLAG(Mlocked, mlocked) __TESTCLEARFLAG(Mlocked, mlocked)
>  #else
>  #define MLOCK_PAGES 0
> -PAGEFLAG_FALSE(Mlocked)
> -	SETPAGEFLAG_NOOP(Mlocked) TESTCLEARFLAG_FALSE(Mlocked)
> +PAGEFLAG_FALSE(Mlocked) SETPAGEFLAG_NOOP(Mlocked)
> +	TESTCLEARFLAG_FALSE(Mlocked) __TESTCLEARFLAG_FALSE(Mlocked)
>  #endif
>  
>  #ifdef CONFIG_IA64_UNCACHED_ALLOCATOR
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index caa9268..b0c8758 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -557,7 +557,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
>  	unsigned long flags;
>  	int i;
>  	int bad = 0;
> -	int wasMlocked = TestClearPageMlocked(page);
> +	int wasMlocked = __TestClearPageMlocked(page);
>  
>  	kmemcheck_free_shadow(page, order);
>  
> @@ -1021,7 +1021,7 @@ static void free_hot_cold_page(struct page *page, int cold)
>  	struct zone *zone = page_zone(page);
>  	struct per_cpu_pages *pcp;
>  	unsigned long flags;
> -	int wasMlocked = TestClearPageMlocked(page);
> +	int wasMlocked = __TestClearPageMlocked(page);
>  
>  	kmemcheck_free_shadow(page, 0);
>  
> -- 
> 1.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[KOSAKI Motohiro]

> From eee677ddea61b1331a3bd8e402a0d02437fe872a Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Wed, 15 Jul 2009 23:40:28 +0200
> Subject: [patch] mm: non-atomic test-clear of PG_mlocked on free
> 
> By the time PG_mlocked is cleared in the page freeing path, nobody
> else is looking at our page->flags anymore.
> 
> It is thus safe to make the test-and-clear non-atomic and thereby
> removing an unnecessary and expensive operation from a hotpath.

I like this patch. but can you please separate two following patches?
  - introduce __TESTCLEARFLAG()
  - non-atomic test-clear of PG_mlocked on free



> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  include/linux/page-flags.h |   12 +++++++++---
>  mm/page_alloc.c            |    4 ++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index e2e5ce5..10e6011 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -158,6 +158,9 @@ static inline int TestSetPage##uname(struct page *page)			\
>  static inline int TestClearPage##uname(struct page *page)		\
>  		{ return test_and_clear_bit(PG_##lname, &page->flags); }
>  
> +#define __TESTCLEARFLAG(uname, lname)					\
> +static inline int __TestClearPage##uname(struct page *page)		\
> +		{ return __test_and_clear_bit(PG_##lname, &page->flags); }
>  
>  #define PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname)		\
>  	SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname)
> @@ -184,6 +187,9 @@ static inline void __ClearPage##uname(struct page *page) {  }
>  #define TESTCLEARFLAG_FALSE(uname)					\
>  static inline int TestClearPage##uname(struct page *page) { return 0; }
>  
> +#define __TESTCLEARFLAG_FALSE(uname)					\
> +static inline int __TestClearPage##uname(struct page *page) { return 0; }
> +
>  struct page;	/* forward declaration */
>  
>  TESTPAGEFLAG(Locked, locked) TESTSETFLAG(Locked, locked)
> @@ -250,11 +256,11 @@ PAGEFLAG(Unevictable, unevictable) __CLEARPAGEFLAG(Unevictable, unevictable)
>  #ifdef CONFIG_HAVE_MLOCKED_PAGE_BIT
>  #define MLOCK_PAGES 1
>  PAGEFLAG(Mlocked, mlocked) __CLEARPAGEFLAG(Mlocked, mlocked)
> -	TESTSCFLAG(Mlocked, mlocked)
> +	TESTSCFLAG(Mlocked, mlocked) __TESTCLEARFLAG(Mlocked, mlocked)
>  #else
>  #define MLOCK_PAGES 0
> -PAGEFLAG_FALSE(Mlocked)
> -	SETPAGEFLAG_NOOP(Mlocked) TESTCLEARFLAG_FALSE(Mlocked)
> +PAGEFLAG_FALSE(Mlocked) SETPAGEFLAG_NOOP(Mlocked)
> +	TESTCLEARFLAG_FALSE(Mlocked) __TESTCLEARFLAG_FALSE(Mlocked)
>  #endif
>  
>  #ifdef CONFIG_IA64_UNCACHED_ALLOCATOR
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index caa9268..b0c8758 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -557,7 +557,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
>  	unsigned long flags;
>  	int i;
>  	int bad = 0;
> -	int wasMlocked = TestClearPageMlocked(page);
> +	int wasMlocked = __TestClearPageMlocked(page);
>  
>  	kmemcheck_free_shadow(page, order);
>  
> @@ -1021,7 +1021,7 @@ static void free_hot_cold_page(struct page *page, int cold)
>  	struct zone *zone = page_zone(page);
>  	struct per_cpu_pages *pcp;
>  	unsigned long flags;
> -	int wasMlocked = TestClearPageMlocked(page);
> +	int wasMlocked = __TestClearPageMlocked(page);
>  
>  	kmemcheck_free_shadow(page, 0);
>  
> -- 
> 1.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:

> I like this patch. but can you please separate two following patches?
>   - introduce __TESTCLEARFLAG()
>   - non-atomic test-clear of PG_mlocked on free

That would mean introducing the macro without any use case? It is fine the
way it is I think.

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:

> I like this patch. but can you please separate two following patches?
>   - introduce __TESTCLEARFLAG()
>   - non-atomic test-clear of PG_mlocked on free

That would mean introducing the macro without any use case? It is fine the
way it is I think.

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

On Thu, Jul 16, 2009 at 09:54:49AM -0400, Christoph Lameter wrote:
> On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:
> 
> > I like this patch. but can you please separate two following patches?
> >   - introduce __TESTCLEARFLAG()
> >   - non-atomic test-clear of PG_mlocked on free
> 
> That would mean introducing the macro without any use case? It is fine the
> way it is I think.

Yeah, it's borderline.  In any case, I have the split version here as
well.  Andrew, you choose :)

> Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

Thanks,

	Hannes

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

On Thu, Jul 16, 2009 at 09:54:49AM -0400, Christoph Lameter wrote:
> On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:
> 
> > I like this patch. but can you please separate two following patches?
> >   - introduce __TESTCLEARFLAG()
> >   - non-atomic test-clear of PG_mlocked on free
> 
> That would mean introducing the macro without any use case? It is fine the
> way it is I think.

Yeah, it's borderline.  In any case, I have the split version here as
well.  Andrew, you choose :)

> Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

Thanks,

	Hannes

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[KOSAKI Motohiro]

> On Thu, Jul 16, 2009 at 09:54:49AM -0400, Christoph Lameter wrote:
> > On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:
> > 
> > > I like this patch. but can you please separate two following patches?
> > >   - introduce __TESTCLEARFLAG()
> > >   - non-atomic test-clear of PG_mlocked on free
> > 
> > That would mean introducing the macro without any use case? It is fine the
> > way it is I think.
> 
> Yeah, it's borderline.  In any case, I have the split version here as
> well.  Andrew, you choose :)
> 
> > Reviewed-by: Christoph Lameter <cl@linux-foundation.org>
> 
> Thanks,

OK, I can agree with Christoph. you don't need change the patch.

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[KOSAKI Motohiro]

> On Thu, Jul 16, 2009 at 09:54:49AM -0400, Christoph Lameter wrote:
> > On Thu, 16 Jul 2009, KOSAKI Motohiro wrote:
> > 
> > > I like this patch. but can you please separate two following patches?
> > >   - introduce __TESTCLEARFLAG()
> > >   - non-atomic test-clear of PG_mlocked on free
> > 
> > That would mean introducing the macro without any use case? It is fine the
> > way it is I think.
> 
> Yeah, it's borderline.  In any case, I have the split version here as
> well.  Andrew, you choose :)
> 
> > Reviewed-by: Christoph Lameter <cl@linux-foundation.org>
> 
> Thanks,

OK, I can agree with Christoph. you don't need change the patch.

	Reviewed-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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Andrew Morton]

On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
Christoph Lameter <cl@linux-foundation.org> wrote:

> On Wed, 15 Jul 2009, Mel Gorman wrote:
> 
> > -static inline int free_pages_check(struct page *page)
> > +static inline int free_pages_check(struct page *page, int wasMlocked)
> >  {
> > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:0x%lX\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> > +
> >  	if (unlikely(page_mapcount(page) |
> 
> There is already a free_page_mlocked() that is only called if the mlock
> bit is set. Move it into there to avoid having to run two checks in the
> hot codee path?

Agreed.

This patch gratuitously adds hotpath overhead.  Moving the change to be
inside those preexisting wasMlocked tests will reduce its overhead a lot.

As it stands, I'm really doubting that the patch's utility is worth its
cost.

Also, it's a bit of a figleaf, but please consider making more use of
CONFIG_DEBUG_VM (see VM_BUG_ON()).

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Andrew Morton]

On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
Christoph Lameter <cl@linux-foundation.org> wrote:

> On Wed, 15 Jul 2009, Mel Gorman wrote:
> 
> > -static inline int free_pages_check(struct page *page)
> > +static inline int free_pages_check(struct page *page, int wasMlocked)
> >  {
> > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:0x%lX\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> > +
> >  	if (unlikely(page_mapcount(page) |
> 
> There is already a free_page_mlocked() that is only called if the mlock
> bit is set. Move it into there to avoid having to run two checks in the
> hot codee path?

Agreed.

This patch gratuitously adds hotpath overhead.  Moving the change to be
inside those preexisting wasMlocked tests will reduce its overhead a lot.

As it stands, I'm really doubting that the patch's utility is worth its
cost.

Also, it's a bit of a figleaf, but please consider making more use of
CONFIG_DEBUG_VM (see VM_BUG_ON()).

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> Christoph Lameter <cl@linux-foundation.org> wrote:
> 
> > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > 
> > > -static inline int free_pages_check(struct page *page)
> > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > >  {
> > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > +		"page:%p flags:0x%lX\n",
> > > +		current->comm, page_to_pfn(page),
> > > +		page, page->flags|__PG_MLOCKED);
> > > +
> > >  	if (unlikely(page_mapcount(page) |
> > 
> > There is already a free_page_mlocked() that is only called if the mlock
> > bit is set. Move it into there to avoid having to run two checks in the
> > hot codee path?
> 
> Agreed.
> 
> This patch gratuitously adds hotpath overhead.  Moving the change to be
> inside those preexisting wasMlocked tests will reduce its overhead a lot.
> 

It adds code duplication then, one of which is in a fast path.

> As it stands, I'm really doubting that the patch's utility is worth its
> cost.
> 

I'm happy to let this one drop. It seemed like it would be nice for debugging
while there are still corner cases where mlocked pages are getting freed
instead of torn down but we already account for that situation occuring. While
I think it'll be tricky to spot, it's probably preferable to having warnings
spew out onto dmesg.

> Also, it's a bit of a figleaf, but please consider making more use of
> CONFIG_DEBUG_VM (see VM_BUG_ON()).
> 

In this particular case, I doubted that DEBUG_VM would be set on
machines that were triggering the mlock corner case but yeah, this does
look more like a DEBUG_VM than a production thing.

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> Christoph Lameter <cl@linux-foundation.org> wrote:
> 
> > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > 
> > > -static inline int free_pages_check(struct page *page)
> > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > >  {
> > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > +		"page:%p flags:0x%lX\n",
> > > +		current->comm, page_to_pfn(page),
> > > +		page, page->flags|__PG_MLOCKED);
> > > +
> > >  	if (unlikely(page_mapcount(page) |
> > 
> > There is already a free_page_mlocked() that is only called if the mlock
> > bit is set. Move it into there to avoid having to run two checks in the
> > hot codee path?
> 
> Agreed.
> 
> This patch gratuitously adds hotpath overhead.  Moving the change to be
> inside those preexisting wasMlocked tests will reduce its overhead a lot.
> 

It adds code duplication then, one of which is in a fast path.

> As it stands, I'm really doubting that the patch's utility is worth its
> cost.
> 

I'm happy to let this one drop. It seemed like it would be nice for debugging
while there are still corner cases where mlocked pages are getting freed
instead of torn down but we already account for that situation occuring. While
I think it'll be tricky to spot, it's probably preferable to having warnings
spew out onto dmesg.

> Also, it's a bit of a figleaf, but please consider making more use of
> CONFIG_DEBUG_VM (see VM_BUG_ON()).
> 

In this particular case, I doubted that DEBUG_VM would be set on
machines that were triggering the mlock corner case but yeah, this does
look more like a DEBUG_VM than a production thing.

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

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Andrew Morton]

On Thu, 23 Jul 2009 11:29:39 +0100 Mel Gorman <mel@csn.ul.ie> wrote:

> On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> > On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> > Christoph Lameter <cl@linux-foundation.org> wrote:
> > 
> > > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > > 
> > > > -static inline int free_pages_check(struct page *page)
> > > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > > >  {
> > > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > > +		"page:%p flags:0x%lX\n",
> > > > +		current->comm, page_to_pfn(page),
> > > > +		page, page->flags|__PG_MLOCKED);
> > > > +
> > > >  	if (unlikely(page_mapcount(page) |
> > > 
> > > There is already a free_page_mlocked() that is only called if the mlock
> > > bit is set. Move it into there to avoid having to run two checks in the
> > > hot codee path?
> > 
> > Agreed.
> > 
> > This patch gratuitously adds hotpath overhead.  Moving the change to be
> > inside those preexisting wasMlocked tests will reduce its overhead a lot.
> > 
> 
> It adds code duplication then, one of which is in a fast path.
> 
> > As it stands, I'm really doubting that the patch's utility is worth its
> > cost.
> > 
> 
> I'm happy to let this one drop. It seemed like it would be nice for debugging
> while there are still corner cases where mlocked pages are getting freed
> instead of torn down but we already account for that situation occuring. While
> I think it'll be tricky to spot, it's probably preferable to having warnings
> spew out onto dmesg.

If we do in it the way which Christoph recommends, the additional
overhead is miniscule?

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Andrew Morton]

On Thu, 23 Jul 2009 11:29:39 +0100 Mel Gorman <mel@csn.ul.ie> wrote:

> On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> > On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> > Christoph Lameter <cl@linux-foundation.org> wrote:
> > 
> > > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > > 
> > > > -static inline int free_pages_check(struct page *page)
> > > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > > >  {
> > > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > > +		"page:%p flags:0x%lX\n",
> > > > +		current->comm, page_to_pfn(page),
> > > > +		page, page->flags|__PG_MLOCKED);
> > > > +
> > > >  	if (unlikely(page_mapcount(page) |
> > > 
> > > There is already a free_page_mlocked() that is only called if the mlock
> > > bit is set. Move it into there to avoid having to run two checks in the
> > > hot codee path?
> > 
> > Agreed.
> > 
> > This patch gratuitously adds hotpath overhead.  Moving the change to be
> > inside those preexisting wasMlocked tests will reduce its overhead a lot.
> > 
> 
> It adds code duplication then, one of which is in a fast path.
> 
> > As it stands, I'm really doubting that the patch's utility is worth its
> > cost.
> > 
> 
> I'm happy to let this one drop. It seemed like it would be nice for debugging
> while there are still corner cases where mlocked pages are getting freed
> instead of torn down but we already account for that situation occuring. While
> I think it'll be tricky to spot, it's probably preferable to having warnings
> spew out onto dmesg.

If we do in it the way which Christoph recommends, the additional
overhead is miniscule?


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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Thu, Jul 23, 2009 at 10:23:16AM -0700, Andrew Morton wrote:
> On Thu, 23 Jul 2009 11:29:39 +0100 Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> > > On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> > > Christoph Lameter <cl@linux-foundation.org> wrote:
> > > 
> > > > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > > > 
> > > > > -static inline int free_pages_check(struct page *page)
> > > > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > > > >  {
> > > > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > > > +		"page:%p flags:0x%lX\n",
> > > > > +		current->comm, page_to_pfn(page),
> > > > > +		page, page->flags|__PG_MLOCKED);
> > > > > +
> > > > >  	if (unlikely(page_mapcount(page) |
> > > > 
> > > > There is already a free_page_mlocked() that is only called if the mlock
> > > > bit is set. Move it into there to avoid having to run two checks in the
> > > > hot codee path?
> > > 
> > > Agreed.
> > > 
> > > This patch gratuitously adds hotpath overhead.  Moving the change to be
> > > inside those preexisting wasMlocked tests will reduce its overhead a lot.
> > > 
> > 
> > It adds code duplication then, one of which is in a fast path.
> > 
> > > As it stands, I'm really doubting that the patch's utility is worth its
> > > cost.
> > > 
> > 
> > I'm happy to let this one drop. It seemed like it would be nice for debugging
> > while there are still corner cases where mlocked pages are getting freed
> > instead of torn down but we already account for that situation occuring. While
> > I think it'll be tricky to spot, it's probably preferable to having warnings
> > spew out onto dmesg.
> 
> If we do in it the way which Christoph recommends, the additional
> overhead is miniscule?
> 

Yep, it should be. I misinterpreted what you said with doubting the patch's
utility. The cost as it was was too high rather than the warning itself was
useless. When moved to free_page_mlock(), patch looks like;

==== CUT HERE ====
mm: Warn once when a page is freed with PG_mlocked set V3

Changelog since V2
  o Move warning to free_page_mlock()
  o Use %#lx instead of 0x%lX in printf format string

Changelog since V1
  o Remove unnecessary branch

When a page is freed with the PG_mlocked set, it is considered an unexpected
but recoverable situation. A counter records how often this event happens
but it is easy to miss that this event has occured at all. This patch warns
once when PG_mlocked is set to prompt debuggers to check the counter to
see how often it is happening.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 mm/page_alloc.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b8283e8..d3d0707 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
  */
 static inline void free_page_mlock(struct page *page)
 {
+	WARN_ONCE(1, KERN_WARNING
+		"Page flag mlocked set for process %s at pfn:%05lx\n"
+		"page:%p flags:%#lx\n",
+		current->comm, page_to_pfn(page),
+		page, page->flags|__PG_MLOCKED);
 	__dec_zone_page_state(page, NR_MLOCK);
 	__count_vm_event(UNEVICTABLE_MLOCKFREED);
 }

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Thu, Jul 23, 2009 at 10:23:16AM -0700, Andrew Morton wrote:
> On Thu, 23 Jul 2009 11:29:39 +0100 Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> > > On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> > > Christoph Lameter <cl@linux-foundation.org> wrote:
> > > 
> > > > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > > > 
> > > > > -static inline int free_pages_check(struct page *page)
> > > > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > > > >  {
> > > > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > > > +		"page:%p flags:0x%lX\n",
> > > > > +		current->comm, page_to_pfn(page),
> > > > > +		page, page->flags|__PG_MLOCKED);
> > > > > +
> > > > >  	if (unlikely(page_mapcount(page) |
> > > > 
> > > > There is already a free_page_mlocked() that is only called if the mlock
> > > > bit is set. Move it into there to avoid having to run two checks in the
> > > > hot codee path?
> > > 
> > > Agreed.
> > > 
> > > This patch gratuitously adds hotpath overhead.  Moving the change to be
> > > inside those preexisting wasMlocked tests will reduce its overhead a lot.
> > > 
> > 
> > It adds code duplication then, one of which is in a fast path.
> > 
> > > As it stands, I'm really doubting that the patch's utility is worth its
> > > cost.
> > > 
> > 
> > I'm happy to let this one drop. It seemed like it would be nice for debugging
> > while there are still corner cases where mlocked pages are getting freed
> > instead of torn down but we already account for that situation occuring. While
> > I think it'll be tricky to spot, it's probably preferable to having warnings
> > spew out onto dmesg.
> 
> If we do in it the way which Christoph recommends, the additional
> overhead is miniscule?
> 

Yep, it should be. I misinterpreted what you said with doubting the patch's
utility. The cost as it was was too high rather than the warning itself was
useless. When moved to free_page_mlock(), patch looks like;

==== CUT HERE ====
mm: Warn once when a page is freed with PG_mlocked set V3

Changelog since V2
  o Move warning to free_page_mlock()
  o Use %#lx instead of 0x%lX in printf format string

Changelog since V1
  o Remove unnecessary branch

When a page is freed with the PG_mlocked set, it is considered an unexpected
but recoverable situation. A counter records how often this event happens
but it is easy to miss that this event has occured at all. This patch warns
once when PG_mlocked is set to prompt debuggers to check the counter to
see how often it is happening.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 mm/page_alloc.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b8283e8..d3d0707 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
  */
 static inline void free_page_mlock(struct page *page)
 {
+	WARN_ONCE(1, KERN_WARNING
+		"Page flag mlocked set for process %s at pfn:%05lx\n"
+		"page:%p flags:%#lx\n",
+		current->comm, page_to_pfn(page),
+		page, page->flags|__PG_MLOCKED);
 	__dec_zone_page_state(page, NR_MLOCK);
 	__count_vm_event(UNEVICTABLE_MLOCKFREED);
 }

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

Looks good.

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Christoph Lameter]

Looks good.

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

On Fri, Jul 24, 2009 at 11:36:56AM +0100, Mel Gorman wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b8283e8..d3d0707 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
>   */
>  static inline void free_page_mlock(struct page *page)
>  {
> +	WARN_ONCE(1, KERN_WARNING
> +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> +		"page:%p flags:%#lx\n",
> +		current->comm, page_to_pfn(page),
> +		page, page->flags|__PG_MLOCKED);

I don't think printing page->flags is all too useful after they have
been cleared by free_pages_check().

But it's probably a reasonable trade-off for not having it in the
fast-path.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Johannes Weiner]

On Fri, Jul 24, 2009 at 11:36:56AM +0100, Mel Gorman wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b8283e8..d3d0707 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
>   */
>  static inline void free_page_mlock(struct page *page)
>  {
> +	WARN_ONCE(1, KERN_WARNING
> +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> +		"page:%p flags:%#lx\n",
> +		current->comm, page_to_pfn(page),
> +		page, page->flags|__PG_MLOCKED);

I don't think printing page->flags is all too useful after they have
been cleared by free_pages_check().

But it's probably a reasonable trade-off for not having it in the
fast-path.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Fri, Jul 24, 2009 at 02:00:04PM +0200, Johannes Weiner wrote:
> On Fri, Jul 24, 2009 at 11:36:56AM +0100, Mel Gorman wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index b8283e8..d3d0707 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
> >   */
> >  static inline void free_page_mlock(struct page *page)
> >  {
> > +	WARN_ONCE(1, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:%#lx\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> 
> I don't think printing page->flags is all too useful after they have
> been cleared by free_pages_check().
> 

I considered that and was going to drop them. Then I remembered that the
node and zone linkages can also be encoded in the flags and conceivably they
could still be useful so I left it.

> But it's probably a reasonable trade-off for not having it in the
> fast-path.
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 

Thanks

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

Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set V2

[Mel Gorman]

On Fri, Jul 24, 2009 at 02:00:04PM +0200, Johannes Weiner wrote:
> On Fri, Jul 24, 2009 at 11:36:56AM +0100, Mel Gorman wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index b8283e8..d3d0707 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -488,6 +488,11 @@ static inline void __free_one_page(struct page *page,
> >   */
> >  static inline void free_page_mlock(struct page *page)
> >  {
> > +	WARN_ONCE(1, KERN_WARNING
> > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > +		"page:%p flags:%#lx\n",
> > +		current->comm, page_to_pfn(page),
> > +		page, page->flags|__PG_MLOCKED);
> 
> I don't think printing page->flags is all too useful after they have
> been cleared by free_pages_check().
> 

I considered that and was going to drop them. Then I remembered that the
node and zone linkages can also be encoded in the flags and conceivably they
could still be useful so I left it.

> But it's probably a reasonable trade-off for not having it in the
> fast-path.
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 

Thanks

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

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