Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Wed, 14 Jun 2023 11:28:32 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From:   "Yajun Deng" <yajun.deng@linux.dev>
Message-ID: <ab067588892217b6ee6ce759bd569b12@linux.dev>
Subject: Re: [PATCH] mm/mm_init.c: remove spinlock in early_pfn_to_nid()
To:     "Greg KH" <gregkh@linuxfoundation.org>
Cc:     rafael@kernel.org, rppt@kernel.org, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
In-Reply-To: <2023061431-litigate-upchuck-7ed1@gregkh>
References: <2023061431-litigate-upchuck-7ed1@gregkh>
 <20230614110324.3839354-1-yajun.deng@linux.dev>
Precedence: bulk

June 14, 2023 7:09 PM, "Greg KH" <gregkh@linuxfoundation.org> wrote:=0A=
=0A> On Wed, Jun 14, 2023 at 07:03:24PM +0800, Yajun Deng wrote:=0A> =0A>=
> When the system boots, only one cpu is enabled before smp_init().=0A>> =
So the spinlock is not needed in most cases, remove it.=0A>> =0A>> Add sp=
inlock in get_nid_for_pfn() because it is after smp_init().=0A> =0A> So t=
his is two different things at once in the same patch?=0A> =0A> Or are th=
ey the same problem and both need to go in to solve it?=0A> =0A> And if a=
 spinlock is not needed at early boot, is it really causing any=0A> probl=
ems?=0A> =0A=0AThey are the same problem.=0AI added pr_info in early_pfn_=
to_nid(), found get_nid_for_pfn() is the only=0Acase need to add spinlock=
.=0AThis patch tested on my x86 system.=0A=0A=0A>> Signed-off-by: Yajun D=
eng <yajun.deng@linux.dev>=0A>> ---=0A>> drivers/base/node.c | 11 +++++++=
++--=0A>> mm/mm_init.c | 18 +++---------------=0A>> 2 files changed, 12 i=
nsertions(+), 17 deletions(-)=0A>> =0A>> diff --git a/drivers/base/node.c=
 b/drivers/base/node.c=0A>> index 9de524e56307..844102570ff2 100644=0A>> =
--- a/drivers/base/node.c=0A>> +++ b/drivers/base/node.c=0A>> @@ -748,8 +=
748,15 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int ni=
d)=0A>> static int __ref get_nid_for_pfn(unsigned long pfn)=0A>> {=0A>> #=
ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT=0A>> - if (system_state < SYSTEM_R=
UNNING)=0A>> - return early_pfn_to_nid(pfn);=0A>> + static DEFINE_SPINLOC=
K(early_pfn_lock);=0A>> + int nid;=0A>> +=0A>> + if (system_state < SYSTE=
M_RUNNING) {=0A>> + spin_lock(&early_pfn_lock);=0A>> + nid =3D early_pfn_=
to_nid(pfn);=0A>> + spin_unlock(&early_pfn_lock);=0A> =0A> Adding an exte=
rnal lock for when you call a function is VERY dangerous=0A> as you did n=
ot document this anywhere, and there's no way to enforce it=0A> properly =
at all.=0A> =0A=0AI should add a comment before early_pfn_to_nid().=0A=0A=
> Does your change actually result in any boot time changes? How was this=
=0A> tested?=0A> =0A=0AJust a bit.=0A=0A> thanks,=0A> =0A> greg k-h