[asterisk-dev] Locking, coding guidelines addition

[Stephen Davies]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20080704/ef8f8a59/attachment.htm