[asterisk-dev] Locking, coding guidelines addition

[Steve Totaro]

On Fri, Jul 4, 2008 at 2:25 PM, Steve Totaro
<stotaro at totarotechnologies.com> wrote:
> On Fri, Jul 4, 2008 at 2:17 PM, Stephen Davies
> <stephen.l.davies at gmail.com> wrote:
>>
>>
>> 2008/7/4 Russell Bryant <russell at digium.com>:
>>>
>>> ----- "Stephen Davies" <stephen.l.davies at gmail.com> wrote:
>>> > I think the discussion reveals that the magic trylock while loop looks
>>> > neato but doesn't really solve the problem.  Tzafrir and Luigi pointed
>>> > out some flaws.
>>>
>>> It doesn't just look neato.  It does actually work, except for when the
>>> recursive property of the locks are used.
>>
>>
>> OK point taken.   Ish.  Because interestingly I've fixed a deadlock recently
>> in chan_local that was to do with the fact that a mutex had two outstanding
>> locks.  So when the magic trylock look in local_queue_frame tried to do its
>> thing, the underlying lock never got released and you STILL end up with a
>> deadlock (or livelock)
>>
>> Here was the fix for that:
>>
>> Index: channels/chan_local.c
>> ===================================================================
>> --- channels/chan_local.c (revision 105553)
>> +++ channels/chan_local.c (working copy)
>> @@ -153,6 +153,7 @@
>>  static int local_queue_frame(struct local_pvt *p, int isoutbound, struct
>> ast_frame *f, struct ast_channel *us)
>>  {
>>   struct ast_channel *other = NULL;
>> + int doublelock = 0;
>>
>>   /* Recalculate outbound channel */
>>   other = isoutbound ? p->owner : p->chan;
>> @@ -175,11 +176,23 @@
>>   /* Ensure that we have both channels locked */
>>   while (other && ast_channel_trylock(other)) {
>>   ast_mutex_unlock(&p->lock);
>> - if (us)
>> + if (us) {
>>   ast_channel_unlock(us);
>> + /* SLD: In some circumstances with generators running on the local channel
>> it is possible
>> +   for there to be two locks on the us channel.  Essential to drop them
>> both to avoid a
>> +   deadlock */
>> + if (!ast_channel_unlock(us)) {
>> + doublelock = 1;
>> + if (option_debug > 2)
>> + ast_log(LOG_VERBOSE, "local_queue_frame had to unlock \"us\" twice\n");
>> + }
>> + }
>>   usleep(1);
>> - if (us)
>> + if (us) {
>>   ast_channel_lock(us);
>> + if (doublelock)
>> + ast_mutex_lock(us);
>> + }
>>   ast_mutex_lock(&p->lock);
>>   other = isoutbound ? p->owner : p->chan;
>>   }
>>
>> Thinking back, there was a point in the past when the use of recursive locks
>> was started.   Or maybe I'm remembering wrong and it was a point at which
>> they became mandatory.
>>
>> Anyway - I guess that the magic trylock look technique was used way back
>> before the recursive mutexes came in.
>>
>> <goes to look at 1.0.9...>
>>
>> Yep - I can see it in there; though often without the while.
>>
>> But even 1.0.9 Asterisk required recursive mutexes - so it was before
>> then...
>>
>> Steve
>>
>
> Call me Mr. Obvious, but why not use locks like Callweaver (the entire
> reason it was created)?
>
> I stay far, far, as far away as I can from IAX, that is unless I am
> fixing a customer's system and switch from IAX to SIP, and make a nice
> profit.
>
> Thanks,
> Steve T
>

Sorry, meant FreeSwitch.

http://freeswitch.org/node/117

Thanks,
Steve T