On 21/03/2016 16:36, Alex Bennée wrote: > > 341 /* The icount_warp_timer is rescheduled soon after > > vm_clock_warp_start > > 342 * changes from -1 to another value, so the race here is okay. > > 343 */ > > 344 if (atomic_read(&vm_clock_warp_start) == -1) { > > 345 return; > > 346 } > > 347 > Odd, the comments say that vm_clock_warp start is protected by the > seqlock, and in fact every other access to it is a plain access.
Yes, the comment says why this is safe. The change from -1 to positive is here: if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) { vm_clock_warp_start = clock; } seqlock_write_unlock(&timers_state.vm_clock_seqlock); timer_mod_anticipate(icount_warp_timer, clock + deadline); If we get a race we must be like this: icount_warp_rt qemu_start_warp_timer -------------- --------------------- read -1 write to vm_clock_warp_start unlock timer_mod_anticipate (*) As soon as you reach (*) the timer is rescheduled and will read a value other than -1. > It seems to me the code should probably just be: > > seqlock_write_lock(&timers_state.vm_clock_seqlock); > if (vm_clock_warp_start !== -1 && runstate_is_running()) { > .. do stuff .. > } > vm_clock_warp_start = -1; > seqlock_write_unlock(&timers_state.vm_clock_seqlock); > > if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { > qemu_clock_notify(QEMU_CLOCK_VIRTUAL); > } Yes, you can make it like that, or even better wrap the read with a seqlock_read_begin/seqlock_read_retry loop. The condition will often be false and it's pointless to lock/unlock the mutex for that. Paolo