All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mel Gorman <mel@csn.ul.ie>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Lee Schermerhorn <Lee.Schermerhorn@hp.com>,
	Jiri Slaby <jirislaby@gmail.com>,
	Maxim Levitsky <maximlevitsky@gmail.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Lameter <cl@linux-foundation.org>,
	Pekka Enberg <penberg@cs.helsinki.fi>,
	Rik van Riel <riel@redhat.com>
Subject: Re: BUG: Bad page state [was: Strange oopses in 2.6.30]
Date: Mon, 29 Jun 2009 11:37:06 +0100	[thread overview]
Message-ID: <20090629103706.GA5065@csn.ul.ie> (raw)
In-Reply-To: <20090629101819.GA2052@cmpxchg.org>

On Mon, Jun 29, 2009 at 12:18:19PM +0200, Johannes Weiner wrote:
> On Mon, Jun 29, 2009 at 09:41:14AM +0100, Mel Gorman wrote:
> > I see the unconditionoal clearing of the flag was merged since but even
> > that might be too heavy handed as we are making a locked bit operation
> > on every page free. That's unfortunate overhead to incur on every page
> > free to handle a situation that should not be occurring at all.
> 
> Linus was probably quick to merge it as istr several people hitting
> bad_page() triggering.  We should get rid of the locked op, I was just
> not 100% sure and chose the safer version.
> 

And I have no problem with the decision. Leaving it as it was would have
caused a storm of bug reports, all similar.

> > > > +		WARN_ONCE(1, KERN_WARNING
> > > > +			"Sloppy page flags set process %s at pfn:%05lx\n"
> > > > +			"page:%p flags:%p\n",
> > > > +			current->comm, page_to_pfn(page),
> > > > +			page, (void *)page->flags);
> [...]
> > > > +		page->flags &= ~PAGE_FLAGS_WARN_AT_FREE;
> > > > +	}
> > > > +
> > > >  	if (unlikely(page_mapcount(page) |
> > > >  		(page->mapping != NULL)  |
> > > >  		(atomic_read(&page->_count) != 0) |
> > > 
> > > Howerver, I like this patch concept. this warning is useful and meaningful IMHO.
> > > 
> > 
> > This is a version that is based on top of current mainline that just
> > displays the warning. However, I think we should consider changing
> > TestClearPageMlocked() back to PageMlocked() and only clearing the flags
> > when the unusual condition is encountered.
> 
> I have a diff at home that makes this an unlocked
> __TestClearPageMlocked(), would you be okay with this?
> 

It'd be an improvement for sure. Post it and I'll take a look.

My preference is still to clear the flag only when found to be erroneously set
and print a warning once but that's because it was the patch I put together
so I'm biased :)

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

WARNING: multiple messages have this Message-ID (diff)
From: Mel Gorman <mel@csn.ul.ie>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Lee Schermerhorn <Lee.Schermerhorn@hp.com>,
	Jiri Slaby <jirislaby@gmail.com>,
	Maxim Levitsky <maximlevitsky@gmail.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Lameter <cl@linux-foundation.org>,
	Pekka Enberg <penberg@cs.helsinki.fi>,
	Rik van Riel <riel@redhat.com>
Subject: Re: BUG: Bad page state [was: Strange oopses in 2.6.30]
Date: Mon, 29 Jun 2009 11:37:06 +0100	[thread overview]
Message-ID: <20090629103706.GA5065@csn.ul.ie> (raw)
In-Reply-To: <20090629101819.GA2052@cmpxchg.org>

On Mon, Jun 29, 2009 at 12:18:19PM +0200, Johannes Weiner wrote:
> On Mon, Jun 29, 2009 at 09:41:14AM +0100, Mel Gorman wrote:
> > I see the unconditionoal clearing of the flag was merged since but even
> > that might be too heavy handed as we are making a locked bit operation
> > on every page free. That's unfortunate overhead to incur on every page
> > free to handle a situation that should not be occurring at all.
> 
> Linus was probably quick to merge it as istr several people hitting
> bad_page() triggering.  We should get rid of the locked op, I was just
> not 100% sure and chose the safer version.
> 

And I have no problem with the decision. Leaving it as it was would have
caused a storm of bug reports, all similar.

> > > > +		WARN_ONCE(1, KERN_WARNING
> > > > +			"Sloppy page flags set process %s at pfn:%05lx\n"
> > > > +			"page:%p flags:%p\n",
> > > > +			current->comm, page_to_pfn(page),
> > > > +			page, (void *)page->flags);
> [...]
> > > > +		page->flags &= ~PAGE_FLAGS_WARN_AT_FREE;
> > > > +	}
> > > > +
> > > >  	if (unlikely(page_mapcount(page) |
> > > >  		(page->mapping != NULL)  |
> > > >  		(atomic_read(&page->_count) != 0) |
> > > 
> > > Howerver, I like this patch concept. this warning is useful and meaningful IMHO.
> > > 
> > 
> > This is a version that is based on top of current mainline that just
> > displays the warning. However, I think we should consider changing
> > TestClearPageMlocked() back to PageMlocked() and only clearing the flags
> > when the unusual condition is encountered.
> 
> I have a diff at home that makes this an unlocked
> __TestClearPageMlocked(), would you be okay with this?
> 

It'd be an improvement for sure. Post it and I'll take a look.

My preference is still to clear the flag only when found to be erroneously set
and print a warning once but that's because it was the patch I put together
so I'm biased :)

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

  reply	other threads:[~2009-06-29 10:37 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-19 21:48 Strange oopses in 2.6.30 Maxim Levitsky
2009-06-20 14:08 ` BUG] " Maxim Levitsky
2009-06-20 15:27   ` BUG: Bad page state [was: Strange oopses in 2.6.30] Jiri Slaby
2009-06-20 15:27     ` Jiri Slaby
2009-06-20 15:44     ` Maxim Levitsky
2009-06-20 15:44       ` Maxim Levitsky
2009-06-22  2:39     ` KOSAKI Motohiro
2009-06-22  2:39       ` KOSAKI Motohiro
2009-06-22  7:42       ` Pekka Enberg
2009-06-22  7:42         ` Pekka Enberg
2009-06-22 20:55         ` Mel Gorman
2009-06-22 20:55           ` Mel Gorman
2009-06-22  9:16       ` Mel Gorman
2009-06-22  9:16         ` Mel Gorman
2009-06-22 16:02         ` Lee Schermerhorn
2009-06-22 16:02           ` Lee Schermerhorn
2009-06-22 20:53           ` Mel Gorman
2009-06-22 20:53             ` Mel Gorman
2009-06-23 11:11             ` KOSAKI Motohiro
2009-06-23 11:11               ` KOSAKI Motohiro
2009-06-29  8:41               ` Mel Gorman
2009-06-29  8:41                 ` Mel Gorman
2009-06-29 10:18                 ` Johannes Weiner
2009-06-29 10:18                   ` Johannes Weiner
2009-06-29 10:37                   ` Mel Gorman [this message]
2009-06-29 10:37                     ` Mel Gorman
2009-06-30  0:31                 ` KOSAKI Motohiro
2009-06-30  0:31                   ` KOSAKI Motohiro
2009-06-30 15:11                   ` Mel Gorman
2009-06-30 15:11                     ` Mel Gorman
2009-06-30 16:34                   ` Mel Gorman
2009-06-30 16:34                     ` Mel Gorman
2009-06-30 23:45                     ` KOSAKI Motohiro
2009-06-30 23:45                       ` KOSAKI Motohiro
2009-06-23 11:04         ` KOSAKI Motohiro
2009-06-23 11:04           ` KOSAKI Motohiro
2009-06-20 15:40   ` BUG] Strange oopses in 2.6.30 Hugh Dickins
2009-06-20 19:26     ` Mel Gorman
2009-06-20 23:01     ` Maxim Levitsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090629103706.GA5065@csn.ul.ie \
    --to=mel@csn.ul.ie \
    --cc=Lee.Schermerhorn@hp.com \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux-foundation.org \
    --cc=hannes@cmpxchg.org \
    --cc=jirislaby@gmail.com \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=maximlevitsky@gmail.com \
    --cc=penberg@cs.helsinki.fi \
    --cc=riel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.