[PATCH v1 2/8] x86/smp: Remove unnecessary WBINVD from mwait_play_dead()

[Pasha Tatashin]

On Thu, Oct 15, 2026 at 11:44 AM <sashiko-bot at kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] x86/smp: memory corruption during suspend if package offlined at runtime
> - [High] x86/smp: offline CPUs miss wbinvd_on_all_cpus() flush causing data corruption
> --
>
> commit 2579aa1d01a44c8d96bb166fd2ce0dff4493a42a
> Author: Pasha Tatashin <pasha.tatashin at soleen.com>
>
> x86/smp: Remove unnecessary WBINVD from mwait_play_dead()
>
> The commit aims to avoid time-consuming WBINVDs during runtime CPU offlining
> by only executing it when cpuhp_tasks_frozen is set (i.e. system sleep).
>
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index ba01a9e919b78..60a781dac9991 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> [ ... ]
> > @@ -1426,15 +1422,21 @@ void __noreturn mwait_play_dead(unsigned int eax_hint)
> >       md->status = CPUDEAD_MWAIT_WAIT;
> >       md->control = CPUDEAD_MWAIT_WAIT;
> >
> > -     wbinvd();
> > +     /*
> > +      * MWAIT C-states keep the caches coherent, so runtime CPU offline
> > +      * does not need to flush them. System sleep, however, may remove
> > +      * power from this package's caches while all of its CPUs are
> > +      * offline, and the flush done by the boot CPU before entering the
> > +      * sleep state only covers the boot CPU's own package.
> > +      */
> > +     if (cpuhp_tasks_frozen)
> > +             wbinvd();
>
> [Severity: Critical]
> Could this result in memory corruption during a system suspend if an entire
> package is offlined at runtime prior to the suspend?
>
> If a package is fully offlined at runtime (so cpuhp_tasks_frozen is false),
> its CPUs enter MWAIT with dirty caches. If the system later suspends,
> freeze_secondary_cpus() skips CPUs that are already offline:
>
> kernel/cpu.c:freeze_secondary_cpus() {
>         ...
>         for (cpu = nr_cpu_ids - 1; cpu >= 0; cpu--) {
>                 if (!cpu_online(cpu) || cpu == primary)
>                         continue;
>         ...
> }
>
> Because these CPUs are skipped, they never awaken to check cpuhp_tasks_frozen
> and perform the flush. Since the sleep-entry cache flush on the boot CPU
> only covers its own package, won't the dirty cache lines of the un-flushed,
> offlined package be permanently lost when power is removed?

I am not sure if this is actually feasible; nevertheless, I can add
wbinvd() when the the last CPU in the package (CPU that shares the
last level cache), enters mwait_play_dead(). That said, also my LLM
tells me that even:

> > +     if (cpuhp_tasks_frozen)
> > +             wbinvd();

that I have is not needed because:

1. Multi-socket x86 servers (where a non-boot package exists) do not
support ACPI S3 suspend-to-RAM.
2. When all cores of a package enter deep MWAIT (C6), hardware flushes
L1/L2 to the LLC on core C6 entry, and the package's power management
unit (PCU) flushes the LLC before removing LLC power in deep package
C-states (PC6+).

>
> [Severity: High]
> Does skipping the flush during runtime offline break the system-wide cache
> flush guarantees of wbinvd_on_all_cpus()?
>
> If a package is completely offlined at runtime and retains dirty cache lines
> in its LLC, wbinvd_on_all_cpus() will miss them entirely because it only
> targets online CPUs:
>
> arch/x86/lib/cache-smp.c:wbinvd_on_all_cpus() {
>         on_each_cpu(__wbinvd, NULL, 1);
> }
>
> If a system-wide non-coherent operation is later triggered (such as mapping
> memory as Uncacheable or persisting PMEM), the missing flush could lead to
> data corruption or Machine Check Exceptions from dirty line evictions.
>
> Furthermore, during Kexec, offline CPUs are kicked out of MWAIT:
>
> arch/x86/kernel/smpboot.c:smp_kick_mwait_play_dead() {
>         ...
>         for_each_cpu_andnot(cpu, cpu_present_mask, cpu_online_mask) {
>                 ...
>                 WRITE_ONCE(md->control, newstate);
>         ...
> }
>
> Will this cause the offlined CPUs to carry unflushed dirty cache lines into
> the new kernel's execution context?

Regarding carrying cachelines through kexec that Sashiko mentioned, my
LLM also says it does not make sense (and I agree with it):

1. Even before our patch, when smp_kick_mwait_play_dead() wakes an
offline CPU out of __mwait(), that CPU immediately writes to memory
(WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT)—dirtying its cache!)
and enters while (1) native_halt(); (smpboot.c:1461-1463) without
executing wbinvd().
2. Even more obviously, all CPUs that are online when kexec runs are
stopped via native_stop_other_cpus() → stop_this_cpu(), which simply
executes native_halt() in a loop without wbinvd().

Pasha

>
> >
> >       while (1) {
> >               /*
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015153331.1872641-1-pasha.tatashin@soleen.com?part=2