On 01/09/2018 00:07, Emilio G. Cota wrote: > On Mon, Aug 20, 2018 at 17:09:02 +0200, Paolo Bonzini wrote: >> In the next patch, we will need to write cpu_ticks_offset from any >> thread, even outside the BQL. Currently, it is protected by the BQL >> just because cpu_enable_ticks and cpu_disable_ticks happen to hold it, >> but the critical sections are well delimited and it's easy to remove >> the BQL dependency. >> >> Add a spinlock that matches vm_clock_seqlock, and hold it when writing >> to the TimerState. This also lets us fix cpu_update_icount when 64-bit >> atomics are not available. >> >> Fields of TiemrState are reordered to avoid padding. >> >> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> >> --- >> cpus.c | 72 ++++++++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 47 insertions(+), 25 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index 680706aefa..63ddd4fd21 100644 >> --- a/cpus.c >> +++ b/cpus.c > (snip) >> @@ -244,11 +250,15 @@ void cpu_update_icount(CPUState *cpu) >> int64_t executed = cpu_get_icount_executed(cpu); >> cpu->icount_budget -= executed; >> >> -#ifdef CONFIG_ATOMIC64 >> +#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); >> -#else /* FIXME: we need 64bit atomics to do this safely */ >> - timers_state.qemu_icount += executed; >> +#ifndef CONFIG_ATOMIC64 >> + seqlock_write_unlock(&timers_state.vm_clock_seqlock, >> + &timers_state.vm_clock_lock); >> #endif > > > I'm puzzled by this hunk. Why are we only adding the seqlock_write > if !CONFIG_ATOMIC64, if we always read .qemu_icount with seqlock_read?
Yeah, I missed that qemu_icount is read by icount_adjust and so it indirectly affects qemu_icount_bias. It needs the seqlock always. Paolo
