12793 kernel FPU support

Review Request #2569 — Created June 5, 2020 and submitted

jjelinek
illumos-gate
12793
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.



  • 0
  • 0
  • 12
  • 2
  • 14
Description From Last Updated
jbk
  1. 
      
  2. 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) the fpu_flags argument should be 0?

    1. 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.

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

    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).

    1. 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).

  3. 
      
seeemef@mac.com
  1. 
      
  2. usr/src/uts/intel/ia32/os/fpu.c (Diff revision 1)
     
     

    SP strucuture

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

    typo "the our"

  4. 
      
pwinder
  1. 
      
  2. 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?

  3. 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?

    1. 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/).

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

  2. 
      
jjelinek
seeemef@mac.com
  1. LGTM (though my contribution here was limited to just looking for typos)

  2. 
      
rzezeski
  1. Overall looks good. Just had a few comments.

  2. 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.

  3. 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?

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

    1. 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.

    2. 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.

    3. I updated the comment to fix this.

  5. 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.

    1. That check exists at line 1342 in this draft.

  6. 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?

  7. 
      
jjelinek
jjelinek
seeemef@mac.com
  1. 
      
  2. usr/src/uts/intel/ia32/os/fpu.c (Diff revision 5)
     
     

    SP surpressed

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

  2. 
      
jjelinek
seeemef@mac.com
  1. 
      
  2. usr/src/uts/intel/ia32/os/fpu.c (Diff revisions 5 - 6)
     
     

    SP should be "suppressed"?

  3. usr/src/uts/intel/ia32/os/fpu.c (Diff revisions 5 - 6)
     
     

    SP should be "perform"?

  4. 
      
jjelinek
seeemef@mac.com
  1. LGTM

  2. 
      
rzezeski
  1. Ship It!
  2. 
      
jbk
  1. Ship It!
  2. 
      
jjelinek
Review request changed

Status: Closed (submitted)

Loading...