[libvirt] [PATCH v3 1/2] timer impl

[Wen Congyang]

At 12/18/2010 06:39 AM, Eric Blake Write:
> On 12/17/2010 01:49 AM, Wen Congyang wrote:
>> * tools/timer.c tools/timer.h: timer implementation
>> * tools/virsh.c: Initialize timer
>> * tools/Makefile.am: build timer
>>
>> Signed-off-by: Wen Congyang <wency at cn.fujitsu.com>
>>
>> +#define VIR_FROM_THIS VIR_FROM_NONE
>> +
>> +struct virTimer {
>> +    int timer_id;
>> +
>> +    int frequency;
> 
> In which unit?  milliseconds?  Worse, this variable is unused.

I forgot to remove this variable.

> 
>> +/* use timerFunc to prevent the user know timer id. */
>> +static void timerFunc(int timer_id ATTRIBUTE_UNUSED, void *opaque)
>> +{
>> +    virTimerPtr timer = (virTimerPtr)opaque;
> 
> This cast is not necessary (in general, in C you can go from void* to
> any other type without a cast).
> 
>> +    timer->function(timer->opaque);
>> +}
>> +
>> +virTimerPtr virNewTimer(virTimerCallback callback, void *opaque)
> 
> Would you mind adding some documentation to each public function,
> describing the parameters and return value?  Right now you're only using
> it with virsh, but it may make sense to move it to src/util, and good
> documentation will ensure that we like the interface (or the act of
> documenting it may help you find ways to improve it).
Ok, I will add some documentation to each public function.

If the interface is moved to src/util, I think we should move daemon/event.c
to src/util too, because we use the API virEventRunOnce().

> 
>> +
>> +int virAddTimer(virTimerPtr timer, int expire_time)
>> +{
>> +    int ret;
>> +
>> +    if ((ret = virEventAddTimeout(expire_time, timerFunc,
>> +                                  timer, NULL)) < 0) {
> 
> Is this function safe to call when the timer is already in use?  If not,
> should you add some sanity checking?
> 
>> +int virModTimer(virTimerPtr timer, int expire_time)
>> +{
>> +    if (timer->timer_id == -1)
>> +        return virAddTimer(timer, expire_time);
>> +
>> +    virEventUpdateTimeout(timer->timer_id, expire_time);
>> +    return 0;
> 
> Do we need both Add and Update, or can we have a single SetTimeout
> interface that nukes the previous timeout value and replaces it with the

I will use one single virSetTimeout() to replace Add, Update, and Delete().

> 
>> +}
>> +
>> +int virDelTimer(virTimerPtr timer)
>> +{
>> +    if (timer->timer_id == -1)
>> +        return 0;
>> +
>> +    if (virEventRemoveTimeout(timer->timer_id) < 0)
>> +        return -1;
> 
> If you merge Add and Mod into one interface, then having a sentiel
> expire_time value of -1 would also serve as a way to disarm any existing
> timer, so you could also merge Del into that same interface.

If we should disarm any existing timer, we should save all existing timer...
And the caller pass the timer to the single interface virSetTimeout(), so we
should only disarm the specified timer.

> 
>> +void virFreeTimer(virTimerPtr timer)
>> +{
> 
> if (timer == NULL) return;
> 
>> +    VIR_FREE(timer);
> 
> Also, modify cfg.mk to list this as a free-like function.

OK.

> 
>> +}
>> +
>> +static int timer_initialized = 0;
>> +static virThread timer_thread;
>> +static bool timer_running = false;
>> +static bool timer_quit = false;
> 
> Explicit zero-initialization is not required for file-scope variables (C
> guarantees it) - three instances.
> 
>> +int virTimerInitialize(void)
>> +{
>> +    if (timer_initialized)
>> +        return 0;
>> +
>> +    if (virMutexInit(&timer_lock) < 0)
>> +        return -1;
>> +
>> +    timer_initialized = 1;
>> +    timer_running = false;
>> +    timer_quit = false;
> 
> At least two bad data races if we expose virTimerInitialize to a
> multi-threaded environment (and probably more):
> 
> Thread 1 calls virTimerInitialize, calls virMutexInit, but gets swapped
> out before time_initialized is set to 1; thread 2 calls
> virTimerInitialize and tries to reinitialize the mutex, which results in
> undefined behavior.
> 
> Thread 1 calls virTimerInitialize, sets timer_initialized to 1, but gets
> swapped out before timer_running is set to false; thread 2 calls
> virTimerInitialize, which returns, then starts a timer; thread 1 resumes
> and sets timer_running to false, nuking thread 2's timer.
> 
> 
> I'm thinking we need to teach src/util/threads.h about some guaranteed
> once-only wrappers around pthread_once and the Windows counterpart, and
> then use virOnce throughout our code base to guarantee race-free
> one-time initialization for things such as virMutexInit, so that we are
> immune to data races where two threads both try to initialize the same
> mutex (this affects more than just your patch); once you have race-free
> mutex initialization, you can then use that mutex to reliably avoid the
> remaining races.
> 
> But for your particular use case, you are only using timers in the
> context of a single-threaded virsh, so it's something we can defer to
> later without impacting the utility of your patch.
> 
>> +
>> +typedef struct virTimer virTimer;
> 
> Our convention is:
> 
> typedef struct _virTimer virTimer;
> 
> that is, the struct version has a leading _ if the type is intentionally
> opaque.
> 
>> +typedef virTimer *virTimerPtr;
>> +typedef void (*virTimerCallback)(void *);
>> +
>> +extern virTimerPtr virNewTimer(virTimerCallback, void *);
> 
> ATTRIBUTE_NONNULL(1)
> 
>> +extern int virAddTimer(virTimerPtr, int);
> 
> ATTRIBUTE_NONNULL(1)
> 
>> +extern int virModTimer(virTimerPtr, int);
> 
> ATTRIBUTE_NONNULL(1)
> 
>> +extern int virDelTimer(virTimerPtr);
> 
> ATTRIBUTE_NONNULL(1)
> 
>> +extern void virFreeTimer(virTimerPtr);
>> +extern int virStartTimer(void);
>> +extern int virStopTimer(void);
>> +extern int virTimerInitialize(void);
> 
> Is the intent here that virTimerInitialize creates the timer resources,
> then you can use virStartTimer/virStopTimer in pairs at will to control
> whether timers are in effect, and finally virFreeTimer to remove all
> resources?  I'm agreeing with Hu that this seems a bit complex, and you
> may be better served by merging virStartTimer into virTimerInitialize (a
> one-shot call to enable you to call virAddTimer/virFreeTimer at will),
> and delete virStopTimer (the one-time resource never needs to be
> reclaimed).  See how we have virThreadInitialize, called once at
> initialization, with no other functions to control whether thread
> resources are in use.
> 
OK.

Thanks for your comment.