[prev in list] [next in list] [prev in thread] [next in thread] 

List:       asterisk-dev
Subject:    Re: [asterisk-dev] [Code Review] Add common implementation for a
From:       "Russell Bryant" <russell () digium ! com>
Date:       2009-01-26 22:40:26
Message-ID: 20090126224026.23034.6485 () hotblack ! digium ! internal
[Download RAW message or body]



> 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
> 
> 


_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev


[prev in list] [next in list] [prev in thread] [next in thread]