Re: [PATCH 23/27] x86/fpu: Defer FPU state load until return to userspace

Borislav Petkov
2019-04-12T17:29:42Z

On Fri, Apr 12, 2019 at 07:19:55PM +0200, Sebastian Andrzej Siewior wrote:
> remove x86_fpu_activate_state.

Ok, zapping it as part of this patch.

> We could also rip all trcepoints out, rethink the situation and add new
> ones based on current code.

Well, since this changes FPU regs handling considerably, I think the
only correct step would be to adjust the tracepoints. BUT(!) we should
not forget we're not supposed to break luserspace.

> - on schedule out, we may need to save registers (depending on
> TIF_NEED_FPU_LOAD) which is new. Before the series we always did.

That makes sense.

> - on schedule in do nothing but set that TIF bit. This is probably
> boring.

Yah.

> - on return to userland we should load the registers but can avoid it if
> we assume that they are valid for the current task
> (fpregs_state_valid())

That is interesting info.

> - in kernel_fpu_begin() we may trash the task's FPU state (by saving its
> registers or by resetting fpu_fpregs_owner_ctx).

Do we care?

You mean, in case you have workloads which might involve a lot of
in-kernel FPU use which would punish task context switches?

> Those might be interesting.
>
> Currently we have:
> "x86/fpu: %p load: %d xfeatures: %llx xcomp_bv: %llx"
>
> and we have to find out what happens based on where that TP was
> recorded. Also I'm not sure if the recorded xfeatures change over time.
> I think they do not…

Good question.

> you mean trace_x86_fpu_activate_state and trace_x86_fpu_regs_activated?
> They were added in d1898b733619b ("x86/fpu: Add tracepoints to dump FPU
> state at key points") and we wouldn't have any otherwise.

I guess it made sense then...

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.