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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 00D20C433F5 for ; Wed, 25 May 2022 09:38:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 695448D0003; Wed, 25 May 2022 05:38:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 644AD8D0002; Wed, 25 May 2022 05:38:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 52E578D0003; Wed, 25 May 2022 05:38:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 416488D0002 for ; Wed, 25 May 2022 05:38:11 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 0E70A348BD for ; Wed, 25 May 2022 09:38:11 +0000 (UTC) X-FDA: 79503764382.01.4F3B597 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf02.hostedemail.com (Postfix) with ESMTP id 5CD5680008 for ; Wed, 25 May 2022 09:38:07 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3D43C1FB; Wed, 25 May 2022 02:38:09 -0700 (PDT) Received: from [10.163.37.70] (unknown [10.163.37.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 079CD3F70D; Wed, 25 May 2022 02:38:06 -0700 (PDT) Message-ID: <7e692769-daca-65cb-2263-c4be9b99ed38@arm.com> Date: Wed, 25 May 2022 15:08:03 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span() Content-Language: en-US To: Oscar Salvador Cc: linux-mm@kvack.org, David Hildenbrand , Andrew Morton , linux-kernel@vger.kernel.org References: <20220525033910.3781764-1-anshuman.khandual@arm.com> From: Anshuman Khandual In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 5CD5680008 X-Stat-Signature: rdb5wu8ns3fm7xjywmcawk1eceib7ctf X-Rspam-User: Authentication-Results: imf02.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf02.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com X-HE-Tag: 1653471487-798148 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 5/25/22 13:55, Oscar Salvador wrote: > On Wed, May 25, 2022 at 09:09:09AM +0530, Anshuman Khandual wrote: >> In check_pfn_span(), a 'reason' string is being used to recreate the caller >> function name, while printing the warning message. It is really unnecessary >> as the warning message could just be printed inside the caller depending on >> the return code. Currentlyy there are just two callers for check_pfn_span() > Currently Ahh, will fix. >> i.e __add_pages() and __remove_pages(). Let's clean this up. >> >> Cc: David Hildenbrand >> Cc: Andrew Morton >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual > > One could argue if this is really a cleanup. > I kind of agree that the "reason" thingy is a bit shaky, but instead of having a > single place where we call WARN(), we now do have two. check_pfn_span() is basically ensuring minimum alignment for both pfn and nr_pages. Resulting error message when this alignment check does not hold true, is caller specific than not. > >> --- >> mm/memory_hotplug.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index 416b38ca8def..9b3d7295ef93 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -220,8 +220,7 @@ static void release_memory_resource(struct resource *res) >> kfree(res); >> } >> >> -static int check_pfn_span(unsigned long pfn, unsigned long nr_pages, >> - const char *reason) >> +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages) >> { >> /* >> * Disallow all operations smaller than a sub-section and only >> @@ -238,12 +237,8 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages, >> min_align = PAGES_PER_SUBSECTION; >> else >> min_align = PAGES_PER_SECTION; >> - if (!IS_ALIGNED(pfn, min_align) >> - || !IS_ALIGNED(nr_pages, min_align)) { >> - WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n", >> - reason, pfn, pfn + nr_pages - 1); >> + if (!IS_ALIGNED(pfn, min_align) || !IS_ALIGNED(nr_pages, min_align)) >> return -EINVAL; >> - } >> return 0; >> } >> >> @@ -320,9 +315,11 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, >> altmap->alloc = 0; >> } >> >> - err = check_pfn_span(pfn, nr_pages, "add"); >> - if (err) >> + err = check_pfn_span(pfn, nr_pages); >> + if (err) { >> + WARN(1, "Misaligned %s start: %#lx end: #%lx\n", __func__, pfn, pfn + nr_pages - 1); >> return err; >> + } > > If you want to further clean this up, I would just do > > if (check_pfn_span()) { > WARN(....) > return -EINVAL; > } > > here as we do in __remove_pages(). check_pfn_span() can either return 0 or -EINVAL, > so I think it is fine. Sure, will change.