Sean Bruno <sbr...@freebsd.org> writes:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
>
> On 03/21/16 08:56, Paolo Bonzini wrote:
>>
>>
>> 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
>>
>>
>
>
> Alex:
>
> Do you want me to work up a patch for this or are you dealing with it?
I wasn't going to look at it for a few days (other things on my queue)
so if you have time to re-write with the seqlock_read_* loop please be
my guest.
>
> sean
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQF8BAEBCgBmBQJW8W6IXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
> ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
> MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kCPMH/3dbvHDfw5fxY8AX3mBoEsby
> 10+JEG8u2HojS37h8xx+gxV5ZI9xFUMVQ8niM5EdCrMUz1YeH3oyVYu6LBrlCm9T
> vlsG0huOXkNuVP+++nAVGr/bPm08IIUBYi1MNVSOZ02MxLgeWblsF4Z9slX5KrH2
> FS49z7vDZYeJF2OWxBF/PHvFomNiqfOnsKelh8cWX6FDIubBSrz2dmHvKUu7Jumw
> EKeZlHP2G+QiuqdUl4t0zvrBYo15IGLUGXNZrp0zgEd2usieA80I1Vb3JQCfrFL1
> GFZo7j86W1iXxGTwI//rM52bXAskk8HovtgtkwRbKG18SGpZJzhjqwQAFsDyuUU=
> =p9Xu
> -----END PGP SIGNATURE-----
--
Alex Bennée
