-
-
usr/src/uts/common/sys/kfpu.h (Diff revision 1) It appears the second arg here (
fpu_flags
) is currently a placeholder. Should we add a comment indicating that for now (at least) thefpu_flags
argument should be0
?
12793 kernel FPU support
Review Request #2569 — Created June 5, 2020 and submitted
Information | |
---|---|
jjelinek | |
illumos-gate | |
12793 | |
Reviewers | |
general | |
This change provides the infrastructure to allow the kernel to use the FPU. This is work that Robert Mustacchi started at Joyent, and which I have added to. There is no consumer for this change in this code review, but the code review for 12794, which adds FPU SIMD support to zfs for the raidz parity calculations, does consume this code. That change was how this kfpu code was tested.
-
-
usr/src/uts/common/sys/kfpu.h (Diff revision 1) I see that for
kernel_fpu_begin
but I don't see anything forkernel_fpu_alloc
-- The only thing that uses it inkernel_fpu_alloc
is an explicit check that it equals 0 (otherwise it returns an error).
-
-
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1) This sort of implies that that a begin/end needs to wrap each usage of the FPU. But the code seems to indicate otherwise - I presume one the save/restore ctx are installed (begin) then the end only needs to be called when the thread is really finished with the FPU. Can you clarify in the comments?
-
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1) This relies on the caller disabling preemption and interrupts when using the FPU. What happens if another thread (which is like this one) is dispatched on the same CPU after this fpinit() and before any disabling. Is it a problem the FPU will not be in the expected state?
Change Summary:
This is the complete set of CR fixes from the initial comments (the previous diff was incomplete).
Diff: |
Revision 3 (+432 -9) |
---|
-
Overall looks good. Just had a few comments.
-
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3) We use
()
to indicate function names in some parts of this text, but don't use it for others. We should probably be consistent. -
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3) I find using the same name for the struct and member a tad confusing. Why not call this
kfpu_ctx
? -
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3) Excuse my ignorance here as this is my first time every looking at FPU code, but I'm wondering about the choice of words to describe the
FPU_EN
flag. Would theFPU_EN
flag indicate the FPU is enabled/in-use andFPU_VALID
indicate the FPU state is valid? -
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3) Not that I think it's likely for someone to share the same kfpu state across threads, but is it worth also checking if
kfpu->kfpu_curthread
is already set? And if so panic with a message indicating a thread is trying to use another thread's kfpu state. -
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3) For debugging purposes, is it worth clearing
kfpu->kfpu_curthread
so that it's clear a particular kfpu state is no longer in use by a particular thread?
Change Summary:
This change has updates from previous code review comments.
In addition, during stress testing we discovered an intermittent bug in the code.
On very rare occasions, it was possible to come out of the kernel_fpu_begin function
with the ts bit still set in %cr0 (this means FPU usage is not allowed). The result
was a panic when the kernel thread tried to actually use the FPU.We tracked this down to improper usage of installctx. The previous code was setting
up state and calling installctx with preeemption and interrupts disabled. However,
installctx can perform a sleeping allocation. If the thread happened to sleep there,
the %cr0 state that was previously set could be reset.To fix this, I rewrote the code in kernel_fpu_begin and kernel_fpu_end to avoid this
situation and reduce/eliminate the kernel preemption and interrupt disabling.
Diff: |
Revision 4 (+427 -9) |
---|
Change Summary:
After some out-of-band CR feedback and some discussion with Robert, I streamlined some of the constraints on preemption and interrupts.
Diff: |
Revision 5 (+414 -9) |
---|
Change Summary:
This update includes code review fixes from the comments in the gerrit CR, along with a new verification check in the swtch handling to catch improper kernel FPU usage.
Diff: |
Revision 6 (+454 -11) |
---|