- Download Diff

Summary:

12793 kernel FPU support

Review Request #2569 — Created June 5, 2020 and submitted July 10, 2020, 2:20 p.m.

Information
Owner:	jjelinek
Repository:	illumos-gate
Branch:
Bugs:	12793
Depends On:
Reviewers
Groups:	general
People:

Description

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.

Testing Done

Issues

0
0
12
2
All Issues: 14

Description	From	Last Updated
It appears the second arg here (fpu_flags) is currently a placeholder. Should we add a comment indicating that for now ...	jbk	June 5, 2020, 7:12 p.m.
I see that for kernel_fpu_begin but I don't see anything for kernel_fpu_alloc -- The only thing that uses it in ...	jbk	June 5, 2020, 7:31 p.m.
This sort of implies that that a begin/end needs to wrap each usage of the FPU. But the code seems ...	pwinder	June 8, 2020, 6:54 p.m.
SP strucuture	seeemef@mac.com	June 8, 2020, 6:03 p.m.
This relies on the caller disabling preemption and interrupts when using the FPU. What happens if another thread (which is ...	pwinder	June 8, 2020, 6:58 p.m.
typo "the our"	seeemef@mac.com	June 8, 2020, 6:04 p.m.
We use () to indicate function names in some parts of this text, but don't use it for others. We ...	rzezeski	June 18, 2020, 12:34 p.m.
I find using the same name for the struct and member a tad confusing. Why not call this kfpu_ctx?	rzezeski	June 18, 2020, 12:36 p.m.
Excuse my ignorance here as this is my first time every looking at FPU code, but I'm wondering about the ...	rzezeski	June 18, 2020, 12:52 p.m.
Not that I think it's likely for someone to share the same kfpu state across threads, but is it worth ...	rzezeski	June 18, 2020, 12:39 p.m.
For debugging purposes, is it worth clearing kfpu->kfpu_curthread so that it's clear a particular kfpu state is no longer in ...	rzezeski	June 18, 2020, 12:46 p.m.
SP should be "suppressed"?	seeemef@mac.com	July 1, 2020, 11:54 a.m.
SP surpressed	seeemef@mac.com	July 1, 2020, 11:54 a.m.
SP should be "perform"?	seeemef@mac.com	July 1, 2020, 11:54 a.m.

usr/src/uts/common/sys/kfpu.h (Diff revision 1)

Show all issues

It appears the second arg here (fpu_flags) is currently a placeholder. Should we add a comment indicating that for now (at least) the fpu_flags argument should be 0?

jjelinek June 5, 2020, 7:12 p.m.

It is not a placeholder since there is nothing consuming this code in this CR.
If you look at the CR for the zfs raidz changes (https://illumos.org/rb/r/2570/),
you'll see an example of how the flag parameter is used.

usr/src/uts/common/sys/kfpu.h (Diff revision 1)

Show all issues

I see that for kernel_fpu_begin but I don't see anything for kernel_fpu_alloc -- The only thing that uses it in kernel_fpu_alloc is an explicit check that it equals 0 (otherwise it returns an error).

jjelinek June 5, 2020, 7:31 p.m.

Yes, that's right. The new kfpu_state_t code was not a good fit for the zfs use case I had.
I'll be sure to point this out in the testing notes (i.e. those code paths are untested).

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1)
Show all issues
```
SP strucuture
```
usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1)
Show all issues
```
typo "the our"
```

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1)

Show all issues

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)

Show all issues

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?

jjelinek June 8, 2020, 6:58 p.m.

Preemption should be disabled before calling kernel_fpu_begin. I've updated the comments to clarify this. You can see an example of this usaga in the first consumer for this change in the new zfs raidz parity code (https://www.illumos.org/rb/r/2570/).

Change Summary:

Changes for CR comments.

Diff:

Revision 2 (+427 -9)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

Disregard the current diff, I missed including most of the CR feedback.

Change Summary:

This is the complete set of CR fixes from the initial comments (the previous diff was incomplete).

Diff:

Revision 3 (+432 -9)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

Ship it!

LGTM (though my contribution here was limited to just looking for typos)

Overall looks good. Just had a few comments.

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3)

Show all issues

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)

Show all issues

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)

Show all issues

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 the FPU_EN flag indicate the FPU is enabled/in-use and FPU_VALID indicate the FPU state is valid?

jjelinek June 18, 2020, 12:51 p.m.

The FPU_EN and FPU_VALID flags predate this change, so I don't want to change their names.
FPU_EN means that FPU usage is currently allowed (enabled) for this thread. FPU_VALID
means that the state currently saved in the pcb_fpu (or the new kfpu_state struct)
is valid. In other words, this means that a context save (e.g. fp_save) has stored
the FPU state into the pcb_fpu struct.

rzezeski June 18, 2020, 3:47 p.m.

Oh, I wasn't saying to change any code. What I meant was that it seems to me the comment doesn't make sense.

The FPU_EN flag is required to indicate that things are valid.

It sounds like you are describing the FPU_VALID flag.

jjelinek June 18, 2020, 4:04 p.m.
```
I updated the comment to fix this.
```

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3)

Show all issues

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.

jjelinek June 18, 2020, 12:39 p.m.

That check exists at line 1342 in this draft.

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 3)

Show all issues

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)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

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)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

usr/src/uts/intel/ia32/os/fpu.c (Diff revision 5)
Show all issues
```
SP surpressed
```

As noted by C. Fraire, there is a typo. Other than that looks good.

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)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/ml/swtch.s
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

usr/src/uts/intel/ia32/os/fpu.c (Diff revisions 5 - 6)
Show all issues
```
SP should be "suppressed"?
```
usr/src/uts/intel/ia32/os/fpu.c (Diff revisions 5 - 6)
Show all issues
```
SP should be "perform"?
```

Change Summary:

Fix comment typos.

Diff:

Revision 7 (+454 -11)

Show changes

	usr/src/cmd/mdb/common/modules/genunix/findstack_subr.c
	usr/src/uts/common/sys/kfpu.h
	usr/src/uts/common/sys/thread.h
	usr/src/uts/intel/ia32/ml/swtch.s
	usr/src/uts/intel/ia32/os/fpu.c
	usr/src/uts/intel/sys/pcb.h

Ship it!

```
LGTM
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

12793 kernel FPU support

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)