[asterisk-dev] [Code Review] Add common implementation for a scheduler context with a dedicated thread

Russell Bryant russell at digium.com
Mon Jan 26 16:40:26 CST 2009



> On 2009-01-26 10:51:47, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 1263-1268
> > <http://reviewboard.digium.com/r/129/diff/1/?file=2315#file2315line1263>
> >
> >     Why is it that scheduler entry replacement is not an atomic operation? At the least, I would expect a lock to be held around the calls to ast_sched_thread_del and ast_sched_thread_add so that nothing could happen between the deletion and addition. I ask also because the pre-existing AST_SCHED_REPLACE macro does not hold a lock around the removal and re-addition of scheduler entries.
> >     
> >     Even though replacement can be accomplished as you have done here with separate calls to delete and add a scheduler entry, why not add a simple public API function for replacing scheduler entries (e.g. ast_sched_thread_replace)?
> 
>  wrote:
>     Yeah, I think sched_thread_replace() is a good idea.  I'll add it.
> 
>  wrote:
>     After looking at this a bit closer, I'm having trouble coming up with a reason that this operation needs to be atomic.  Do you have a case where it not being atomic could be problematic?  The issues I thought of and have been addressing have to do with synchronization between the scheduler thread and the add operations.  The deletes really aren't that big of a deal.
> 
>  wrote:
>     No, I don't know of any specific situations where this needs to be atomic. That's partially why I asked the question in the first place. I noticed the operation was not atomic, and I was curious as to why. It may be that it is not atomic simply for the reason that it doesn't actually hurt anything to not operate atomically.
>     
>     The question about the atomicity of the replacement operation was more to indulge my curiosity than to state how I think it should be. My thought process was that a replacement operation would be atomic, so I asked about it.
> 
>  wrote:
>     Thanks for the feedback.  I just wanted to clarify.  I have thought about and I think for replace, there is just no need for it to be atomic.
> 
>  wrote:
>     There is actually good reason for it to be atomic.  Consider the possible collision of 2 threads:
>     A: delete job, set id to -1
>     B: Check id, see that it is -1 (nothing to cancel).
>     B: Add job, set id.
>     A: Add job, set id (B's id is lost and isn't cancellable).
>     
>     Furthermore:
>     
>     C: delete job, set id to -1.
>     C: deallocate memory
>     SCHED: job runs from what B scheduled, referencing free'd memory.

Thanks for the feedback, Tilghman.  However, I do not think that either of those cases are related to synchronization requirements that should be enforced by this API.

The first case should be handled by proper external locking of the data structure holding the scheduler ID.  Having two threads both operate on the same data like that is broken from a higher level.

In the second case, this would not be possible if proper reference counting is being done for the object with respect to scheduler entries.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/129/#review321
-----------------------------------------------------------


On 2009-01-26 15:21:38, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/129/
> -----------------------------------------------------------
> 
> (Updated 2009-01-26 15:21:38)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There are a number of modules that create a scheduler context.  Some use a dedicated thread, while others process the scheduler context within the context of a thread that handles other things as well.  I have noticed a number of bugs in both types of implementations related to race conditions between checking how much time until the next scheduled entry and sleeping for an appropriate amount of time.
> 
> To address the problems found in the implementations that use a dedicated thread, I have written this patch.  This patch adds a common implementation of a scheduler context that uses a dedicated thread for processing.
> 
> chan_iax2 has been updated to use this new API for its dedicated scheduler thread instead of the one that was written into chan_iax2 directly.  The previous implementation has some race conditions that can lead to the scheduler thread sleeping longer than it is supposed to, leading to scheduled actions not running when they are supposed to.  Bugs caused by this type of thing are often very subtle and difficult to track down.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_iax2.c 171450 
>   /trunk/include/asterisk/sched.h 171450 
>   /trunk/main/sched.c 171450 
> 
> Diff: http://reviewboard.digium.com/r/129/diff
> 
> 
> Testing
> -------
> 
> It compiles.  Basic chan_iax2 usage appears unaffected.
> 
> 
> Thanks,
> 
> Russell
> 
>




More information about the asterisk-dev mailing list