cpu-exec: fix missed CPU kick during interrupt injection
PATCH 1/1 cpu-exec: fix missed CPU kick during interrupt injection
The conditional memory barrier not only looks strange but actually is wrong.
On s390x, I can reproduce interrupts via cpu_interrupt() not leading to a proper kick out of emulation every now and then. cpu_interrupt() is especially used for inter CPU communication via SIGP (esp. external calls and emergency interrupts).
With this patch, I was not able to reproduce. (esp. no stalls or hangs in the guest).
My setup is s390x MTTCG with 16 VCPUs on 8 CPU host, running make -j16.
--- accel/tcg/cpu-exec.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
accel/tcg/cpu-exec.c +4 -8
@@ -525,19 +525,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, TranslationBlock **last_tb) {CPUClass *cc =CPU_GET_CLASS (cpu); -int32_t insns_left;/* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. */ - insns_left =atomic_read (&cpu->icount_decr.u32);atomic_set (&cpu->icount_decr.u16.high,0 ); -if (unlikely (insns_left <0 )) { -/* Ensure the zeroing of icount_decr comes before the next read -* of cpu->exit_request or cpu->interrupt_request. -*/ -smp_mb (); - } +/* Ensure zeroing happens before reading cpu->exit_request or +* cpu->interrupt_request. (also see cpu_exit()) +*/ +smp_mb ();if (unlikely (atomic_read (&cpu->interrupt_request))) {int interrupt_request;
Signature · 1 line
2.14.3
Quoted 37 lines · David Hildenbrand
The conditional memory barrier not only looks strange but actually is wrong.
On s390x, I can reproduce interrupts via
cpu_interrupt()not leading to a proper kick out of emulation every now and then.cpu_interrupt()is especially used for inter CPU communication via SIGP (esp. external calls and emergency interrupts).With this patch, I was not able to reproduce. (esp. no stalls or hangs in the guest).
My setup is s390x MTTCG with 16 VCPUs on 8 CPU host, running make -j16.
Signed-off-by David Hildenbrand <[email protected]>--- accel/tcg/cpu-exec.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)accel/tcg/cpu-exec.c +0
-5@@ -525,19 +525,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, TranslationBlock **last_tb) {CPUClass *cc =CPU_GET_CLASS (cpu); -int32_t insns_left;/* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. */ - insns_left =atomic_read (&cpu->icount_decr.u32);atomic_set (&cpu->icount_decr.u16.high,0 ); -if (unlikely (insns_left <0 )) { -/* Ensure the zeroing of icount_decr comes before the next read -* of cpu->exit_request or cpu->interrupt_request.
accel/tcg/cpu-exec.c static inline bool cpu_handle_interrupt(CPUState *cpu, · line 539
Quoted 10 lines
accel/tcg/cpu-exec.c +4
-3-*/ -smp_mb (); - } +/* Ensure zeroing happens before reading cpu->exit_request or +* cpu->interrupt_request. (also see cpu_exit()) +*/ +smp_mb ();if (unlikely (atomic_read (&cpu->interrupt_request))) {int interrupt_request;
atomic_mb_set can be a little faster on x86, so:
accel/tcg/cpu-exec.c +3 -5
@@ -528,12 +528,10 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,/* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. +* Ensure zeroing happens before reading cpu->exit_request or +* cpu->interrupt_request (see also smp_wmb in cpu_exit()) */ -atomic_set (&cpu->icount_decr.u16.high,0 ); -/* Ensure zeroing happens before reading cpu->exit_request or -* cpu->interrupt_request. (also see cpu_exit()) -*/ -smp_mb (); +atomic_mb_set (&cpu->icount_decr.u16.high,0 );if (unlikely (atomic_read (&cpu->interrupt_request))) {int interrupt_request;
Quoted 14 lines
atomic_mb_setcan be a little faster on x86, so:accel/tcg/cpu-exec.c +2
-0@@ -528,12 +528,10 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,/* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. +* Ensure zeroing happens before reading cpu->exit_request or +* cpu->interrupt_request (see also smp_wmb in cpu_exit())
accel/tcg/cpu-exec.c static inline bool cpu_handle_interrupt(CPUState *cpu, · line 537
Quoted 10 lines
accel/tcg/cpu-exec.c +1
-5*/ -atomic_set (&cpu->icount_decr.u16.high,0 ); -/* Ensure zeroing happens before reading cpu->exit_request or -* cpu->interrupt_request. (also see cpu_exit()) -*/ -smp_mb (); +atomic_mb_set (&cpu->icount_decr.u16.high,0 );if (unlikely (atomic_read (&cpu->interrupt_request))) {int interrupt_request;
Looks good to me! Thanks!
Signature · 3 lines
Thanks, David / dhildenb