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

Submitted by:  Thomas Taranowski <taranowski>
Submitted on:  Tue 21 Aug 2007 12:26:35 AM UTC  
 
Category: ContribSeverity: 2 - Minor
Item Group: Faulty BehaviourStatus: Invalid
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: None

(Jump to the original submission 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project Member

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by fbernon (Posted a comment)
  • -unavailable- added by jifl (Posted a comment)
  • -unavailable- added by taranowski (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 21 Aug 2007 11:33:16 PM UTCtaranowskiStatusNone=>Invalid
      Open/ClosedOpen=>Closed
      Summaryunix sys_arch_protect faulty=>unix sys_arch_protect faulty - deferred to task 7212
    Tue 21 Aug 2007 11:10:29 PM UTCtaranowskiSeverity3 - Normal=>2 - Minor

    Back to the top


    Powered by Savane 3.1-cleanup1