[asterisk-dev] channel.c locking and other

[Mark Michelson]

Russell Bryant wrote:
> Damien Wedhorn wrote:
>> 4 why does __ast_queue_frame use the channel lock. Couldn't we queue and 
>> dequeue based on a list lock? This is the biggest for me. If we used a 
>> list lock or similar, I could remove most (if not all) chan locking in 
>> chan_skinny. May apply to other channels as well. However, I'm not 
>> familiar enough with all the ins and outs of channel.c (and how it 
>> relates to all the other code) to know if this is feasible.
> 
> This is a valid suggestion.  You are correct that it would simplify some 
> of the channel locking complexity in channel driver code.  A patch that 
> implements this technique would be welcome.
> 
> It may be a bit more complicated than it sounds when you get into the 
> details.  This lock would also have to be used to synchronize access to 
> the channel's timing interface, for example.  I'd have to put some more 
> thought into this to think of all of the dirty details ...
> 

This suggestion applies to chan_local as well. Big time.

In a recent code review I posted (http://reviewboard.digium.com/r/219/), I took 
a different approach to this problem than what is suggested here. Instead of 
adding a list lock to the channel's frame queue, I instead added a task 
processor to the local_pvt struct. I pushed the frame queueing operation to the 
task processor so that it would be executed in a different thread. My idea could 
be made more generic by adding a taskprocessor to the ast_channel struct instead 
and changing the ast_queue_frame function to push the frame queuing execution to 
the task processor instead. In addition, there could be API calls added for 
pushing tasks to the channel task processor.

I'll list some pros and cons to the approaches listed here:

1. Lock on the channel's frame queue.
     + Does not require any extra threads
     - As Russell suggested, this may not be as straightforward as it seems.
     - Only would solve deadlock issues with regards to queueing frames.

2. Taskprocessor on the ast_channel.
     + Allows for a generic method of asynchronously pushing tasks to a channel*.
     - Essentially will double the number of running threads on a system (could 
be the killer for this suggestion)
     - Does not solve problems where we must lock a channel and perform a 
synchronous operation**

* I found this to be necessary when fixing up chan_local. The reason was that I 
needed to grab a channel's lock for performing tasks like setting flags.

** This could be fixed by adding an API call to the task processor system such 
as ast_taskprocessor_push_sync. This would use either a sem_t or ast_cond_t to 
wait until the task processor has finished executing its task. For situations 
where we have the tech_pvt locked but need to perform a task that involves 
locking the channel, this would work wonderfully. However, as I learned while 
messing with chan_local, you must be careful with your locking habits if trying 
to do something like this. For example, don't try to lock a channel from a 
synchronous task processor callback if the thread which pushed the task already 
has that channel locked; that is guaranteed to cause a deadlock.

Mark Michelson