Re: [External] Re: [PATCH v8 8/9] x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup

From: Thomas Gleixner <tglx@linutronix.de>
To: Usama Arif <usama.arif@bytedance.com>,
	David Woodhouse <dwmw2@infradead.org>,
	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
Subject: Re: [External] Re: [PATCH v8 8/9] x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup
Date: Mon, 13 Feb 2023 21:42:18 +0100	[thread overview]
Message-ID: <87y1p1s3f9.ffs@tglx> (raw)
In-Reply-To: <04d98646-7c2e-9a62-1340-2d8efe69a121@bytedance.com>

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