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 <david@redhat.com>
---
 accel/tcg/cpu-exec.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 9b544d88c8..dfba5ebd29 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -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;
-- 
2.14.3

On 29/11/2017 20:13, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  accel/tcg/cpu-exec.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 9b544d88c8..dfba5ebd29 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -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;
> 

atomic_mb_set can be a little faster on x86, so:

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index dfba5ebd29..4452cd9856 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -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;

> atomic_mb_set can be a little faster on x86, so:
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index dfba5ebd29..4452cd9856 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -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;
> 

Looks good to me! Thanks!


-- 

Thanks,

David / dhildenb