Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH 10/10] x86/fpu: defer FPU state load until return to userspace
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20180919170515.ptqmmpsxrdjsi64j@linutronix.de>
Date:   Thu, 20 Sep 2018 20:45:56 -0700
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Andy Lutomirski <luto@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?utf-8?Q?Radim_Kr=C4=8Dm=C3=A1=C5=99?= <rkrcmar@redhat.com>,
        kvm@vger.kernel.org, "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Rik van Riel <riel@surriel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <D5D1FB08-9CE8-498B-9636-C0390153814A@amacapital.net>
References: <20180912133353.20595-1-bigeasy@linutronix.de> <20180912133353.20595-11-bigeasy@linutronix.de> <650FC457-7E4C-473A-9E5F-EAFC74F6444B@amacapital.net> <20180919170515.ptqmmpsxrdjsi64j@linutronix.de>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Sep 19, 2018, at 10:05 AM, Sebastian Andrzej Siewior <bigeasy@linutroni=
x.de> wrote:
>=20
> On 2018-09-12 08:47:19 [-0700], Andy Lutomirski wrote:
>>> --- a/arch/x86/kernel/fpu/core.c
>>> +++ b/arch/x86/kernel/fpu/core.c
>>> @@ -101,14 +101,14 @@ void __kernel_fpu_begin(void)
>>>=20
>>>   kernel_fpu_disable();
>>>=20
>>> -    if (fpu->initialized) {
>>> +    __cpu_invalidate_fpregs_state();
>>> +
>>> +    if (!test_and_set_thread_flag(TIF_LOAD_FPU)) {
>>=20
>> Since the already-TIF_LOAD_FPU path is supposed to be fast here, use test=
_thread_flag() instead. test_and_set operations do unconditional RMW operati=
ons and are always full barriers, so they=E2=80=99re slow.
> okay.
>=20
>> Also, on top of this patch, there should be lots of cleanups available. I=
n particular, all the fpu state accessors could probably be reworked to take=
 TIF_LOAD_FPU into account, which would simplify the callers and maybe even t=
he mess of variables tracking whether the state is in regs.
>=20
> Do you refer to the fpu.initilized check or something else?
>=20

I mean the fpu.initialized variable entirely. AFAIK, its only use is for ker=
nel threads =E2=80=94 setting it to false lets us switch to a kernel thread a=
nd back without saving and restoring. But TIF_LOAD_FPU should be able to rep=
lace it: when we have FPU regs loaded and we switch to *any* thread, kernel o=
r otherwise, we can set TIF_LOAD_FPU and leave the old regs loaded.  So we d=
on=E2=80=99t need the special case for kernel threads.

Which reminds me: if you haven=E2=80=99t already done so, can you add a help=
er to sanity check the current context?  It should check that the combinatio=
n of owner_ctx, last_cpu, and TIF_LOAD_FPU is sane. For example, if owner_ct=
x or last_cpu is says the cpu regs are invalid for current but TIF_LOAD_FPU i=
s clear, it should warn.  I think that at least switch_fpu_finish should cal=
l it.  Arguably switch_fpu_prepare should too, at the beginning.=