Re: [Qemu-devel] [PATCH v2 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Stefan Hajnoczi]

On Tue, Aug 27, 2013 at 09:55:22AM +0100, Alex Bligh wrote:
> On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote:
> > diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> > index e4934dd..b58903b 100644
> > --- a/include/qemu/timer.h
> > +++ b/include/qemu/timer.h
> > @@ -115,6 +115,10 @@ static inline int64_t qemu_clock_get_us(QEMUClockType 
> > type)
> >  * Determines whether a clock's default timer list
> >  * has timers attached
> >  *
> > + * Note that this function should not be used when other threads also 
> > access
> > + * the timer list.  The return value may be outdated by the time it is 
> > acted
> > + * upon.
> > + *
> 
> I'm in two minds as to whether there is a problem with the comment or the
> code here. Despite git's best endeavours, this is a comment on 
> qemu_clock_has_timers,
> which is called in qemu_clock_warp in cpus.c. I'm still a little confused as
> to what thread(s) that can be called from.

Remember icount is a TCG feature and will only be accessed from a single
thread.

> > @@ -531,6 +545,9 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
> >  *
> >  * Modify a timer to expiry at @expire_time, taking into
> >  * account the scale associated with the timer.
> > + *
> > + * This function is thread-safe but the timer and its timer list must not 
> > be
> > + * freed while this function is running.
> >  */
> > void timer_mod(QEMUTimer *ts, int64_t expire_timer);
> 
> timer_mod and timer_mod_ns are inevitably called having just read the
> clock with qemu_clock_get. I am presuming these are thread safe (I haven't
> checked) and if so the documentation for them should be similar adjusted.
> Otherwise making these thread safe is all but useless.

Ping Fan's series deals with thread-safe clock sources, this series only
tackles the timer_list.

We need to apply both series to be able to use timers from threads.

Stefan