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 191EBEB64D9 for ; Thu, 15 Jun 2023 06:21:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2485D6B0072; Thu, 15 Jun 2023 02:21:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1D1B76B0074; Thu, 15 Jun 2023 02:21:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0732B6B0078; Thu, 15 Jun 2023 02:21:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id EC9C26B0072 for ; Thu, 15 Jun 2023 02:21:00 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id B836EC0A28 for ; Thu, 15 Jun 2023 06:21:00 +0000 (UTC) X-FDA: 80903984280.24.8B46F78 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf03.hostedemail.com (Postfix) with ESMTP id E548D2000C for ; Thu, 15 Jun 2023 06:20:57 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BrTBDU5g; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf03.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686810058; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=RAPBWOCb+iWtDhtaPFIQ+GBIs2eGDpJ1XmtLe7J6me8=; b=ULYnYeNiuQYVP04jmvO6Yc0QMZZ6zwhSLurKjvgVBauL/XPyM8WygD0WXMmDTaYl2SqgDy U9cCh/RimT22IyEhTP5LYNxYFcK2IbwQ0gLtGH2MTkPbwAm6zGZZYVX2Qx/2ObphdTsath xvsYenbg3nVoZyZr6rBnYzBgygyOdMw= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BrTBDU5g; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf03.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686810058; a=rsa-sha256; cv=none; b=MAfu3iKr6CjtjuFOPYK9Thslb6cgo0uXxHivp5cN7juKkXjajstcsFcm6ci75HOqk8zZ72 UrKPA/IELZG7f7I7Qj62v+fS3xS9KBo/SbTtQB25u6sCX484ZSyNv0gdNSSIsFz3ZBfZc/ Huo+ygDGn7HJOMuHB5FATnLD0mTdrRU= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id CE8036101B; Thu, 15 Jun 2023 06:20:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D7CEEC433C8; Thu, 15 Jun 2023 06:20:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686810056; bh=/xe5fXDbmZSA8RTDGVuJ8oboo42pBGKbqRuzSjdPzrA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BrTBDU5g2Y8d6nsdUviSygWgM5bR7b0ZT48EWFiHCuLL4sQdJhw315HWh9L1etYCF yEffsKatan84SMW9biQKRqE5Z1IoFj4p4RJwxFAABS2RdP/t9lt4IcOihgBOou15Yn F7DWPSUVE1D0NrwzWXk9+mdyg7ziSTkq1TIeuPiE4c8gc2q7z4g1sKolR8JZ6C688z 29804wP2OVKhMQtFlYiBsBVKf1AR+4Mi9FJRtP2G4CNjtm+YJQIXFHEmge1AGSiMKY naOY7N4RXv0w3wQ29Aa/OWDJCOMRzarj/nBvJy5BtympJiqYZNTT37xmbuizAicET+ wPZX+TV+V5diw== Date: Thu, 15 Jun 2023 09:20:21 +0300 From: Mike Rapoport To: Yajun Deng Cc: Greg KH , rafael@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: <20230615062021.GI52412@kernel.org> References: <20230614115339.GX52412@kernel.org> <2023061431-litigate-upchuck-7ed1@gregkh> <20230614110324.3839354-1-yajun.deng@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: E548D2000C X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: u95udhu6ytjy611iupig6x5ji5kcth8z X-HE-Tag: 1686810057-913491 X-HE-Meta: U2FsdGVkX1+NzCq+T+e4IAOvUWVDf5ViFpjpu1oBJuSsESKnvg7Xs+9grCm9UvSDH47XYjMqQCmuetxAYQZxRcJX5uBzOjgbApo5EwfNbzmbmcbhL4l7LM4EdtUYz3NxBQl7lwDuQpgraKO6tBZMSwlf6TNvPGt4R8JIPV3GKKEfS7bPXThPT06o4KUctT7U/Tp48sfUS3O/ng842b49XLkQkmhlzgb2LyzrL5rNFnn79uVnmx2lnB/0k20A42gRFGNRydfYnNTvD+TQ8MnL8S9KUOJbP2aQ15tQ2fshVCtDlhi1v1GGq6m6J5N4ORKsEqkvUsM4Cw31rNXH35QIEPHr6F6UVXD2wjSIL5fZVwschV9K6ie3Xl5KB+b2V2KBcP0ykTq3bKOun87BdayXuM5XgjAC24wzX+XELzpOIAkljJjAJoQ8Bf0GhE/GkjuqupAr4ARZENO+gJaO/n6cNaj2knVltBXlo5yYJgcr/5LZTdJrtTK7ZtaOWTIWzLZQpRf9pZgq6hwT7qVPQtifaVUZmNAMuDHTAyZ0WBT1qb36+GPyI1Ili/qpiaa3d+/QQoeK6Z1OQphsfSlkzKZyUOnZLkqBjpXOAay2gqXiVTuqzIPDDwyMbSXOuDRTxJsK4gDNdOPNOUGoRP3LUb2o2WQY1P7GC+eZDBshBrK7+Z8tdkg++8RNmTvq7TH7+dCgimrMVj2QlHZvixMiPn81CrUxkSbqVSbH4mhlJOEX4FZPRIUTdQD+CtAmYFrjCyEp1B641ID6NPOB8kvZ11JW9zRW1PmTO6mXibzUdRA5GfUNdzutqVAltSaEmjHugoNtLxe/XR2ywKPnhLFNzOvWZh73sAnCzioEBwsXERtm4RSRhNR2rzNJe2jItZIrH2FZxl6Zwi6bkqrCnqID7frqdYyfJ7vi6BTJnGJeEjrkH1W3LggK4GRaiQvuYbH1sHtcfYiFYOXjRpei6BLvpoS 4/oyk+I/ B6QrPCMtV/4zesrN29cdVLfcqxWHwC2tlM4dw9rZEqGSbOadrH+5KNDmDPNzCD2pNyighUQLN1GqyxsFASWG67yQrW0uCS9R2xS5YKzIBx5UJ+fDmrtbJ0uphF1+wxDm2AHloJaArb+nslrKEFa/M4Q3A4a1zIS6DtT1cY9qp6lgND08CF9wZrmdS4JOFln/Y1CI7/Lji6NuKuPN5AWuOTRPX+RuLzKAWnh7DiEt0YksGProdPIuBhL7WWaYtg4ew4PiRWoAErMTG++mw8AexfAJf0ElDoRErepcnrhC/oJPAdJRYTBLmA5mUtp1A3EbMhj0kW4xnoAbwivaXrWMQ1LFiVOoU0JvZJ5o3BTRCI0c+OnJHBZYvvXG+6618wGmV5h1VxyIYsXZEVbIds2t5vhVLkcUSJxFZLRodZV4ir0d7a8DxJM0Uu+pLPqQqR1VCIw/1TSqWes+A1Vo= 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 Thu, Jun 15, 2023 at 03:02:58AM +0000, Yajun Deng wrote: > June 14, 2023 7:53 PM, "Mike Rapoport" wrote: > > > Hi, > > > > On Wed, Jun 14, 2023 at 11:28:32AM +0000, Yajun Deng wrote: > > > >> June 14, 2023 7:09 PM, "Greg KH" wrote: > >> > >> 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? > >> > >> They are the same problem. > >> I added pr_info in early_pfn_to_nid(), found get_nid_for_pfn() is the only > >> case need to add spinlock. > >> This patch tested on my x86 system. > > > > Are you sure it'll work on !x86? > > > > I'm probably sure of that, although I don't have a !x86 machine. > > early_pfn_to_nid() is called in smp_init() and kasan_init() on > different architectures. If it works well on x86, it'll work on > !x86. This is often not true. Please verify that other architectures do not call early_pfn_to_nid() after smp_init(). The explanation why it is safe should be a part of the changelog. > >> Signed-off-by: Yajun Deng > >> --- > >> 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. > >> > >> I should add a comment before early_pfn_to_nid(). > >> > >> Does your change actually result in any boot time changes? How was this > >> tested? > >> > >> Just a bit. > > > > Just a bit tested? Or just a bit of boot time changes? > > For the latter, do you have numbers? > > > > For the latter, the most beneficial function is memmap_init_reserved_pages(), > the boot time changes depending on whether DEFERRED_STRUCT_PAGE_INIT > is defined or not. > > -->memmap_init_reserved_pages() > -->for_each_reserved_mem_range() > reserve_bootmem_region() > -->for() > init_reserved_page() > --> early_pfn_to_nid() A better solution would be to pass nid to reserve_bootmem_range() and drop the call to early_pfn_to_nid() in init_reserved_page(). Then there won't be lock contention and no need for fragile changes in the locking. > If define CONFIG_DEFERRED_STRUCT_PAGE_INIT: > > before: > memmap_init_reserved_pages() 1.87 seconds > after: > memmap_init_reserved_pages() 1.27 seconds > > 32% time reduction. These measurements should be part of the changelog. > If not define CONFIG_DEFERRED_STRUCT_PAGE_INIT: > > early_pfn_to_nid() is called by few, > boot time didn't change. > > > By the way, this machine has 190GB RAM. > > > -- > > Sincerely yours, > > Mike. -- Sincerely yours, Mike.