Re: [PATCH] x86, fpu: avoid FPU lazy restore after suspend

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Nov 30, 2012 at 10:52 AM, Vincent Palatin <vpalatin@chromium.org> wrote:
> When a cpu enters S3 state, the FPU state is lost.
> After resuming for S3, if we try to lazy restore the FPU for a process running
> on the same CPU, this will result in a corrupted FPU context.

Good catch, and I think the patch is technically correct, but:

> We can just invalidate the "fpu_owner_task", so nobody will try to
> lazy restore a state which no longer exists in the hardware.

I think this works well, but is a tiny bit subtle.

And what I _don't_ necessarily think is the right thing to do is to
only comment on it in the commit message, because it means that later
generations will not necessarily ever notice. And it does result in a
new combination that I don't think we've had before: if the current
task is the owner, we now have "tsk->thread.fpu.has_fpu  = 1" but with
"fpu_owner_task" being NULL.

Which gets us the semantics we want (we will save the current CPU info
when switching away, but we will not restore when switching back), but
my gut feel is that we really want to comment on that exact thing. And
possibly even make a helper function for this in <sm/fpu-internal.h>,
something like

    /*
     * Must be run with preemption disabled: this clears the fpu_owner_task,
     * on this CPU.
     *
     * This will disable any lazy FPU state restore of the current FPU state,
     * but if the current thread owns the FPU, it will still be saved by.
     */
    static inline void __cpu_disable_lazy_restore(void)
    {
        this_cpu_write(fpu_owner_task, NULL);
    }

and in fact I think the right place to do this *might* be in
"native_cpu_die()" instead, at which point it would actually be
something like

    per_cpu(fpu_owner_task, cpu) = NULL;

*after* the CPU is dead, so that nothing ever can actually see the
state where a process is still running on the CPU and might possibly
use the FPU.

I dunno. I think doing it after really killing the CPU (ie in the
native_cpu_die() function) might be easier to think about, but I don't
really hate your patch either (it does make me go "ok, we need to
guarantee no scheduling or FP use after" - which is probably true, but
it's still some non-local thing). Either way, a comment about it and
abstracting whatever the invalidation sequence is in fpu-internal.h
sounds like a good idea.

Hmm?

                  Linus