[asterisk-dev] Locking, coding guidelines addition

[Steve Totaro]

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