On Fri, 15 Mar 2019, Chang S. Bae wrote:
> The helper functions will switch on faster accesses to FSBASE and GSBASE
> when the FSGSBASE feature is enabled.
>
> Accessing user GSBASE needs a couple of SWAPGS operations. It is avoidable
> if the user GSBASE is saved at kernel entry, being updated as changes, and
> restored back at kernel exit. However, it seems to spend more cycles for
> savings and restorations. Little or no benefit was measured from
> experiments.
This smells fishy and looking at the end result of this series just
confirms it. This ends up being a mixture of SWAPGS and FSGSBASE usage and
as already pointed out in the other reply, it causes inconsistencies.
Let's look at the big picture.
For both variants GS needs to be swapped on kernel entry and on kernel
exit.
1) SWAPGS
MSR_KERNEL_GS_BASE contains the user space GS when running in the kernel
and the kernel GS when running in user space.
SWAPGS is used to swap the content of GS and MSR_KERNEL_GS_BASE on the
transitions from and to user space.
On context switch MSR_KERNEL_GS_BASE has to be updated when switching
between processes.
User space cannot change GS other than through the PRCTL which updates
MSR_KERNEL_GS_BASE.
2) FSGSBASE
User space can set GS without kernel interaction.
So on user space to kernel space transitions swapping in kernel GS should
simply do:
userGS = RDGSBASE()
WRGSBASE(kernelGS)
and on the way out:
WRGSBASE(userGS)
instead of SWAPGS all over the place.
userGS is stored in thread_struct, except for the few paranoid
exceptions which return straight to user space, e.g. NMI. Those can just
keep it on stack or in a register.
Context switch does not have to do anything at all vs. GS because
thread_struct contains the correct value already.
The PRCTL is straight forward to support. Instead of fiddling with
MSR_KERNEL_GS_BASE it just updates thread struct.
I don't see how that's NOT going to be an advantage and I don't see
either how this seems to cause more cycles for save and restore.
Making it consistently FSGSBASE avoids especially this piece of art in the
context switch path:
local_irq_save(flags);
native_swapgs();
gsbase = rdgsbase();
native_swapgs();
local_irq_restore(flags);
along with it's write counterpart.
The whole point of FSGSBASE support is performance, right?
So can please someone explain why having the following in the context
switch path when it can be completely avoided is enhancing performance:
- 4 x SWAPGS
- 1 x RDMSR
- 1 x WRMSR
- 2 x local_irq_save()
- 2 x local_irq_restore()
Of course the local_irq_save/restore() pairs are utterly pointless because
switch_to() runs with interrupts disabled already.
SWAPGS instead needs:
1 x WRMSR
and nothing else.
So trading the single WRMSR against the above in the context switch path is
gaining performance, right?
The only thing which gains performance is user space switching GS. And this
user space performance gain is achieved by:
- Inconsistent and fragile code with a guarantee for subtle and hard to
diagnose bugs
- Pointless overhead in the context switch code
Sorry, not going to happen ever.
Get your act together and make this consistent. Either SWAPGS or FSGSBASE,
but not a mix of it.
Thanks,
tglx
