From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06ED4C35243 for ; Sun, 26 Jan 2020 02:44:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A6D482071A for ; Sun, 26 Jan 2020 02:44:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6D482071A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0BD096B0005; Sat, 25 Jan 2020 21:44:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 06E1D6B0006; Sat, 25 Jan 2020 21:44:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EC5146B0007; Sat, 25 Jan 2020 21:44:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0053.hostedemail.com [216.40.44.53]) by kanga.kvack.org (Postfix) with ESMTP id D69D16B0005 for ; Sat, 25 Jan 2020 21:44:46 -0500 (EST) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 9593A8248076 for ; Sun, 26 Jan 2020 02:44:46 +0000 (UTC) X-FDA: 76418242572.07.word72_63cef7310cf3d X-HE-Tag: word72_63cef7310cf3d X-Filterd-Recvd-Size: 7102 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Sun, 26 Jan 2020 02:44:45 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2020 18:44:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,364,1574150400"; d="scan'208";a="222946679" Received: from richard.sh.intel.com (HELO localhost) ([10.239.159.54]) by fmsmga008.fm.intel.com with ESMTP; 25 Jan 2020 18:44:17 -0800 Date: Sun, 26 Jan 2020 10:44:29 +0800 From: Wei Yang To: Anshuman Khandual Cc: Wei Yang , akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, rientjes@google.com Subject: Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page() Message-ID: <20200126024429.GA24582@richard> Reply-To: Wei Yang References: <20200120030415.15925-1-richardw.yang@linux.intel.com> <20200120030415.15925-2-richardw.yang@linux.intel.com> <8426f31b-606e-deca-acbe-dd59b193e113@arm.com> <20200120085530.GB18028@richard> <1c2e1cd6-5b65-79d7-f332-b866d5446c71@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1c2e1cd6-5b65-79d7-f332-b866d5446c71@arm.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jan 21, 2020 at 10:50:41AM +0530, Anshuman Khandual wrote: > > >On 01/20/2020 02:25 PM, Wei Yang wrote: >> On Mon, Jan 20, 2020 at 11:42:30AM +0530, Anshuman Khandual wrote: >>> >>> >>> On 01/20/2020 08:34 AM, Wei Yang wrote: >>>> This is a preparation to dump all reasons during check page. >>> >>> This really makes sense rather then just picking the reason from >>> the last "if" statement. >>> >>>> >>>> Signed-off-by: Wei Yang >>>> --- >>>> include/linux/mmdebug.h | 2 +- >>>> mm/debug.c | 11 ++++++----- >>>> mm/page_alloc.c | 2 +- >>>> 3 files changed, 8 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h >>>> index 2ad72d2c8cc5..f0a612db8bae 100644 >>>> --- a/include/linux/mmdebug.h >>>> +++ b/include/linux/mmdebug.h >>>> @@ -10,7 +10,7 @@ struct vm_area_struct; >>>> struct mm_struct; >>>> >>>> extern void dump_page(struct page *page, const char *reason); >>>> -extern void __dump_page(struct page *page, const char *reason); >>>> +extern void __dump_page(struct page *page, int num, const char **reason); >>>> void dump_vma(const struct vm_area_struct *vma); >>>> void dump_mm(const struct mm_struct *mm); >>>> >>>> diff --git a/mm/debug.c b/mm/debug.c >>>> index 0461df1207cb..a8ac6f951f9f 100644 >>>> --- a/mm/debug.c >>>> +++ b/mm/debug.c >>>> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = { >>>> {0, NULL} >>>> }; >>>> >>>> -void __dump_page(struct page *page, const char *reason) >>>> +void __dump_page(struct page *page, int num, const char **reason) >>>> { >>>> struct address_space *mapping; >>>> bool page_poisoned = PagePoisoned(page); >>>> - int mapcount; >>>> + int mapcount, i; >>>> >>>> /* >>>> * If struct page is poisoned don't access Page*() functions as that >>>> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason) >>>> sizeof(unsigned long), page, >>>> sizeof(struct page), false); >>>> >>>> - if (reason) >>>> - pr_warn("page dumped because: %s\n", reason); >>>> + pr_warn("page dumped because:\n"); >>>> + for (i = 0; i < num; i++) >>>> + pr_warn("\t%s\n", reason[i]); >>> >>> We should have a NR_BAD_PAGE_REASONS or something to cap this iteration >>> and also check reason[i] for non-NULL before trying to print the array. >>> There might be call sites like the following which will be problematic >>> otherwise. >>> >>> split_huge_page_to_list() -> dump_page(head, NULL) >>> >> >> You are right, I missed this case. >> >>>> >>>> #ifdef CONFIG_MEMCG >>>> if (!page_poisoned && page->mem_cgroup) >>> >>> While here, will it be better to move the above debug print block after >>> mem_cgroup block instead ? >>> >> >> Not sure, let's see whether others have some idea. >> >>>> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason) >>>> >>>> void dump_page(struct page *page, const char *reason) >>>> { >>>> - __dump_page(page, reason); >>>> + __dump_page(page, 1, &reason); >>>> dump_page_owner(page); >>>> } >>>> EXPORT_SYMBOL(dump_page); >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index d047bf7d8fd4..0cf6218aaba7 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -638,7 +638,7 @@ static void bad_page(struct page *page, const char *reason, >>>> >>>> pr_alert("BUG: Bad page state in process %s pfn:%05lx\n", >>>> current->comm, page_to_pfn(page)); >>>> - __dump_page(page, reason); >>>> + __dump_page(page, 1, &reason); >>>> bad_flags &= page->flags; >>>> if (bad_flags) >>>> pr_alert("bad because of flags: %#lx(%pGp)\n", >>>> >>> >>> Do we still need to have bad_flags ? After consolidating all reasons making >>> a page bad should not we just print page->flags unconditionally each time and >>> let the user decipher it instead. __dump_page() will print page->flags for >>> each case (atleast after the new patch from Vlastimil). AFAICS, the only >>> place currently consuming bad_flags is bad_page() which seems redundant after >>> first calling __dump_page(). >> >> Hmm... I don't catch this. The work in __dump_page() seems a little different >> from this one. Not sure we could remove it. > >Lets look at 'bad_flags' as it exists today without this series. > >It gets evaluated in free_pages_check_bad() and check_new_page_bad() before >being passed into bad_page(). All other call sites for bad_page() just pass >0 for 'bad_flags'. Now in bad_page(), we have > > __dump_page(page, reason); > bad_flags &= page->flags; > if (bad_flags) > pr_alert("bad because of flags: %#lx(%pGp)\n", > bad_flags, &bad_flags); > >Here, bad_flags &= page->flags will always be positive when 'reason' >is either > >"PAGE_FLAGS_CHECK_AT_FREE flag(s) set" > >or > >"PAGE_FLAGS_CHECK_AT_PREP flag set" > >The point here is we dont need to print bad_flags here as __dump_page() >already prints page->flags universally along with the "bad_reason" >after the following change. > >[1] https://patchwork.kernel.org/patch/11332035/ Hi, Anshuman I am preparing a patch to remove the bad_flags. While since the above change is not merged upstream yet, how can I wording the change log to point this change? Or I should wait till this one is merged? -- Wei Yang Help you, Help me