On Sat, Jan 26, 2019 at 1:31 AM Waldek Kozaczuk <jwkozac...@gmail.com> wrote:
> Thanks for reviewing this patch. > > On Wednesday, January 23, 2019 at 5:10:56 PM UTC-5, Nadav Har'El wrote: >> >> >> So I'm really curious *why* this EMMS is needed beyond the one we added >> to the >> FPU save) because it's possible that the actual scheduler code used the >> FPU and >> then *inlined *the thread::switch_to() causing switch_to() to begin with >> an unclear >> FPU stack. Can you please help me verify if this is (or isn't) the reason >> why this >> second EMMS is needed? You can check this by adding __attribute__ >> ((noinline) >> to thread::switch_to(). If this makes this EMMS call unnecessary, we >> found the >> reason (if this EMMS is still necessary, then we still don't understand >> something...). >> > > This patch did not help fix the issue #1010 (I did not test #1018 and #536 > this time but given the stack trace looks identical I think I am guessing > "noinline" fix would not help there either. But who knows. > Now that I tested the cooperative context switch performance (with tests/misc-ctxsw) and saw that the extra EMMS doesn't hurt performance, I'm not against it (and I even suggested it in #1020), and will commit your next version of this patch even with this second EMMS. But what makes me sad is that I don't really understand *why* it is needed in addition to that other EMMS (which is called when saving FPU on preemption). So as long as you don't include an unconvincing comment trying to explain why we need it, and instead just refer to #1020 (and/or say we're not sure why it's needed in addition to the other EMMS call), I'm ok with including it. > > Finally OSv scheduler for sure does do floating point calculation, but is > it a way that corrupts FPU, I do not know. I believe > thread_runtime::time_until() > calls taulog() that uses floats to calculate some expression. See > https://github.com/cloudius-systems/osv/blob/a3cd022fcda2c88eae89476aa6c29e3c4be04926/core/sched.cc#L1759-L1773. > The time_until() is called by cpu::reschedule_from_interrupt() which > eventually calls switch_to(). > Yes, I know the scheduler does FPU calculations, but my point was that if switch_to() is a non-inline function, the compiler must obey the ABI which dictates that it is called with "good" FPU state (nothing on the stack and no MMX), so as long as switch_to() *itself* doesn't use the FPU (I looked at it's disassembly -objdump -da build/release/loader.elf - and it doesn't seem it does), at the moment it jumps to the other thread, it should have a good FPU state. That thread we return to will at that point either feel like it returned from a function (so the "good FPU state" is what it needs on return), or if was preempted, will restore some old FPU state. So I thought this would have been fine, but apparently it isn't. -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
