On 02.10.19 09:04, David Hildenbrand wrote: > On 01.10.19 23:59, Richard Henderson wrote: >> On 10/1/19 12:47 PM, David Hildenbrand wrote: >>> On 01.10.19 21:17, Richard Henderson wrote: >>>> On 10/1/19 11:16 AM, David Hildenbrand wrote: >>>>> +static inline bool should_interrupt_instruction(CPUState *cs) >>>>> +{ >>>>> + /* >>>>> + * Something asked us to stop executing chained TBs, e.g., >>>>> + * cpu_interrupt() or cpu_exit(). >>>>> + */ >>>>> + if ((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0) { >>>>> + return true; >>>>> + } >>>>> + >>>>> + /* We have a deliverable interrupt pending. */ >>>>> + if ((atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_HARD) && >>>>> + s390_cpu_has_int(S390_CPU(cs))) { >>>>> + return true; >>>>> + } >>>>> + return false; >>>>> +} >>>> >>>> The first condition should be true whenever the second condition is true. >>> >>> @@ -1018,6 +1018,7 @@ static inline bool >>> should_interrupt_instruction(CPUState *cs) >>> /* We have a deliverable interrupt pending. */ >>> if ((atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_HARD) && >>> s390_cpu_has_int(S390_CPU(cs))) { >>> + g_assert((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0); >>> return true; >>> } >>> return false; >>> >>> >>> ... >>> >>> >>> [ 60.109761] systemd[1]: Set hostname to <rhel8>. >>> ** >>> ERROR:/home/dhildenb/git/qemu/target/s390x/mem_helper.c:1021:should_interrupt_instruction: >>> assertion failed: ((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0) >>> >>> >>> A race? Roughly 20-30% pass the first but not the second check. And >>> in total, on a Fedora 30 boot, I can maybe see 30 calls of >>> should_interrupt_instruction() succeeding. >>> >>> I thought these could be pending interrupts that were not deliverable >>> when injected but are now deliverable. For these, >>> icount_decr.u32.high would already have been set to 0. >>> >>> OTOH, I guess we always exit the TB in case we change the "deliverable" >>> state >>> of an IRQ, e.g., after LPSW or LCTL. E.g., >>> >>> static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o) >>> { >>> ... >>> /* Exit to main loop to reevaluate s390_cpu_exec_interrupt. */ >>> return DISAS_PC_STALE_NOCHAIN; >>> } >>> >>> Maybe really a race then - or we are not properly exiting back to the >>> main loop in all scenarios. >> >> I think that it's a race right here in should_interrupt_instruction. >> >> Notice, interrupt_request gets set before icount_decr. Indeed, the barrier >> happens immediately before the set of icount_decr in cpu_exit(). >> >> (It is briefly confusing that we have a barrier in cpu_exit and not in >> tcg_handle_interrupt. But that's explained by the cpu_is_self -- no need >> for a >> barrier for the current cpu. I also think we could usefully use >> atomic_store_release instead of a separate smp_wmb.) >> >> Therefore checking interrupt_request after checking icount_decr violates the >> ordering rules. >> >> This is confirmed, ish, by noticing putting a breakpoint at that second >> return >> (or assert) and noticing that icount_decr.u16.hi == -1. It did get set by >> one >> of the other threads, and before gdb managed to stop the world. > > I am still puzzled why I can trigger that many races. I'll do some more > investigation, however, if there would be a deliverable interrupt which > will not set icount_decr.u16.hi == -1, then we would have to fix > something that ionstead. So the second check in this patch can indeed > go, thanks for clarifying! >
So there is one place I once introduced that is at least prone to a very rare race when CPUs are waking up: hw/intc/s390_flic.c:qemu_s390_flic_notify() We set "cs->interrupt_request |= CPU_INTERRUPT_HARD" for all CPUs - especially also STOPPED ones and trigger cpu_interrupt(cs, CPU_INTERRUPT_HARD) only if a) the CPU is running b) the CPU is sleeping and could eventually consume the interrupt There is a very small chance that a CPU would wake up and not get a cpu_interrupt(cs, CPU_INTERRUPT_HARD). In the worst case, the I/O interrupt would be delayed on that CPU, usually another CPU will pick it up - totally acceptable. If pending interrupts are deliverable is affected by the PSW mask and control registers: - lctl(g): We do a DISAS_PC_STALE_NOCHAIN - lpsw(e): We do a DISAS_NORETURN - stnosm: We do a DISAS_PC_STALE_NOCHAIN - ssm: We do a DISAS_PC_STALE_NOCHAIN Of course, the PSW will be changed when injecting exceptions, but that path should also properly check for other deliverable interrupts (I once added a deliver loop to s390_cpu_do_interrupt()). So I think the only option really is that I was seeing a bunch of the races you explained. -- Thanks, David / dhildenb