Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 cpus.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index ebc13bac2d..38dabb138d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -250,16 +250,12 @@ void cpu_update_icount(CPUState *cpu)
     int64_t executed = cpu_get_icount_executed(cpu);
     cpu->icount_budget -= executed;
 
-#ifndef CONFIG_ATOMIC64
     seqlock_write_lock(&timers_state.vm_clock_seqlock,
                        &timers_state.vm_clock_lock);
-#endif
     atomic_set__nocheck(&timers_state.qemu_icount,
                         timers_state.qemu_icount + executed);
-#ifndef CONFIG_ATOMIC64
     seqlock_write_unlock(&timers_state.vm_clock_seqlock,
                          &timers_state.vm_clock_lock);
-#endif
 }
 
 static int64_t cpu_get_icount_raw_locked(void)
-- 
2.17.1

On 09/10/2018 04:27 PM, Emilio G. Cota wrote:
>  
> -#ifndef CONFIG_ATOMIC64
>      seqlock_write_lock(&timers_state.vm_clock_seqlock,
>                         &timers_state.vm_clock_lock);
> -#endif
>      atomic_set__nocheck(&timers_state.qemu_icount,
>                          timers_state.qemu_icount + executed);
> -#ifndef CONFIG_ATOMIC64
>      seqlock_write_unlock(&timers_state.vm_clock_seqlock,
>                           &timers_state.vm_clock_lock);
> -#endif

I don't understand this.  Surely you either want atomic64_set,
or an actual atomic64_add, but no extra lock when CONFIG_ATOMIC64.


r~

On 11/09/2018 14:53, Richard Henderson wrote:
>>  
>> -#ifndef CONFIG_ATOMIC64
>>      seqlock_write_lock(&timers_state.vm_clock_seqlock,
>>                         &timers_state.vm_clock_lock);
>> -#endif
>>      atomic_set__nocheck(&timers_state.qemu_icount,
>>                          timers_state.qemu_icount + executed);
>> -#ifndef CONFIG_ATOMIC64
>>      seqlock_write_unlock(&timers_state.vm_clock_seqlock,
>>                           &timers_state.vm_clock_lock);
>> -#endif
> I don't understand this.  Surely you either want atomic64_set,
> or an actual atomic64_add, but no extra lock when CONFIG_ATOMIC64.

Even though writes of qemu_icount can safely race with reads in
qemu_icount_raw, qemu_icount is also read by icount_adjust, which runs
in the I/O thread.  Therefore, writes do need protection of the
vm_clock_lock; for simplicity Emilio's patch protects it with both
seqlock+spinlock, which we already do for hosts that lack 64-bit atomics.

The bug actually predated the introduction of vm_clock_lock;
cpu_update_icount would have needed the BQL before the spinlock was
introduced.

The above could have been a commit message, by the way. :)

Paolo

On Mon, Sep 10, 2018 at 19:27:48 -0400, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  cpus.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index ebc13bac2d..38dabb138d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -250,16 +250,12 @@ void cpu_update_icount(CPUState *cpu)
>      int64_t executed = cpu_get_icount_executed(cpu);
>      cpu->icount_budget -= executed;
>  
> -#ifndef CONFIG_ATOMIC64
>      seqlock_write_lock(&timers_state.vm_clock_seqlock,
>                         &timers_state.vm_clock_lock);
> -#endif
>      atomic_set__nocheck(&timers_state.qemu_icount,
>                          timers_state.qemu_icount + executed);
> -#ifndef CONFIG_ATOMIC64
>      seqlock_write_unlock(&timers_state.vm_clock_seqlock,
>                           &timers_state.vm_clock_lock);
> -#endif
>  }

Applying this on my local tree is deadlocking icount, since
cpu_update_icount is called from cpu_get_icount_raw_locked:

#6  cpu_update_icount (cpu=<optimized out>) at /data/src/qemu/cpus.c:257
#7  0x000055a6fbc7ae5c in cpu_get_icount_raw_locked () at /data/src/qemu/cpus.c:271
#8  0x000055a6fbc7ae99 in cpu_get_icount_locked () at /data/src/qemu/cpus.c:279
#9  0x000055a6fbc7b3ac in cpu_get_icount () at /data/src/qemu/cpus.c:302
#10 0x000055a6fc0f3a05 in qemu_clock_get_ns (type=type@entry=QEMU_CLOCK_VIRTUAL) at /data/src/qemu/util/qemu-timer.c:601

I am however not sure what Paolo's queued tree looks like, so I
might be missing something.

Paolo: is that tree available anywhere?

Thanks,

		Emilio

On 24/09/2018 20:46, Emilio G. Cota wrote:
> Applying this on my local tree is deadlocking icount, since
> cpu_update_icount is called from cpu_get_icount_raw_locked:
> 
> #6  cpu_update_icount (cpu=<optimized out>) at /data/src/qemu/cpus.c:257
> #7  0x000055a6fbc7ae5c in cpu_get_icount_raw_locked () at /data/src/qemu/cpus.c:271
> #8  0x000055a6fbc7ae99 in cpu_get_icount_locked () at /data/src/qemu/cpus.c:279
> #9  0x000055a6fbc7b3ac in cpu_get_icount () at /data/src/qemu/cpus.c:302
> #10 0x000055a6fc0f3a05 in qemu_clock_get_ns (type=type@entry=QEMU_CLOCK_VIRTUAL) at /data/src/qemu/util/qemu-timer.c:601
> 
> I am however not sure what Paolo's queued tree looks like, so I
> might be missing something.

No, you're not missing anything.

Looking at other callers of cpu_update_icount, this should be the fix:

diff --git a/cpus.c b/cpus.c
index b429b5a9e0..551076fb98 100644
--- a/cpus.c
+++ b/cpus.c
@@ -245,15 +245,25 @@ static int64_t cpu_get_icount_executed(CPUState *cpu)
  * account executed instructions. This is done by the TCG vCPU
  * thread so the main-loop can see time has moved forward.
  */
-void cpu_update_icount(CPUState *cpu)
+static void cpu_update_icount_locked(CPUState *cpu)
 {
     int64_t executed = cpu_get_icount_executed(cpu);
     cpu->icount_budget -= executed;
 
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
     atomic_set_i64(&timers_state.qemu_icount,
                    timers_state.qemu_icount + executed);
+}
+
+/*
+ * Update the global shared timer_state.qemu_icount to take into
+ * account executed instructions. This is done by the TCG vCPU
+ * thread so the main-loop can see time has moved forward.
+ */
+void cpu_update_icount(CPUState *cpu)
+{
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    cpu_update_icount_locked(cpu);
     seqlock_write_unlock(&timers_state.vm_clock_seqlock,
                          &timers_state.vm_clock_lock);
 }
@@ -268,7 +278,7 @@ static int64_t cpu_get_icount_raw_locked(void)
             exit(1);
         }
         /* Take into account what has run */
-        cpu_update_icount(cpu);
+        cpu_update_icount_locked(cpu);
     }
     /* The read is protected by the seqlock, but needs atomic64 to avoid UB */
     return atomic_read_i64(&timers_state.qemu_icount);

> Paolo: is that tree available anywhere?

Nope, unfortunately some patches I've queued broke on 32-bit and stuff so I
couldn't send out the pull request before leaving for Kernel Recipes...

Paolo

On Wed, Sep 26, 2018 at 10:23:25 +0200, Paolo Bonzini wrote:
> On 24/09/2018 20:46, Emilio G. Cota wrote:
> > Applying this on my local tree is deadlocking icount, since
> > cpu_update_icount is called from cpu_get_icount_raw_locked:
> > 
> > #6  cpu_update_icount (cpu=<optimized out>) at /data/src/qemu/cpus.c:257
> > #7  0x000055a6fbc7ae5c in cpu_get_icount_raw_locked () at /data/src/qemu/cpus.c:271
> > #8  0x000055a6fbc7ae99 in cpu_get_icount_locked () at /data/src/qemu/cpus.c:279
> > #9  0x000055a6fbc7b3ac in cpu_get_icount () at /data/src/qemu/cpus.c:302
> > #10 0x000055a6fc0f3a05 in qemu_clock_get_ns (type=type@entry=QEMU_CLOCK_VIRTUAL) at /data/src/qemu/util/qemu-timer.c:601
> > 
> > I am however not sure what Paolo's queued tree looks like, so I
> > might be missing something.
> 
> No, you're not missing anything.
> 
> Looking at other callers of cpu_update_icount, this should be the fix:
> 
> diff --git a/cpus.c b/cpus.c
(snip)

This does indeed fix the deadlock. Feel free to add my

Tested-by: Emilio G. Cota <cota@braap.org>

in the eventual patch.

Thanks,

		Emilio