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

[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@xxxxxxxxxxxxxxxxxxxx> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/