buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20842, unix sys_arch_protect faulty -...

 
 

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

bug #20842: unix sys_arch_protect faulty - deferred to task 7212

Submitter:  Thomas Taranowski <taranowski>
Submitted:  Tue 21 Aug 2007 12:26:35 AM UTC
   
 
Category:  Contrib Severity:  2 - Minor
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Tue 21 Aug 2007 11:33:16 PM UTC, comment #8: 

The proposed solution isn't the right one for memp.c, as that file needs a file-scope mutex.  The required solution will be implemented in task #7212.

Thomas Taranowski <taranowski>
Group Member
Tue 21 Aug 2007 11:10:29 PM UTC, comment #7: 

My sys_arch differs slightly from the unix, and I don't have a unix test bed, so I'm putting what I got, which should be readily translatable to the unix version... 

Note that each function that  contains a critical section declares a single static semaphore that's local to the function.  With this method the critical section is function-local, rather than system-wide, which appears to be the correct semantics, given the code that is current using it.

In sys_arch.h
--------------
#define SYS_ARCH_DECL_PROTECT(lev)  static sys_sem_t localsem = (sys_sem_t)0xffffffff; static unsigned protct=0; static thread_handle_t protthread=(thread_handle_t)0xffffffff; if(localsem==(sys_sem_t)0xffffffff) localsem=sys_sem_new(1);


#define SYS_ARCH_PROTECT(lev) sys_arch_protect(localsem, &protct, &protthread)

#define SYS_ARCH_UNPROTECT(lev) sys_arch_unprotect(localsem, &protct, &protthread)

In sys_arch.c
-------------
sys_prot_t sys_arch_protect(sys_sem_t localsem, unsigned *protct, thread_handle_t *protthread)
{
 
  if(protthread!=currentThreadHandle()) {
    We have a new thread that wants to lock some critical session.  Wait until it gets it.
    sys_arch_sem_wait(localsem,0);
    *protthread = currentThreadHandle();
    *protct=1;
  } else {
    The thread handle matches, so this thread already has it locked!Bump the count
    (*protct)++;   
  }
 

  return 0;
}
void sys_arch_unprotect(sys_sem_t localsem, unsigned *protct, thread_handle_t *protthread)
{
  LWIP_ASSERT("sys_arch_unprotect: Not the current critical section locker.",
    *protthread==currentThreadHandle());

  //I have it locked, unprotecting
  (*protct)--;
  if(*protct==0) {
    reset the thread id
    *protthread=0xffffffff;
    release the semaphore
    sys_sem_signal(localsem);
  }



Thomas Taranowski <taranowski>
Group Member
Tue 21 Aug 2007 06:12:39 PM UTC, comment #6: 

So, I propose to close this item, and to continue on https://savannah.nongnu.org/task/?7212. Agree Thomas?

Frédéric Bernon <fbernon>
Group Member
Tue 21 Aug 2007 02:11:27 PM UTC, comment #5: 

I agree with Frederic. There's nothing that says SYS_ARCH_PROTECT should only protect that one critical region. For example, it's used to protect memp metadata in both memp_alloc and memp_free. Different critical regions of code, but they both have to be protected because it's the data that's being protected, not the critical region.

Yes mutexes might help, although not all systems have recursive mutexes. I'm interested to see that recursive mutexes are in SUS2. They are not in POSIX 1003.1c-1995, so they aren't that portable in practice.

The problem with mutexes in some places like the memp code is that, for example, you may be doing a pbuf_alloc(PBUF_POOL,... from an interrupt context (due to a new incoming packet), and in an interrupt context you cannot block. Which means you can't block on a mutex.

Jonathan Larmour <jifl>
Group Member
Tue 21 Aug 2007 07:40:11 AM UTC, comment #4: 

But perhaps this lightweight mutex is "faster" than unix implementation ? I don't know...

But except that which is more "coding style", perhaps your "problem" Thomas is that you think the SYS_ARCH_PROTECT should not be based on a unique global mutex, to avoid to block taskA during a pbuf reference decrement, while taskB do a memp_malloc (or any longer processing)?

But the design of SYS_ARCH_DECL_PROTECT / SYS_ARCH_PROTECT/ SYS_ARCH_UNPROTECT macros seems more based on a interrupt level feature (where the sys_prot_t type/parameter is useful), than on a mutex one (where, since only a global mutex is possible, sys_prot_t type/parameter is not useful).

That's why I have propose in https://savannah.nongnu.org/task/?7212 to introduce a real mutex feature (as SYS_ARCH_PROTECT is global, and sys_sem_t can't be really recursive).

So, the problem is in all ports (not only in the unix one), and I think we can close it as "invalid". Right?

Frédéric Bernon <fbernon>
Group Member
Tue 21 Aug 2007 07:21:47 AM UTC, comment #3: 

If the mutex was created with PTHREAD_MUTEX_RECURSIVE attribute with a pthread_mutex_init, I think it will be better than using the macro PTHREAD_MUTEX_INITIALIZER. In this case, you could have :

sys_prot_t sys_arch_protect(void)
{ return pthread_mutex_lock(&lwprot_mutex);
}

Or even a define...

http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_init.html
http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_lock.html

Frédéric Bernon <fbernon>
Group Member
Tue 21 Aug 2007 01:33:26 AM UTC, comment #2: 

Well, that's true enough, but isn't what you want as it induces an artificial coupling.  Thread tB is blocked by tA, even though it doesn't need to be, as they are working through different critical sections.

Thomas Taranowski <taranowski>
Group Member
Tue 21 Aug 2007 01:18:54 AM UTC, comment #1: 

I don't see the issue. Thread tB would block in pthread_mutex_lock() until thread tA unlocked the mutex completely.

Jonathan Larmour <jifl>
Group Member
Tue 21 Aug 2007 12:26:35 AM UTC, original submission:  

The unix version of sys_arch_protect appears faulty.

Assume the following scenario:
I have two critical sections in different parts of the code, cA and cB, and two threads, tA and tB.  Thread tA wants to protect cA, etc.

Thread tA calls SYS_ARCH_PROTECT to protect a critical section.  The lwprot_thread!=current thread, so tA locks the mutex, and bumps the count to 1.

Next, thread tB also calls SYS_ARCH_PROTECT to protect cB.  Since the single global lwprot_thread!=current thread, it will overwrite the lwprot_thread id with it's won, and reset the lwprot_count to 1.



Code excerpt below - comments stripped out for readability:

sys_prot_t sys_arch_protect(void)
{
    if (lwprot_thread != pthread_self())
    {
        pthread_mutex_lock(&lwprot_mutex);
        lwprot_thread = pthread_self();
        lwprot_count = 1;
    }
    else
        lwprot_count++;
    return 0;
}

Thomas Taranowski <taranowski>
Group Member

 

(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 fbernon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by taranowski (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
    2007-08-21 taranowski StatusNone Invalid
        Open/ClosedOpen Closed
        Summaryunix sys_arch_protect faulty unix sys_arch_protect faulty - deferred to task 7212
    2007-08-21 taranowski Severity3 - Normal 2 - Minor

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code