From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail172.messagelabs.com (mail172.messagelabs.com [216.82.254.3]) by kanga.kvack.org (Postfix) with SMTP id 5F0598D0039 for ; Fri, 11 Feb 2011 05:49:16 -0500 (EST) Date: Fri, 11 Feb 2011 11:49:06 +0100 From: Andrea Arcangeli Subject: Re: [mmotm] BUG: Bad page state in process khugepaged ? Message-ID: <20110211104906.GE3347@random.random> References: <20110209151036.f24a36a6.kamezawa.hiroyu@jp.fujitsu.com> <20110209155001.0e369475.nishimura@mxp.nes.nec.co.jp> <20110209155246.69a7f3a1.kamezawa.hiroyu@jp.fujitsu.com> <20110209200728.GQ3347@random.random> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Hugh Dickins Cc: KAMEZAWA Hiroyuki , Daisuke Nishimura , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "akpm@linux-foundation.org" , "hannes@cmpxchg.org" On Thu, Feb 10, 2011 at 11:02:50PM -0800, Hugh Dickins wrote: > There is a separate little issue here, Andrea. > > Although we went to some trouble for bad_page() to take the page out > of circulation yet let the system continue, your VM_BUG_ON(!PageBuddy) > inside __ClearPageBuddy(page), from two callsites in bad_page(), is > turning it into a fatal error when CONFIG_DEBUG_VM. I see what you mean. Of course it is only a problem after bad_page already triggered.... but then it trigger an BUG_ON instead of only a bad_page. > You could that only MM developers switch CONFIG_DEBUG_VM=y, and they > would like bad_page() to be fatal; maybe, but if so we should do that > as an intentional patch, rather than as an unexpected side-effect ;) Fedora kernels are built with CONFIG_DEBUG_VM, all my kernels runs with CONFIG_DEBUG_VM too, so we want it to be as "production" as possible, and we don't want DEBUG_VM to decrease any reliability (only to increase it of course). > I noticed this a few days ago, but hadn't quite decided whether just to > remove the VM_BUG_ON, or move it to __ClearPageBuddy's third callsite, > or... doesn't matter much. > > I do also wonder if PageBuddy would better be _mapcount -something else: > if we've got a miscounted page (itself unlikely of course), there's a > chance that its _mapcount will be further decremented after it has been > freed: whereupon it will go from -1 to -2, PageBuddy at present. The > special avoidance of PageBuddy being that it can pull a whole block of > pages into misuse if its mistaken. Agreed. What about the below? ===== Subject: mm: PageBuddy cleanups From: Andrea Arcangeli bad_page could VM_BUG_ON(!PageBuddy(page)) inside __ClearPageBuddy(). I prefer to keep the VM_BUG_ON for safety and to add a if to solve it. Change the _mapcount value indicating PageBuddy from -2 to -1024 for more robusteness against page_mapcount() undeflows. Signed-off-by: Andrea Arcangeli Reported-by: Hugh Dickins --- diff --git a/include/linux/mm.h b/include/linux/mm.h index f6385fc..fa16ba0 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -402,16 +402,22 @@ static inline void init_page_count(struct page *page) /* * PageBuddy() indicate that the page is free and in the buddy system * (see mm/page_alloc.c). + * + * PAGE_BUDDY_MAPCOUNT_VALUE must be <= -2 but better not too close to + * -2 so that an underflow of the page_mapcount() won't be mistaken + * for a genuine PAGE_BUDDY_MAPCOUNT_VALUE. */ +#define PAGE_BUDDY_MAPCOUNT_VALUE (-1024*1024) + static inline int PageBuddy(struct page *page) { - return atomic_read(&page->_mapcount) == -2; + return atomic_read(&page->_mapcount) == PAGE_BUDDY_MAPCOUNT_VALUE; } static inline void __SetPageBuddy(struct page *page) { VM_BUG_ON(atomic_read(&page->_mapcount) != -1); - atomic_set(&page->_mapcount, -2); + atomic_set(&page->_mapcount, PAGE_BUDDY_MAPCOUNT_VALUE); } static inline void __ClearPageBuddy(struct page *page) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index a873e61..8aac134 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -286,7 +286,9 @@ static void bad_page(struct page *page) /* Don't complain about poisoned pages */ if (PageHWPoison(page)) { - __ClearPageBuddy(page); + /* __ClearPageBuddy VM_BUG_ON(!PageBuddy(page)) */ + if (PageBuddy(page)) + __ClearPageBuddy(page); return; } @@ -317,7 +319,8 @@ static void bad_page(struct page *page) dump_stack(); out: /* Leave bad fields for debug, except PageBuddy could make trouble */ - __ClearPageBuddy(page); + if (PageBuddy(page)) /* __ClearPageBuddy VM_BUG_ON(!PageBuddy(page)) */ + __ClearPageBuddy(page); add_taint(TAINT_BAD_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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org