| Message ID | 20180910232752.31565-9-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | i386 + x86_64 mttcg | expand |
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
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)
Signed-off-by: Emilio G. Cota <cota@braap.org> --- cpus.c | 4 ---- 1 file changed, 4 deletions(-)