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