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
