[asterisk-dev] Locking, coding guidelines addition

[Russell Bryant]

Steve Murphy wrote:
> I'm sorry, but on the subject of channel and pvt locking, I have a
> different thought, and I hope it's neither too late nor an unwelcome
> thought. Perhaps  it's outrageous or impossible... but...
> 
> I keep asking myself, "Why do we have separate locks for the pvt and
> channel structs, why not consolidate them into a single lock?"
> 
> A channel lock should be obtained before a pvt lock. So, theoretically,
> the pvt lock might provide a finer locking... on a subset of the channel
> data. But, since any other section of code that might want to play with
> the pvt data can't/shouldn't do so until it has the channel lock first,
> well, it's already covered!
> 
> Also, why this arbitrary separation of channel and pvt data? Isn't the 
> whole concept of pvt data just because a subset of the channel data is
> specific to a certain technology? Pvt data is really channel data, it's
> just part of a conceptual union (implemented with a pointer). Pvt data
> is really just channel data. 
> 
> Well, most likely I'm missing some critical factoid that demands this 
> separation. But if all it is one small (but seemingly critical) factoid,
> can't we work around it somehow and unify the two?
> 
> It would seem to me that if unified the pvt and channel data (at least
> lockwise), it would simplify the locking situation tremendously. And 
> having a channel driver just tweak pvt data wouldn't require any 
> lock-order reversals, etc.

Yes, it would simplify the locking scheme quite a bit.  I have thought 
about this, too, but haven't gone too far into it.  It has basically 
been sitting in the back of my mind as a thing that I would have done 
differently, given the chance to re-do some things.

1) You said the pvt is a subset of an ast_channel, but it's actually an 
extension of the ast_channel.  If this were C++ land, I would say that 
ast_channel is an abstract class, and sip_pvt, iax2_pvt, etc., along 
with their associated ast_channel_tech function table, are 
implementations of the abstract class.

2) #1 is only partially true.  The pvt structs in channel drivers get 
used for much more than simply the channel specific data for an 
associated ast_channel.  For example, the sip_pvt structure has been 
overloaded to represent all dialogs, such as registrations and such, 
even those that are not associated with a channel.

This would have to be greatly cleaned up if we went this route.

3) Channel locks are used to synchronize things in media processing.  If 
a channel driver decided to hold on to a lock too long for poking at pvt 
data, it would affect audio processing, where that is not the case 
today.  I'm not sure if this is really a realistic concern, though.

4) chan_local/chan_agent are another weird case that would have to be 
carefully considered.  There, we have a single local_pvt for 2 parent 
ast_channels.

> As Russell has previously said, if we astobj2-ify the channels, that
> would eliminate the possibility of channels disappearing while a 
> structure was being accessed. (if we are really, really careful 
> about how/where/when we remove refs and destroy things)... 
> And, we can define different astobj2 tables for different
> kinds of find_channel needs... a complete by-name search would be
> one table. We could handle name-prefix searches with a trie; channel
> by exten with another hash table... and so on.

I've got some of this done.  However, making the reference count _truly_ 
correct across the entire code base is a far more daunting task than I 
have attempted to complete, given the pervasiveness of the ast_channel 
data structure.  Deciding what to do about this is something that I need 
to bring up in a separate discussion.

> If we rwlock the channel, we could allow several competing threads to
> at least access (just reading) a channel's info without blocking 
> each other out.

This isn't really feasible, unfortunately.  pthread rwlocks do not 
support being recursive, and there is a _lot_ of code that makes 
assumptions about the channel lock being recursive.

All of this is not impossible, of course, but it's one of the most 
incredibly invasive architecture changes proposed since I have been 
involved with the project.

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.