Message ID | 20230209154156.266385-9-usama.arif@bytedance.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp416774wrn; Thu, 9 Feb 2023 08:00:31 -0800 (PST) X-Google-Smtp-Source: AK7set+nQ+UnaYwZL80dbc2qQeQXCDv0d/VZSx9SNZxCG3R0nC4i9/aRsz61AsVeXru0xFh06YON X-Received: by 2002:a05:6a21:3383:b0:b8:50fd:fd1a with SMTP id yy3-20020a056a21338300b000b850fdfd1amr14056976pzb.19.1675958431190; Thu, 09 Feb 2023 08:00:31 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675958431; cv=none; d=google.com; s=arc-20160816; b=ILzW+13ezl+z5QIz+i36aDgRALT7nHObcpsJBZvYFSwDzjtGanmvFGw7KeivZEwMWs 3Bzu5522WF7Xc2yltXnOBvupc7+Zz/8833iihiHWTRuWRjSLipk2oUsT6BW5Rk+9u33j MJsgiao6cxanSO+rXSYpy8T2rixxTRSbSlwZJvWeGmHe1ag78nCHxy3DinFEba+IApLT MIyivIelHBTSQxX8MatNB9rKhC2V0BTK3kxTbVREK/b1l1TS0vyZELEmZdIi3ZrTnz0M AuBMfnMm6ZanIOqLIs1NWJvJm+F92CXRrcVJ0m3sGRZfEv1fOF6lczVQWV1JPrfqVEm7 eiZg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=J+ne+m/jA9f2Vf5iapwS72QpCbb1ZulqNbxU7adH9sw=; b=0SqqJcD9VJ2nCl/7W51YAwFHmnMYzVn6N50uA6zndZzGG5AHIWZ4ssx92awNX0FlSg u3+U3CcZAkAdWZWu99hrhJ7QNJamJBxUC4yMcGdaFvfI2HbWvq+BBOzNWl1RFkJYLGj7 rArIa21lHrEjAM+LWPCgr//xUGElI3ZH8jRZUm04z7mvibq8DJTSRyT38zR1rOJOIkig fGiZ1GLbNKLDnNPQrda/C0XrCgQzJh7r1zG0w+mgIre2H6UB4YlbtFmFVHwPhcYi6ce/ SAki9vz3n7osgCRquTWIwpbC7dQv5fjN17LiFN8aDPlkEcT8PV8ITH+Vb6InUxq7kHrN 0jhA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=5ayItAXX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=bytedance.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id z21-20020a63b915000000b004da45c40f7fsi2174841pge.807.2023.02.09.08.00.17; Thu, 09 Feb 2023 08:00:31 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=5ayItAXX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=bytedance.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231600AbjBIPnR (ORCPT <rfc822;ybw1215001957@gmail.com> + 99 others); Thu, 9 Feb 2023 10:43:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231532AbjBIPmr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 9 Feb 2023 10:42:47 -0500 Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7A5296466D for <linux-kernel@vger.kernel.org>; Thu, 9 Feb 2023 07:42:09 -0800 (PST) Received: by mail-wm1-x334.google.com with SMTP id c4-20020a1c3504000000b003d9e2f72093so4182936wma.1 for <linux-kernel@vger.kernel.org>; Thu, 09 Feb 2023 07:42:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=J+ne+m/jA9f2Vf5iapwS72QpCbb1ZulqNbxU7adH9sw=; b=5ayItAXXkr2LKM0D15OvuWaDbJN5E6hG9PHeXml42YtZ9TzV7Oy1fg6JVKJD8Tsnyo l9LhMel+mSwzxjR5+pEhTg+DpFouf58cA4eNvbULMQftW7EozDC5VCykpNI9Ocy7KXRk 7eG8LL/leNth8VaJoRqsJTWOPqaM6XCcNxeiFCQnUeIUF03iCQjbujUjBJn12VHAc4lU Gw6IwrZNO2Ui/nif1Az4vFjsyNxP2tTuj0gjA6Ce+qnzGLOd3UDIZTT5GtbmHuPD7Z+n hGt1M2hhbyF+88D//Z3+cn9/RwJ1t5CW2S5BuMbYZgwb9NjL15ueESBzmfp7CgCCixFX 55Qg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=J+ne+m/jA9f2Vf5iapwS72QpCbb1ZulqNbxU7adH9sw=; b=7wROVEyX6NfBhVVMDBF7TPS+9nX0tt3ngelASGSUTBtIhP9nir7B+5uxMT/mEemz88 QLItHkeZaLS8RlP3nwkrmXjsfsZe3ZRUL9zwyWvm80gh4HXMkgxvZGVnNTADPcGyjY21 wujafzoB9hBLTDWcKL+7dI9KsTfhG8IsMZjns6E++xd5cbmLvX7QRYdPOqNiJV/juP1p f9Va9i5JvW7AXwThOFu4pqofe2xfjMZsE7bEjassYcNmyeqUDw2jc63Xd/ud+/FeCqzB s19rnWhNOIQoqffGzOU/KyD0X7VVQ3OHGaRDoRp+AWEknTUQcdgT4L4iuBf432Z7VVkH t29A== X-Gm-Message-State: AO0yUKVm3d+a/Myfwk3aZAF2rvqvSEiSyi9FtlmDTwvxOoWB6W0BXwN6 OEmuf9UAPennuY7BpgDlF5AlxQ== X-Received: by 2002:a05:600c:755:b0:3dc:4b87:a570 with SMTP id j21-20020a05600c075500b003dc4b87a570mr11538710wmn.35.1675957328115; Thu, 09 Feb 2023 07:42:08 -0800 (PST) Received: from usaari01.cust.communityfibre.co.uk ([2a02:6b6a:b566:0:8009:2525:9580:8db2]) by smtp.gmail.com with ESMTPSA id y6-20020a05600c364600b003df7b40f99fsm5099754wmq.11.2023.02.09.07.42.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Feb 2023 07:42:07 -0800 (PST) From: Usama Arif <usama.arif@bytedance.com> To: dwmw2@infradead.org, tglx@linutronix.de, kim.phillips@amd.com Cc: arjan@linux.intel.com, mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, hpa@zytor.com, x86@kernel.org, pbonzini@redhat.com, paulmck@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, rcu@vger.kernel.org, mimoja@mimoja.de, hewenliang4@huawei.com, thomas.lendacky@amd.com, seanjc@google.com, pmenzel@molgen.mpg.de, fam.zheng@bytedance.com, punit.agrawal@bytedance.com, simon.evans@bytedance.com, liangma@liangbit.com, David Woodhouse <dwmw@amazon.co.uk>, Usama Arif <usama.arif@bytedance.com> Subject: [PATCH v8 8/9] x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup Date: Thu, 9 Feb 2023 15:41:55 +0000 Message-Id: <20230209154156.266385-9-usama.arif@bytedance.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230209154156.266385-1-usama.arif@bytedance.com> References: <20230209154156.266385-1-usama.arif@bytedance.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1757369787550971713?= X-GMAIL-MSGID: =?utf-8?q?1757369787550971713?= |
Series |
Parallel CPU bringup for x86_64
|
|
Commit Message
Usama Arif
Feb. 9, 2023, 3:41 p.m. UTC
From: David Woodhouse <dwmw@amazon.co.uk> There's no need to repeatedly save the BSP's MTRRs for each AP being brought up at boot time. And no need to use smp_call_function_single() even for the one time they do need to be saved. Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> Signed-off-by: Usama Arif <usama.arif@bytedance.com> --- arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Comments
On Thu, Feb 09 2023 at 15:41, Usama Arif wrote: > void mtrr_save_state(void) > { > + static bool mtrr_saved; > int first_cpu; > > if (!mtrr_enabled()) > return; > > + if (system_state < SYSTEM_RUNNING) { > + if (!mtrr_saved) { > + mtrr_save_fixed_ranges(NULL); > + mtrr_saved = true; > + } > + return; > + } > + > first_cpu = cpumask_first(cpu_online_mask); > smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); So why is this relevant after the initial bringup? The BP MTRRs have been saved already above, no? Thanks, tglx
On 09/02/2023 18:31, Thomas Gleixner wrote: > On Thu, Feb 09 2023 at 15:41, Usama Arif wrote: >> void mtrr_save_state(void) >> { >> + static bool mtrr_saved; >> int first_cpu; >> >> if (!mtrr_enabled()) >> return; >> >> + if (system_state < SYSTEM_RUNNING) { >> + if (!mtrr_saved) { >> + mtrr_save_fixed_ranges(NULL); >> + mtrr_saved = true; >> + } >> + return; >> + } >> + >> first_cpu = cpumask_first(cpu_online_mask); >> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); > > So why is this relevant after the initial bringup? The BP MTRRs have > been saved already above, no? > > Thanks, > > tglx I will let David confirm if this is correct and why he did it, but this is what I thought while reviewing before posting v4: - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state is called in do_cpu_up at roughly the same time so MTRR is going to be the same, we can just save it once and then reuse for other secondary cores as it wouldn't have changed for the rest of the do_cpu_up calls. - When the system is running and you offline and then online a CPU, you want to make sure that hotplugged CPU gets the current MTRR (which might have changed since boot?), incase the MTRR has changed after the system has been booted, you save the MTRR of the first online CPU. When the hotplugged CPU runs its initialisation code, its fixed-range MTRRs will be updated with the newly saved fixed-range MTRRs. So mainly for hotplug, but will let David confirm. Thanks, Usama
On Thu, 2023-02-09 at 20:32 +0000, Usama Arif wrote: > > I will let David confirm if this is correct and why he did it, but this > is what I thought while reviewing before posting v4: > > - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state > is called in do_cpu_up at roughly the same time so MTRR is going to be > the same, we can just save it once and then reuse for other secondary > cores as it wouldn't have changed for the rest of the do_cpu_up calls. > > - When the system is running and you offline and then online a CPU, you > want to make sure that hotplugged CPU gets the current MTRR (which might > have changed since boot?), incase the MTRR has changed after the system > has been booted, you save the MTRR of the first online CPU. When the > hotplugged CPU runs its initialisation code, its fixed-range MTRRs will > be updated with the newly saved fixed-range MTRRs. > > So mainly for hotplug, but will let David confirm. Sounds about right.
On Thu, Feb 09 2023 at 20:32, Usama Arif wrote: > On 09/02/2023 18:31, Thomas Gleixner wrote: >>> first_cpu = cpumask_first(cpu_online_mask); >>> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); >> >> So why is this relevant after the initial bringup? The BP MTRRs have >> been saved already above, no? >> > > I will let David confirm if this is correct and why he did it, but this > is what I thought while reviewing before posting v4: > > - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state > is called in do_cpu_up at roughly the same time so MTRR is going to be > the same, we can just save it once and then reuse for other secondary > cores as it wouldn't have changed for the rest of the do_cpu_up calls. > > - When the system is running and you offline and then online a CPU, you > want to make sure that hotplugged CPU gets the current MTRR (which might > have changed since boot?), incase the MTRR has changed after the system > has been booted, you save the MTRR of the first online CPU. When the > hotplugged CPU runs its initialisation code, its fixed-range MTRRs will > be updated with the newly saved fixed-range MTRRs. I knew that already :) But seriously: If the MTRRs are changed post boot then the cached values want to be updated too. We are not making these changes just to satisfy some fast boot challenge. They have to make sense in general. And this does not amke sense at all. Thanks, tglx
On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote: > On Thu, Feb 09 2023 at 20:32, Usama Arif wrote: > > On 09/02/2023 18:31, Thomas Gleixner wrote: > > > > first_cpu = cpumask_first(cpu_online_mask); > > > > smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); > > > > > > So why is this relevant after the initial bringup? The BP MTRRs have > > > been saved already above, no? > > > > > > > I will let David confirm if this is correct and why he did it, but this > > is what I thought while reviewing before posting v4: > > > > - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state > > is called in do_cpu_up at roughly the same time so MTRR is going to be > > the same, we can just save it once and then reuse for other secondary > > cores as it wouldn't have changed for the rest of the do_cpu_up calls. > > > > - When the system is running and you offline and then online a CPU, you > > want to make sure that hotplugged CPU gets the current MTRR (which might > > have changed since boot?), incase the MTRR has changed after the system > > has been booted, you save the MTRR of the first online CPU. When the > > hotplugged CPU runs its initialisation code, its fixed-range MTRRs will > > be updated with the newly saved fixed-range MTRRs. > > I knew that already :) But seriously: > > If the MTRRs are changed post boot then the cached values want to be > updated too. They are, aren't they? The only way we come out of mtrr_save_state() without calling mtrr_save_fixed_ranges() — either directly or via smp_call_function_single() — is if they've already been saved once *and* system_state < SYSTEM_RUNNING. I suppose we could make that clearer by moving the definition of the mtrr_saved flags inside the if (system_state < SYSTEM_RUNNING) block? @@ -721,11 +721,20 @@ void __init mtrr_bp_init(void) */ void mtrr_save_state(void) { int first_cpu; if (!mtrr_enabled()) return; + if (system_state < SYSTEM_RUNNING) { + static bool mtrr_saved; + if (!mtrr_saved) { + mtrr_save_fixed_ranges(NULL); + mtrr_saved = true; + } + return; + } + first_cpu = cpumask_first(cpu_online_mask); smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); }
On 10/02/2023 08:55, David Woodhouse wrote: > On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote: >> On Thu, Feb 09 2023 at 20:32, Usama Arif wrote: >>> On 09/02/2023 18:31, Thomas Gleixner wrote: >>>>> first_cpu = cpumask_first(cpu_online_mask); >>>>> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); >>>> >>>> So why is this relevant after the initial bringup? The BP MTRRs have >>>> been saved already above, no? >>>> >>> >>> I will let David confirm if this is correct and why he did it, but this >>> is what I thought while reviewing before posting v4: >>> >>> - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state >>> is called in do_cpu_up at roughly the same time so MTRR is going to be >>> the same, we can just save it once and then reuse for other secondary >>> cores as it wouldn't have changed for the rest of the do_cpu_up calls. >>> >>> - When the system is running and you offline and then online a CPU, you >>> want to make sure that hotplugged CPU gets the current MTRR (which might >>> have changed since boot?), incase the MTRR has changed after the system >>> has been booted, you save the MTRR of the first online CPU. When the >>> hotplugged CPU runs its initialisation code, its fixed-range MTRRs will >>> be updated with the newly saved fixed-range MTRRs. >> >> I knew that already :) But seriously: >> >> If the MTRRs are changed post boot then the cached values want to be >> updated too. > I had previously only done smpboot time measurements for the whole patchset, but I tested the patchset without this commit and it doesn't make a difference to smpboot time as its negligable work to read those MTRR MSRs into mtrr_state.fixed_ranges. This commit is also independent of parallel smp bringup, similar to reusing timer calibration so I think it could be considered as a separate patchset if needed. I will post the next revision without this commit, but here is my view on MTRR save/restore (which shouldn't matter for the next revision...). If the MTRR changes on a running system, there might be a bug during hotplug in the original code that handles MTRR? which is also carried over in this patch. From what I can see, MTRR is only saved+restored during initial boot, hotplugging CPU and __save/__restore_processor_state() (used in creating image for hibernation, suspend, kexec...). So if for e.g. in a running system (that has not hibernated, suspended, kexeced), if MTRR for CPU0 (first_cpu) changed post-boot and CPU3 is hotplugged, only MTRR for CPU3 is updated and CPU0 and CPU3 will hold the same value, while the rest of the CPUs will have the older first-boot value? This behavior will happen with or without this patch. I think this is what Thomas is referring to above when he says that the cached values want to be updated? But the issue is present in the original code as well. Thanks! Usama > They are, aren't they? The only way we come out of mtrr_save_state() > without calling mtrr_save_fixed_ranges() — either directly or via > smp_call_function_single() — is if they've already been saved once > *and* system_state < SYSTEM_RUNNING. > > I suppose we could make that clearer by moving the definition of the > mtrr_saved flags inside the if (system_state < SYSTEM_RUNNING) block? > > @@ -721,11 +721,20 @@ void __init mtrr_bp_init(void) > */ > void mtrr_save_state(void) > { > int first_cpu; > > if (!mtrr_enabled()) > return; > > + if (system_state < SYSTEM_RUNNING) { > + static bool mtrr_saved; > + if (!mtrr_saved) { > + mtrr_save_fixed_ranges(NULL); > + mtrr_saved = true; > + } > + return; > + } > + > first_cpu = cpumask_first(cpu_online_mask); > smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); > } >
On Mon, Feb 13 2023 at 15:19, Usama Arif wrote: > On 10/02/2023 08:55, David Woodhouse wrote: >> On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote: >>> I knew that already :) But seriously: >>> >>> If the MTRRs are changed post boot then the cached values want to be >>> updated too. >> > I had previously only done smpboot time measurements for the whole > patchset, but I tested the patchset without this commit and it doesn't > make a difference to smpboot time as its negligable work to read those > MTRR MSRs into mtrr_state.fixed_ranges. > This commit is also independent of parallel smp bringup, similar to > reusing timer calibration so I think it could be considered as a > separate patchset if needed. I will post the next revision without this > commit, but here is my view on MTRR save/restore (which shouldn't matter > for the next revision...). Correct. It's an orthogonal problem and not a prerequisite for the initial parallel bringup. > If the MTRR changes on a running system, there might be a bug during > hotplug in the original code that handles MTRR? which is also carried > over in this patch. > From what I can see, MTRR is only saved+restored during initial boot, > hotplugging CPU and __save/__restore_processor_state() (used in creating > image for hibernation, suspend, kexec...). So if for e.g. in a running > system (that has not hibernated, suspended, kexeced), if MTRR for CPU0 > (first_cpu) changed post-boot and CPU3 is hotplugged, only MTRR for CPU3 > is updated and CPU0 and CPU3 will hold the same value, while the rest of > the CPUs will have the older first-boot value? This behavior will happen > with or without this patch. I think this is what Thomas is referring to > above when he says that the cached values want to be updated? But the > issue is present in the original code as well. No. Changing a MTTR is done via stop_machine() on all online CPUs in parallel. So there is no bug. But its stupid that an update to a MTRR is not updating the cached value, which results in this sillyness of reading back the real values into the cache on CPU hotplug in order to set them up correctly on the upcoming CPU. So the real improvement would be to: 1) Cache the MTRRs on the boot CPU once during early boot 2) When MTRRs are changed then update the cache too That ensures that the MTRR cache is always up to date and can be utilized on CPU hotplug. The 'if (state < RUNNING)' thing is just a sloppy hack which tries to save a few cycles at boot. Thanks, tglx
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c index 783f3210d582..b6eae3ad4414 100644 --- a/arch/x86/kernel/cpu/mtrr/mtrr.c +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c @@ -721,11 +721,20 @@ void __init mtrr_bp_init(void) */ void mtrr_save_state(void) { + static bool mtrr_saved; int first_cpu; if (!mtrr_enabled()) return; + if (system_state < SYSTEM_RUNNING) { + if (!mtrr_saved) { + mtrr_save_fixed_ranges(NULL); + mtrr_saved = true; + } + return; + } + first_cpu = cpumask_first(cpu_online_mask); smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1); }