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=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 8DC13C33CAF for ; Mon, 20 Jan 2020 00:33:53 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5847E20678 for ; Mon, 20 Jan 2020 00:33:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5847E20678 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 05DAB6B0584; Sun, 19 Jan 2020 19:33:53 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 00C8E6B0593; Sun, 19 Jan 2020 19:33:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E63C76B0594; Sun, 19 Jan 2020 19:33:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0020.hostedemail.com [216.40.44.20]) by kanga.kvack.org (Postfix) with ESMTP id D1ED06B0584 for ; Sun, 19 Jan 2020 19:33:52 -0500 (EST) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with SMTP id 811DA180AD80F for ; Mon, 20 Jan 2020 00:33:52 +0000 (UTC) X-FDA: 76396139904.27.wound47_3edd6287a8f52 X-HE-Tag: wound47_3edd6287a8f52 X-Filterd-Recvd-Size: 4855 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf43.hostedemail.com (Postfix) with ESMTP for ; Mon, 20 Jan 2020 00:33:51 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jan 2020 16:33:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,340,1574150400"; d="scan'208";a="374184346" Received: from richard.sh.intel.com (HELO localhost) ([10.239.159.54]) by orsmga004.jf.intel.com with ESMTP; 19 Jan 2020 16:33:48 -0800 Date: Mon, 20 Jan 2020 08:33:59 +0800 From: Wei Yang To: David Rientjes Cc: Wei Yang , akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/4] mm/page_alloc.c: extract commom part to check page Message-ID: <20200120003359.GB26292@richard> Reply-To: Wei Yang References: <20200119131408.23247-1-richardw.yang@linux.intel.com> <20200119131408.23247-2-richardw.yang@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Sun, Jan 19, 2020 at 02:06:18PM -0800, David Rientjes wrote: >On Sun, 19 Jan 2020, Wei Yang wrote: > >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index d047bf7d8fd4..8cd06729169f 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page, >> return true; >> } >> >> -static void free_pages_check_bad(struct page *page) >> +static inline const char *__check_page(struct page *page) >> { >> - const char *bad_reason; >> - unsigned long bad_flags; >> - >> - bad_reason = NULL; >> - bad_flags = 0; >> + const char *bad_reason = NULL; >> >> if (unlikely(atomic_read(&page->_mapcount) != -1)) >> bad_reason = "nonzero mapcount"; >> @@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page) >> bad_reason = "non-NULL mapping"; >> if (unlikely(page_ref_count(page) != 0)) >> bad_reason = "nonzero _refcount"; >> - if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { >> - bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; >> - bad_flags = PAGE_FLAGS_CHECK_AT_FREE; >> - } >> #ifdef CONFIG_MEMCG >> if (unlikely(page->mem_cgroup)) >> bad_reason = "page still charged to cgroup"; >> #endif >> + return bad_reason; >> +} >> + >> +static void free_pages_check_bad(struct page *page) >> +{ >> + const char *bad_reason = NULL; >> + unsigned long bad_flags = 0; >> + >> + bad_reason = __check_page(page); >> + if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { >> + bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; >> + bad_flags = PAGE_FLAGS_CHECK_AT_FREE; >> + } >> bad_page(page, bad_reason, bad_flags); >> } >> >> @@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page) >> const char *bad_reason = NULL; >> unsigned long bad_flags = 0; >> >> - if (unlikely(atomic_read(&page->_mapcount) != -1)) >> - bad_reason = "nonzero mapcount"; >> - if (unlikely(page->mapping != NULL)) >> - bad_reason = "non-NULL mapping"; >> - if (unlikely(page_ref_count(page) != 0)) >> - bad_reason = "nonzero _refcount"; >> + bad_reason = __check_page(page); >> if (unlikely(page->flags & __PG_HWPOISON)) { >> bad_reason = "HWPoisoned (hardware-corrupted)"; >> bad_flags = __PG_HWPOISON; >> @@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page) >> bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set"; >> bad_flags = PAGE_FLAGS_CHECK_AT_PREP; >> } >> -#ifdef CONFIG_MEMCG >> - if (unlikely(page->mem_cgroup)) >> - bad_reason = "page still charged to cgroup"; >> -#endif >> bad_page(page, bad_reason, bad_flags); >> } >> > >I think this is compounding a previous problem in these functions: these >are all "if" clauses, not "else if" clauses so they are presumably ordered >based on least significant to most significant (we only see the last >bad_reason that we find). For the page->mem_cgroup check, this leaves >bad_flags set but it doesn't match bad_reason. > I have thought about this. And curious about the order of those reasons. >Could you instead fix the problem with these functions so that we actually >list *all* the problems with the page rather than only the last >conditional that is true? Sure, thanks for the suggestion. -- Wei Yang Help you, Help me