[asterisk-dev] channel.c locking and other

[Russell Bryant]

Damien Wedhorn wrote:
> I'm tidying up chan_skinny's chan->tech_pvt handling which sort of meant 
> screwing with the locking. To this end, I've removed a bunch of locks 
> from chan_skinny so I basically only have a device lock and use the chan 
> lock in combination with the device lock when required. Anyway, to avoid 
> deadlocking, I basically make sure I have the chan lock before calling 
> any channel.c functions.

That sounds fine.  Just make sure that you follow the rule that you must 
acquire the channel lock before your private lock.

> I thought this a bit overzealous and so had a look through channel.c and 
> ended up with a bunch of wtf's. Admitting that I really haven't looked 
> at channel.c in much detail previously, I noted the following:

> 1 ast_queue_hangup basically goes "!trylock(set softhangup, unlock), 
> lock, add queue frame, unlock". Why not just lock the chan to begin with 
> rather than unlocking it to immediately relock it?

I have no idea.  That code is just plain wrong.  It _might_ set the 
softhangup field ... or it might not ... ugh.

> 2 ast_queue_frame_head's only use appears to be to fix AST_LIST_ADD_HEAD 
> in ast_answer. If AST_LIST_ADD_TAIL was used, the function and 
> associated code for ast_queue_frame_head could be removed.

I do not think this is correct.  This function is required because there 
are cases where the channel readq has frames in it, but we need to 
insert frames to be read _before_ frames that were already there.  This 
is also done in autoservice.c.  No matter what, we're going to require 
the ability to queue frames on both ends of the queue.

> 3 chan->_softhangup seems to be set locked and unlocked. It's a bit mask 
> and some comparisons are ==, it is sometimes specifically assigned 
> values without checking whether something is already there. Overall 
> there doesn't appear to be enough consistency to ensure that set values 
> aren't overwritten.

Yep ...

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

> Just a general comment. Some doxygen comments for channel.c indicate 
> that chan_locking is not required for certain functions. It may be worth 
> including whether or not that function locks the chan, because if it 
> does and a thread holds another lock (not a chan lock), the chan may 
> need to be locked to avoid a potential deadlock. Also, some functions 
> have comments that they do not lock the chan when they actually do.

Absolutely.  At one point, I started adding those comments for this 
exact reason.  If there are some comments that are wrong, we definitely 
need to fix them.  Also, if someone wants to go through and fill out the 
documentation of what functions do with the channel lock, that would be 
very welcome.

> I'm aware that a lot of this is probably just leftovers from previous 
> code, but I would especially like some feedback on the feasibility/issue 
> of using a list lock rather than the chan lock for frame 
> queueing/dequeueing.

It's not a trivial job, but it does seem like it is worth pursuing. 
I'll let it roll around in my head a bit more to see if I think of 
anything that would be a show stopper.

Also, if you want svn access for working on this in a branch, feel free 
to contact me off-list.

-- 
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org