buglwIP - A Lightweight TCP/IP stack - Bugs: bug #23408, Deadlock on sys_mbox_post...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #23408: Deadlock on sys_mbox_post sys_mbox_fetch

Submitter:  Homyak <onlyslon>
Submitted:  Wed 28 May 2008 11:08:22 AM UTC
   
 
Category:  Contrib Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Jump to the original submission

Sat 15 Nov 2008 12:39:00 PM UTC, comment #12: 

Problem solved. Thank you Kieran.

Homyak <onlyslon>
Fri 30 May 2008 11:29:37 AM UTC, comment #11: 

I've checked in something that should fix it, but I'm not in a postition to test it right now.

Vadim: could you take a look and confirm that this solves the problem you're seeing.  We can then close this bug.

Kieran Mansley <kieranm>
Group Member
Fri 30 May 2008 11:15:36 AM UTC, comment #10: 

...and the suggested solution of separate semaphores for fetch and post should do the trick.  We could also solve it by taking the semaphore mutex before dropping the sys_mbox mutex, but that's a bit ugly.

I'll check something in.

Kieran Mansley <kieranm>
Group Member
Fri 30 May 2008 11:10:13 AM UTC, comment #9: 

OK, I think I understand.  Here is what I think, please correct me if I'm wrong:

1) Something calls sys_arch_mbox_fetch(), but the mailbox is empty, so it drops the mutex and it is about to wait for the mail semaphore to be signalled when it is interrupted by...

2) Something calls sys_mbox_post().  This puts something in the mailbox and because it was empty signals the mail semaphore.  If (1) had succeeded in getting to the point where it waits on the mail semaphore, it would have then be woken up to get that message.

3) sys_mbox_post() is then called a lot, until the mailbox is full, and it also goes to wait on the mail semaphore.  As it earlier signalled this (when it put the first message in) it will immediately wake, but then wait on the semaphore again as the condition it's waiting for has not been met.  In doing so it consumes the signal that should have gone to sys_arch_mbox_fetch()

4) sys_arch_mbox_fetch() is then re-scheduled and goes to wait on the mail semaphore.  The signal that should have woken it has been consumed by the posting thread, and so it blocks even though the mailbox now has things in to read.

Hence deadlock.

Kieran Mansley <kieranm>
Group Member
Wed 28 May 2008 05:44:12 PM UTC, comment #8: 

Comment #5 - a result of my negligence.  ;-)
Comment #6 -  much correctly.

i.e.
between
 sys_sem_signal(mbox->mutex);
and
 sys_arch_sem_wait(mbox->mail, 0);
from function sys_arch_mbox_fetch, possibly multiple execution of function sys_arch_mbox_post which can lock mbox->mail

Thats all. I think.






Homyak <onlyslon>
Wed 28 May 2008 05:19:49 PM UTC, comment #7: 

That explains why I didn't understand: I didn't use the unix port, so far.

But looking through it and your code, it seems you have mixed up the function names and contents in the first comment (in contrast to comment #5), which is why I still not really get it.

You first said you have one reader that is interrupted by the writer, but the code you said to be interrupted seems to be in the writer function sys_mbox_post, doesn't it? So who's interrupting who here?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 May 2008 05:08:53 PM UTC, comment #6: 

Sorry. A little tired.  This is corrected post.

u32_t
sys_arch_mbox_fetch(struct sys_mbox mbox, void *msg, u32_t timeout)
{
  u32_t time = 0;

  /* The mutex lock is quick so we don't bother with the timeout
     stuff here. */
  sys_arch_sem_wait(mbox->mutex, 0);

  while (mbox->first == mbox->last) {
    sys_sem_signal(mbox->mutex); <-------- 1st operation

    /* We block while waiting for a mail to arrive in the mailbox. We
       must be prepared to timeout. */
    if (timeout != 0) {
      time = sys_arch_sem_wait(mbox->mail, timeout);

      if (time == SYS_ARCH_TIMEOUT) {
        return SYS_ARCH_TIMEOUT;
      }
    } else {
/*
I want to say that between these two operations (1st and 2nd) possibly switch CPU context to writer thread and filling the whole messagebox, followed by blocking semaphore mbox->mail.

As a result we get a deadlock.

When a large amount of msgbox elements (SYS_MBOX_SIZE) that is likely to happen is small.

*/

      sys_arch_sem_wait(mbox->mail, 0); <-------- 2nd operation
    }
    sys_arch_sem_wait(mbox->mutex, 0);
  }

  if (msg != NULL) {
    LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_fetch: mbox %p msg %p\n", (void *)mbox, *msg));
    *msg = mbox->msgs[mbox->first % SYS_MBOX_SIZE];
  }
  else{
    LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_fetch: mbox %p, null msg\n", (void *)mbox));
  }

  mbox->first++;

  if (mbox->wait_send) {
    sys_sem_signal(mbox->mail);
  }

  sys_sem_signal(mbox->mutex);

  return time;
}




Homyak <onlyslon>
Wed 28 May 2008 04:56:35 PM UTC, comment #5: 

Ok I try again. 8-). Sorry for my english.

I use lwip v 1.3.0 under Linux.

my sys_arch file is contrib/ports/unix/sys_arch.c


void
sys_mbox_post(struct sys_mbox *mbox, void *msg)
{
  u8_t first;

  sys_sem_wait(mbox->mutex);

  LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_post: mbox %p msg %p\n", (void )mbox, (void )msg));

  while ((mbox->last + 1) >= (mbox->first + SYS_MBOX_SIZE)) {
    mbox->wait_send++;
    sys_sem_signal(mbox->mutex); <------ 1st operation

/*
I want to say that between these two operations (1st and 2nd) possibly switch CPU context to writer thread and filling the whole messagebox, followed by blocking semaphore mbox->mail.

As a result we get a deadlock.

When a large amount of msgbox elements (SYS_MBOX_SIZE) that is likely to happen is small.

*/

    sys_arch_sem_wait(mbox->mail, 0); <-------- 2nd operation
    sys_arch_sem_wait(mbox->mutex, 0);
    mbox->wait_send--;
  }

  mbox->msgs[mbox->last % SYS_MBOX_SIZE] = msg;

  if (mbox->last == mbox->first) {
    first = 1;
  } else {
    first = 0;
  }

  mbox->last++;

  if (first) {
    sys_sem_signal(mbox->mail);
  }

  sys_sem_signal(mbox->mutex);
}





Homyak <onlyslon>
Wed 28 May 2008 04:04:08 PM UTC, comment #4: 

I'm afraid I still don't get what you're talking about.

Your using mbox->mail and mbox->mutex suggests to me you are talking about a sys_archo port of lwIP? Can you tell us which?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 May 2008 12:55:33 PM UTC, comment #3: 

version 1.3.0

ok. 2 threads.

1st   -  read from msgbox
2nd    - write

Situation: msgbox is fully empty

thread #1 (reader) block mbox->mail


sys_arch_mbox_fetch(struct sys_mbox mbox, void *msg, u32_t timeout)
.
.
#check for empty
while (mbox->first == mbox->last)
{
sys_sem_signal(mbox->mutex);
# writers are waked.
# Between this operations write thread(s) post messages. Fill all msgbox and  lock mbox->mail
sys_arch_sem_wait(mbox->mail, 0); <--- DEADLOCK
sys_arch_sem_wait(mbox->mutex, 0);
}

perhaps I missed something....


Homyak <onlyslon>
Wed 28 May 2008 11:23:33 AM UTC, comment #2: 

Can you describe your problem a little more detailed? Which version of lwIP are you using? A problem like this should only occur if you try to write to a mailbox which is full and simply drop the mail (which is wrong, of course). This should be solved in 1.3.0, I think.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 May 2008 11:22:59 AM UTC, comment #1: 

Can you explain more about the deadlock you experienced.  I don't understand how it arose.

Kieran Mansley <kieranm>
Group Member
Wed 28 May 2008 11:08:22 AM UTC, original submission:  

Hello..

On small values of SYS_MBOX_SIZE (40 and less)
mbox reader and writer threads stops on sys_arch_sem_wait(mbox->mail, 0);

Solution for this bug is separate semaphores.
First one -  for busy
Second -  for empty event

Like this...

struct sys_mbox {
  int first, last;
  void *msgs[SYS_MBOX_SIZE];
  struct sys_sem *mutex;
  int wait_send;
  int mbox_size;
  struct sys_sem *can_post_mail;
  struct sys_sem *can_fetch_mail;
};

sys_mbox_post(struct sys_mbox *mbox, void *msg)
{
  u8_t first;

  sys_sem_wait(mbox->mutex);

  LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_post: mbox %p msg %p\n", (void )mbox, (void )msg));

  while ((mbox->last + 1) >= (mbox->first + SYS_MBOX_SIZE)) {
    mbox->wait_send++;
    sys_sem_signal(mbox->mutex);
-    sys_arch_sem_wait(mbox->mail, 0);
+   sys_arch_sem_wait(mbox->can_post_mail, 0);
    sys_arch_sem_wait(mbox->mutex, 0);
    mbox->wait_send--;
  }

  mbox->msgs[mbox->last % SYS_MBOX_SIZE] = msg;

  if (mbox->last == mbox->first) {
    first = 1;
  } else {
    first = 0;
  }

  mbox->last++;

  if (first) {
-    sys_sem_signal(mbox->mail)
+    sys_sem_signal(mbox->can_fetch_mail);
  }

  sys_sem_signal(mbox->mutex);
}


u32_t
sys_arch_mbox_fetch(struct sys_mbox mbox, void *msg, u32_t timeout)
{
  u32_t time = 0;

  /* The mutex lock is quick so we don't bother with the timeout
     stuff here. */
  sys_arch_sem_wait(mbox->mutex, 0);

  while (mbox->first == mbox->last) {
    sys_sem_signal(mbox->mutex);

    /* We block while waiting for a mail to arrive in the mailbox. We
       must be prepared to timeout. */
    if (timeout != 0) {
-      time = sys_arch_sem_wait(mbox->mail, timeout);
+      time = sys_arch_sem_wait(mbox->can_fetch_mail, timeout);

      if (time == SYS_ARCH_TIMEOUT) {
        return SYS_ARCH_TIMEOUT;
      }
    } else {
      sys_arch_sem_wait(mbox->mail, 0);
      sys_arch_sem_wait(mbox->can_fetch_mail, 0);
    }

    sys_arch_sem_wait(mbox->mutex, 0);
  }

  if (msg != NULL) {
    LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_fetch: mbox %p msg %p\n", (void *)mbox, *msg));
    *msg = mbox->msgs[mbox->first % SYS_MBOX_SIZE];
  }
  else{
    LWIP_DEBUGF(SYS_DEBUG, ("sys_mbox_fetch: mbox %p, null msg\n", (void *)mbox));
  }

  mbox->first++;

  if (mbox->wait_send) {
-    sys_sem_signal(mbox->mail);
+    sys_sem_signal(mbox->can_post_mail)
  }

  sys_sem_signal(mbox->mutex);

  return time;
}









Homyak <onlyslon>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by onlyslon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-11-17 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2008-05-30 kieranm Assigned toNone kieranm
        Planned Release 1.3.1

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code