Re: [8/11] kernel32: Implement the timer queue thread.

[Dan Hipschman]

Thanks for these especially good comments, Rob!

On Sat, Jul 19, 2008 at 07:08:17PM +0100, Rob Shearman wrote:
> 2008/7/19 Dan Hipschman :
> > This patch implements the timer thread, running callbacks, cleaning up
> > timers, and all that good stuff.  It should be very efficient, using mostly
> > atomic operations and one critical section for thread safety.
>
> It would be more efficient for queues with large numbers of timers if
> you used the approach used by server/fd.c of keeping a list of timers
> sorted by when they next expire to avoid having to search through the
> entire list every time a timer expires.

The answer to most of your questions is, because I was trying to start
simple.  I'll look into this, though.
> > +static void queue_timer_expire(struct queue_timer *t)
> > +{
> > - /* We MUST hold the queue cs while calling this function.  */
> > - if (!t->die)
> > - {
> > -
> >    ULONG flags =
> > -
> >        t->flags & (WT_EXECUTEINIOTHREAD | WT_EXECUTEINPERSISTENTTHREAD
> > -
> >                    | WT_EXECUTELONGFUNCTION | WT_TRANSFER_IMPERSONATION);
> > -
> >    InterlockedIncrement(&t->runcount);
> > -
> >    QueueUserWorkItem(timer_callback_wrapper, t, flags);
>
> You should check the return value and undo the incrementing of
> t->runcount if it failed. However, you're also calling
> QueueUserWorkItem inside a critical section so you're imposing a lock
> ordering on the loader lock. This may cause a deadlock that may or may
> not exist on Windows if a timer queue function is called from an
> application's DllMain.

Thanks for telling me about this.  It should be safe to move
QueueUserWorkItem outside the CS.  I'll think about it later.
> It would be better to use a thread pool thread for processing timer
> queue since it can be reused when the timer queue is destroyed. It
> might then also be better to only start the timer queue thread when
> there are timers to process and also release the timer queue thread
> back to the thread pool when the last timer is removed from the queue.
>
> Also, have you thought about using one thread for all timer queues?
> This might not be appropriate for WT_EXECUTEINTIMERTHREAD timers, but
> should improve the performance of the process as a whole without
> affecting other timers.

Yes, I thought about these, but again, trying to keep it simple...
> > @@ -1085,22 +1226,32 @@ BOOL WINAPI DeleteTimerQueueEx(HANDLE TimerQueue, HANDLE CompletionEvent)
> >     struct queue_timer *t, *temp;
> >     BOOL ret;
> > - RtlEnterCriticalSection(&q->cs);
> > - LIST_FOR_EACH_ENTRY_SAFE(t, temp, &q->timers, struct queue_timer, entry)
> > -
> >    t->die = TRUE;
> > - q->quit = TRUE;
> > - RtlLeaveCriticalSection(&q->cs);
> > - SetEvent(q->event);
> > - if (CompletionEvent == INVALID_HANDLE_VALUE)
> >   {
> >       ret = TRUE;
> > -
> >    WaitForSingleObject(q->thread, INFINITE);
>
> What if timer_queue_thread_proc exits before you reach here? q->thread
> will now be invalid (and could be re-used by another thread in the
> mean time). Another way of looking at it is that you don't own q after
> the critical section where q->quit is set.

This is a great catch.  I utterly missed this.  Maybe the easiest fix is
to dup the thread handle into a local.  I'll look for a cleaner approach
as well.
> If you use an expire list in a local variable then you won't need
> locking when processing the expired timers and so this will be trivial
> to implement.

I'll take another look.
> Quite a few comments, but the patches are of a good general quality,
> so well done.

Thanks for taking the time to go over this so thoroughly.  Thanks, Hans,
for the comments as well; I'll check out GetTickCount64.

Dan