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;
Date:   Wed, 14 Jun 2023 13:09:45 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Yajun Deng <yajun.deng@linux.dev>
Cc:     rafael@kernel.org, rppt@kernel.org, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] mm/mm_init.c: remove spinlock in early_pfn_to_nid()
Message-ID: <2023061431-litigate-upchuck-7ed1@gregkh>
References: <20230614110324.3839354-1-yajun.deng@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230614110324.3839354-1-yajun.deng@linux.dev>
Precedence: bulk

On Wed, Jun 14, 2023 at 07:03:24PM +0800, Yajun Deng wrote:
> When the system boots, only one cpu is enabled before smp_init().
> So the spinlock is not needed in most cases, remove it.
> 
> Add spinlock in get_nid_for_pfn() because it is after smp_init().

So this is two different things at once in the same patch?

Or are they the same problem and both need to go in to solve it?

And if a spinlock is not needed at early boot, is it really causing any
problems?

> 
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
>  drivers/base/node.c | 11 +++++++++--
>  mm/mm_init.c        | 18 +++---------------
>  2 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 9de524e56307..844102570ff2 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -748,8 +748,15 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
>  static int __ref get_nid_for_pfn(unsigned long pfn)
>  {
>  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> -	if (system_state < SYSTEM_RUNNING)
> -		return early_pfn_to_nid(pfn);
> +	static DEFINE_SPINLOCK(early_pfn_lock);
> +	int nid;
> +
> +	if (system_state < SYSTEM_RUNNING) {
> +		spin_lock(&early_pfn_lock);
> +		nid = early_pfn_to_nid(pfn);
> +		spin_unlock(&early_pfn_lock);

Adding an external lock for when you call a function is VERY dangerous
as you did not document this anywhere, and there's no way to enforce it
properly at all.

Does your change actually result in any boot time changes?  How was this
tested?

thanks,

greg k-h