Re: [ros-svn] [hbirr] 14443: - Fixed ExTimerRundown.

[Alex Ionescu]

hbirr(a)svn.reactos.com wrote:

>  - Fixed ExTimerRundown.  
>   
>  
Hi,

Can you please explain some of your changes? You have introduced several 
changes that I don't understand, such as cancelling the timer's APC 
without actually making sure that it has an APC associated, as well as 
slowing down the path and forcing additionnal locking of the DB lock by 
using KeCancelTimer (which also uselessly checks if the timer is 
inserted -- we are sure it already is).

Apart from that, thanks for fixing the silly bugs!

Best regards,
Alex Ionescu

[Phillip Susi]

I just glanced at the diffs on that change and it looks like it is 
releasing the global timer lock and then grabbing the lock on the 
individual timer.  Doesn't that create a race condition where the timer 
can be deleted in between releasing the global lock and grabbing the 
local?  Shouldn't that order be reversed?


Alex Ionescu wrote:

>   hbirr(a)svn.reactos.com wrote:
>
> >   - Fixed ExTimerRundown.   
> >  
>  Hi,
>
>  Can you please explain some of your changes? You have introduced 
>  several changes that I don't understand, such as cancelling the 
>  timer's APC without actually making sure that it has an APC 
>  associated, as well as slowing down the path and forcing additionnal 
>  locking of the DB lock by using KeCancelTimer (which also uselessly 
>  checks if the timer is inserted -- we are sure it already is).
>
>  Apart from that, thanks for fixing the silly bugs!
>
>  Best regards,
>  Alex Ionescu
>

[Hartmut Birr]

Phillip Susi wrote:

>   I just glanced at the diffs on that change and it
> looks like it is
>  releasing the global timer lock and then grabbing the lock on the
>  individual timer.  Doesn't that create a race condition where the
>  timer can be deleted in between releasing the global lock and grabbing
>  the local?  Shouldn't that order be reversed? 
It isn't possible to delete the timer within this gap. The timer has an
additional reference which depends on ApcAssociated. ApcAssociated is
only changed if the local lock is held.

- Hartmut

[Hartmut Birr]

Alex Ionescu wrote:

>   hbirr(a)svn.reactos.com wrote:
>
> >   - Fixed ExTimerRundown.   
> >  
>  Hi,
>
>  Can you please explain some of your changes? You have introduced
>  several changes that I don't understand, such as cancelling the
>  timer's APC without actually making sure that it has an APC
>  associated, as well as slowing down the path and forcing additionnal
>  locking of the DB lock by using KeCancelTimer (which also uselessly
>  checks if the timer is inserted -- we are sure it already is). 
Hi,

the thread's active timer list can only contain timers which have an apc
associated. It isn't necessary to check for an apc. There exist a little
gap between deliver the apc and calling the apc routine which can
removes the timer itself from the list. In this case the timers apc and
dpc aren't on the apc and dpc list and some of the called functions do
nothing (KeRemoveQueueDpc, KeRemoveQueueApc, KeCancelTimer). The goal
for my changes was get the apc2 sample working again.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:

>  Alex Ionescu wrote:
>
>   
>
> >  hbirr(a)svn.reactos.com wrote:
> >
> >     
> >
> > >  - Fixed ExTimerRundown.   
> > >
> > >       
> > >  
> > Hi,
> >
> > Can you please explain some of your changes? You have introduced
> > several changes that I don't understand, such as cancelling the
> > timer's APC without actually making sure that it has an APC
> > associated, as well as slowing down the path and forcing additionnal
> > locking of the DB lock by using KeCancelTimer (which also uselessly
> > checks if the timer is inserted -- we are sure it already is).
> >     
> >  
> Hi,
>
> the thread's active timer list can only contain timers which have an apc
> associated. It isn't necessary to check for an apc. There exist a little
> gap between deliver the apc and calling the apc routine which can
> removes the timer itself from the list. In this case the timers apc and
> dpc aren't on the apc and dpc list and some of the called functions do
> nothing (KeRemoveQueueDpc, KeRemoveQueueApc, KeCancelTimer). The goal
> for my changes was get the apc2 sample working again.
> \
>   
>  
Hi,

You are absolutely right, but I was still afraid that ApcPresent might 
be set to FALSE while the timer is still on the list,  but even then, as 
you said, the functions don't do anything.

I still think however that it would be wiser to inline the KeCancelTimer 
call and avoid the Dispatcher lock acquire/release and inserted check..

Best regards,
Alex Ionescu