slight problem with r254457 (kthread_add fix)

[Konstantin Belousov]

On Thu, Mar 27, 2014 at 01:42:02PM -0600, Chris Torek wrote:
> >I agree with the problem statement, but disagree with the proposed
> >'fix'. I.e., I agree that on the creation of a new kernel thread, the
> >current thread FPU grab state must not leak into the created thread.
> >
> >In fact, cpu_set_upcall() already almost handles this correctly, by
> >resetting the pcb_save pointer back to the user save area.  What is
> >not done is the clearing of PCB_KERNFPU flag, which seems to be the
> >issue you met.  Am I right ?
> 
> Yes.  I considered your fix as well but decided it required also
> understanding other architectures too much. :-)  (I don't know
> if sparc has similar code, for instance, but it *could*; its
> FPU is much like the amd64 one.  You've fixed i386, but there
> are more.)
The kernel-FPU code only exists for x86.

> 
> >If yes, then the following patch should handle the problem, hopefully.
> 
> I'm OK with this too, as long as all architectures do it, or
> something sufficiently similar.
Did you tested the patch I posted, for your situation ?

> 
> (Another way to look at this is that creating a new kernel thread
> requires cloning a child from somewhere, but we don't have a
> particularly good "somewhere" so we clone something chosen at what
> amounts to random.  We then have to make this look like a "virgin
> birth", untainted by any previous actions of the thing we're
> cloning.
> 
> While I was hunting this problem I also looked at cpu_throw in
> amd64/amd64/cpu_switch.S.  It took me a long time to figure out
> why it is safe to ignore the per-cpu fpcurthread there.  It is,
> because cpu_throw is used for only one case, the "virgin birth"
> setup on each CPU.  There can be no existing thread using the FPU
> at that point, so this is OK.)

It seems that you formulation somewhat contradictory.  The cpu_throw
is used only to do the last switch out of the exiting thread.

And indeed, I think that there is a bug, on x86.  The CR0.TS must be
cleared, and fpcurthread must be reset if current cpu still carries
FPU state of the curthread.  At least, I do not see anything which
would guarantee that CLTS was done before cpu_throw.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140328/380bd1fc/attachment.sig>