From 363403fb1f135319e0a2c401b2caa63539bfb080 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Sun, 26 Mar 2017 10:35:23 -0600 Subject: [PATCH] pthreads: Add more robustness characteristics: pthread_mutex_lock() and trylock() will now return EOWNERDEAD if the mutex is locked by a thread that no longer exists. Add pthread_mutex_consistent() to recover from this situation. --- TODO | 36 ++++++- include/pthread.h | 6 +- sched/pthread/Make.defs | 1 + sched/pthread/pthread_mutexconsistent.c | 138 ++++++++++++++++++++++++ sched/pthread/pthread_mutexdestroy.c | 96 +++++++++++------ sched/pthread/pthread_mutexlock.c | 53 ++++++--- sched/pthread/pthread_mutextrylock.c | 90 ++++++++++------ sched/pthread/pthread_mutexunlock.c | 15 ++- 8 files changed, 343 insertions(+), 92 deletions(-) create mode 100644 sched/pthread/pthread_mutexconsistent.c diff --git a/TODO b/TODO index ad47655bf40..b0bb4f5017d 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated March 14, 2017) +NuttX TODO List (Last updated March 26, 2017) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -14,7 +14,7 @@ nuttx/: (1) Memory Management (mm/) (0) Power Management (drivers/pm) (3) Signals (sched/signal, arch/) - (2) pthreads (sched/pthread) + (4) pthreads (sched/pthread) (0) Message Queues (sched/mqueue) (8) Kernel/Protected Build (3) C++ Support @@ -346,7 +346,7 @@ o Signals (sched/signal, arch/) Priority: Low. Even if there are only 31 usable signals, that is still a lot. o pthreads (sched/pthreads) - ^^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^ Title: PTHREAD_PRIO_PROTECT Description: Extend pthread_mutexattr_setprotocol(). It should support @@ -448,6 +448,36 @@ o pthreads (sched/pthreads) Status: Not really open. This is just the way it is. Priority: Nothing additional is planned. + Title: PTHREAD FILES IN WRONG LOCATTION + Description: There are many pthread interface functions in files located in + sched/pthread. These should be moved from that location to + libc/pthread. In the flat build, this really does not matter, + but in the protected build that location means that system calls + are required to access the pthread interface functions. + Status: Open + Priority: Medium-low. Priority may be higher if system call overheade becomes + an issue. + + Title: ROBUST MUTEX ATTRIBUTE NOT SUPPORTED + Description: In NuttX, all mutexes are 'robust' in the sense that an attmpt + to lock a mutex will return EOWNDERDEAD if the holder of the + mutex has died. Unlocking of a mutex will fail if the caller + is not the holder of the mutex. + + POSIX, however, requires that there be a mutex attribute called + robust that determines which behavior is supported. non-robust + should be the default. NuttX does not support this attribute + and robust behavior is the default and only supported behavior. + + The spec is not clear, but I think there there is also missing + logic when the thread exits. I believe that the next highest + prority thread waiting for the mutex should be awakend and + pthread_mutex_lock() should return EOWNERDEAD. + That does not happen now. They will just remain blocked. + Status: Open + Priority: Low. The non-robust behavior is dangerous and really should never + be used. + o Message Queues (sched/mqueue) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/include/pthread.h b/include/pthread.h index d943a843e2f..6fed93a8371 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -1,7 +1,7 @@ /******************************************************************************** * include/pthread.h * - * Copyright (C) 2007-2009, 2011-2012, 2015-2016 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2011-2012, 2015-2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -424,6 +424,10 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex); int pthread_mutex_trylock(FAR pthread_mutex_t *mutex); int pthread_mutex_unlock(FAR pthread_mutex_t *mutex); +/* Make sure that the pthread mutex is in a consistent state */ + +int pthread_mutex_consistent(FAR pthread_mutex_t *mutex); + /* Operations on condition variables */ int pthread_condattr_init(FAR pthread_condattr_t *attr); diff --git a/sched/pthread/Make.defs b/sched/pthread/Make.defs index 6156dd54112..5a602d3e1fa 100644 --- a/sched/pthread/Make.defs +++ b/sched/pthread/Make.defs @@ -39,6 +39,7 @@ CSRCS += pthread_create.c pthread_exit.c pthread_join.c pthread_detach.c CSRCS += pthread_yield.c pthread_getschedparam.c pthread_setschedparam.c CSRCS += pthread_mutexinit.c pthread_mutexdestroy.c CSRCS += pthread_mutexlock.c pthread_mutextrylock.c pthread_mutexunlock.c +CSRCS += pthread_mutexconsistent.c CSRCS += pthread_condinit.c pthread_conddestroy.c CSRCS += pthread_condwait.c pthread_condsignal.c pthread_condbroadcast.c CSRCS += pthread_barrierinit.c pthread_barrierdestroy.c pthread_barrierwait.c diff --git a/sched/pthread/pthread_mutexconsistent.c b/sched/pthread/pthread_mutexconsistent.c new file mode 100644 index 00000000000..0579cb3841d --- /dev/null +++ b/sched/pthread/pthread_mutexconsistent.c @@ -0,0 +1,138 @@ +/**************************************************************************** + * sched/pthread/pthread_mutexconsistent.c + * + * Copyright (C) 2017 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * 3. Neither the name NuttX nor the names of its contributors may be + * used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include + +#include +#include +#include +#include + +#include + +#include "pthread/pthread.h" + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: pthread_mutex_consistent + * + * Description: + * If mutex is a robust mutex in an inconsistent state, the + * pthread_mutex_consistent() function can be used to mark the state + * protected by the mutex referenced by mutex as consistent again. + * + * If an owner of a robust mutex terminates while holding the mutex, the + * mutex becomes inconsistent and the next thread that acquires the mutex + * lock will be notified of the state by the return value EOWNERDEAD. + * In this case, the mutex does not become normally usable again until the + * state is marked consistent. + * + * If the thread which acquired the mutex lock with the return value + * EOWNERDEAD terminates before calling either pthread_mutex_consistent() + * or pthread_mutex_unlock(), the next thread that acquires the mutex lock + * will be notified about the state of the mutex by the return value + * EOWNERDEAD. + * + * The behavior is undefined if the value specified by the mutex argument + * to pthread_mutex_consistent() does not refer to an initialized mutex. + * + * Input Parameters: + * mutex -- a reference to the mutex to be made consistent + * + * Returned Value: + * Upon successful completion, the pthread_mutex_consistent() function + * will return zero. Otherwise, an error value will be returned to + * indicate the error. + * + ****************************************************************************/ + +int pthread_mutex_consistent(FAR pthread_mutex_t *mutex) +{ + int ret = EINVAL; + int status; + + sinfo("mutex=0x%p\n", mutex); + DEBUGASSERT(mutex != NULL); + + if (mutex != NULL) + { + /* Make sure the mutex is stable while we make the following checks. */ + + sched_lock(); + + /* Is the mutex available? */ + + DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */ + if (mutex->pid >= 0) + { + /* No.. Verify that the PID still exists. We may be destroying + * the mutex after cancelling a pthread and the mutex may have + * been in a bad state owned by the dead pthread. NOTE: The + * folling is unspecified behavior (see pthread_mutex_consistent()). + * + * If the holding thread is still valid, then we should be able to + * map its PID to the underlying TCB. That is what sched_gettcb() + * does. + */ + + if (sched_gettcb(mutex->pid) == NULL) + { + /* The thread associated with the PID no longer exists */ + + mutex->pid = -1; + + /* Reset the semaphore. This has the same affect as if the + * dead task had called pthread_mutex_unlock(). + */ + + status = sem_reset((FAR sem_t *)&mutex->sem, 1); + ret = (status != OK) ? get_errno() : OK; + } + } + + sched_unlock(); + ret = OK; + } + + sinfo("Returning %d\n", ret); + return ret; +} diff --git a/sched/pthread/pthread_mutexdestroy.c b/sched/pthread/pthread_mutexdestroy.c index 8bab62c4743..23905fd407e 100644 --- a/sched/pthread/pthread_mutexdestroy.c +++ b/sched/pthread/pthread_mutexdestroy.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/pthread/pthread_mutexdestroy.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -46,6 +46,8 @@ #include #include +#include + #include "pthread/pthread.h" /**************************************************************************** @@ -70,60 +72,86 @@ int pthread_mutex_destroy(FAR pthread_mutex_t *mutex) { - int ret = OK; + int ret = EINVAL; int status; sinfo("mutex=0x%p\n", mutex); + DEBUGASSERT(mutex != NULL); - if (mutex == NULL) + if (mutex != NULL) { - ret = EINVAL; - } - else - { - /* Make sure the semaphore is stable while we make the following - * checks - */ + /* Make sure the semaphore is stable while we make the following checks */ sched_lock(); - /* Is the semaphore available? */ + /* Is the mutex available? */ - if (mutex->pid != -1) + if (mutex->pid >= 0) { -#ifndef CONFIG_DISABLE_SIGNALS - /* Verify that the PID still exists. We may be destroying the - * mutex after cancelling a pthread and the mutex may have been - * in a bad state owned by the dead pthread. + DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */ + + /* No.. Verify that the PID still exists. We may be destroying + * the mutex after cancelling a pthread and the mutex may have + * been in a bad state owned by the dead pthread. NOTE: The + * following behavior is unspecified for pthread_mutex_destroy() + * (see pthread_mutex_consistent()). + * + * If the holding thread is still valid, then we should be able to + * map its PID to the underlying TCB. That is what sched_gettcb() + * does. */ - ret = kill(mutex->pid, 0); - if (ret < 0) - { - /* The thread associated with the PID no longer exists */ + if (sched_gettcb(mutex->pid) == NULL) + { + /* The thread associated with the PID no longer exists */ - mutex->pid = -1; + mutex->pid = -1; - /* Destroy the semaphore */ + /* Reset the semaphore. If threads are were on this + * semaphore, then this will awakened them and make + * destruction of the semaphore impossible here. + */ - status = sem_destroy((FAR sem_t *)&mutex->sem); - ret = (status != OK) ? get_errno() : OK; + status = sem_reset((FAR sem_t *)&mutex->sem, 1); + if (status < 0) + { + ret = -status; + } + + /* Check if the reset caused some other thread to lock the + * mutex. + */ + + else if (mutex->pid != -1) + { + /* Yes.. then we cannot destroy the mutex now. */ + + ret = EBUSY; + } + + /* Destroy the underlying semaphore */ + + else + { + status = sem_destroy((FAR sem_t *)&mutex->sem); + ret = (status != OK) ? get_errno() : OK; + } + } + else + { + ret = EBUSY; } - else -#endif - { - ret = EBUSY; - } } else { - /* Destroy the semaphore */ + /* Destroy the semaphore + * + * REVISIT: What if there are threads waiting on the semaphore? + * Perhaps this logic should all sem_reset() first? + */ status = sem_destroy((FAR sem_t *)&mutex->sem); - if (status != OK) - { - ret = get_errno(); - } + ret = ((status != OK) ? get_errno() : OK); } sched_unlock(); diff --git a/sched/pthread/pthread_mutexlock.c b/sched/pthread/pthread_mutexlock.c index d5c8af2db2f..e14a7de17ec 100644 --- a/sched/pthread/pthread_mutexlock.c +++ b/sched/pthread/pthread_mutexlock.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/pthread/pthread_mutexlock.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -42,9 +42,12 @@ #include #include #include +#include #include #include +#include + #include "pthread/pthread.h" /**************************************************************************** @@ -104,15 +107,12 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex) { int mypid = (int)getpid(); - int ret = OK; + int ret = EINVAL; sinfo("mutex=0x%p\n", mutex); + DEBUGASSERT(mutex != NULL); - if (!mutex) - { - ret = EINVAL; - } - else + if (mutex != NULL) { /* Make sure the semaphore is stable while we make the following * checks. This all needs to be one atomic action. @@ -120,7 +120,7 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex) sched_lock(); - /* Does this task already hold the semaphore? */ + /* Does this thread already hold the semaphore? */ if (mutex->pid == mypid) { @@ -129,9 +129,12 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex) #ifdef CONFIG_MUTEX_TYPES if (mutex->type == PTHREAD_MUTEX_RECURSIVE) { - /* Yes... just increment the number of locks held and return success */ + /* Yes... just increment the number of locks held and return + * success. + */ mutex->nlocks++; + ret = OK; } else #endif @@ -139,19 +142,39 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex) /* No, then we would deadlock... return an error (default * behavior is like PTHREAD_MUTEX_ERRORCHECK) * - * NOTE: This is non-compliant behavior for the case of a - * NORMAL mutex. In that case, it the deadlock condition should - * not be detected and the thread should be permitted to - * deadlock. + * NOTE: This is the correct behavior for a 'robust', NORMAL + * mutex. Compiant behavior for non-robust mutex should not + * include these checks. In that case, it the deadlock + * condition should not be detected and the thread should be + * permitted to deadlock. */ serr("ERROR: Returning EDEADLK\n"); ret = EDEADLK; } } + + /* The calling thread does not hold the semaphore. The correct + * behavior for the 'robust' mutex is to verify that the holder of the + * mutex is still valid. This is protection from the case + * where the holder of the mutex has exitted without unlocking it. + */ + + else if (mutex->pid > 0 && sched_gettcb(mutex->pid) == NULL) + { + DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */ + + /* A thread holds the mutex, but there is no such thread. POSIX + * requires that the 'robust' mutex return EOWNERDEAD in this case. + * It is then the caller's responsibility to call pthread_mutx_consistent() + * fo fix the mutex. + */ + + ret = EOWNERDEAD; + } else { - /* Take the semaphore */ + /* Take the underlying semaphore, waiting if necessary */ ret = pthread_takesemaphore((FAR sem_t *)&mutex->sem); @@ -159,7 +182,7 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex) * that we own it. */ - if (!ret) + if (ret == OK) { mutex->pid = mypid; #ifdef CONFIG_MUTEX_TYPES diff --git a/sched/pthread/pthread_mutextrylock.c b/sched/pthread/pthread_mutextrylock.c index 825cb992a14..a71e845f77d 100644 --- a/sched/pthread/pthread_mutextrylock.c +++ b/sched/pthread/pthread_mutextrylock.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/pthread/pthread_mutextrylock.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -83,15 +84,13 @@ int pthread_mutex_trylock(FAR pthread_mutex_t *mutex) { - int ret = OK; + int status; + int ret = EINVAL; sinfo("mutex=0x%p\n", mutex); + DEBUGASSERT(mutex != NULL); - if (!mutex) - { - ret = EINVAL; - } - else + if (mutex != NULL) { int mypid = (int)getpid(); @@ -103,7 +102,8 @@ int pthread_mutex_trylock(FAR pthread_mutex_t *mutex) /* Try to get the semaphore. */ - if (sem_trywait((FAR sem_t *)&mutex->sem) == OK) + status = sem_trywait((FAR sem_t *)&mutex->sem); + if (status == OK) { /* If we successfully obtained the semaphore, then indicate * that we own it. @@ -117,33 +117,63 @@ int pthread_mutex_trylock(FAR pthread_mutex_t *mutex) mutex->nlocks = 1; } #endif + ret = OK; } - /* Was it not available? */ + /* sem_trywait failed */ - else if (get_errno() == EAGAIN) - { -#ifdef CONFIG_MUTEX_TYPES - - /* Check if recursive mutex was locked by ourself. */ - - if (mutex->type == PTHREAD_MUTEX_RECURSIVE && mutex->pid == mypid) - { - /* Increment the number of locks held and return successfully. */ - - mutex->nlocks++; - } - else - { - ret = EBUSY; - } -#else - ret = EBUSY; -#endif - } else { - ret = EINVAL; + /* Did it fail because the semaphore was not avaialabl? */ + + int errcode = get_errno(); + if (errcode == EAGAIN) + { +#ifdef CONFIG_MUTEX_TYPES + /* Check if recursive mutex was locked by the calling thread. */ + + if (mutex->type == PTHREAD_MUTEX_RECURSIVE && mutex->pid == mypid) + { + /* Increment the number of locks held and return successfully. */ + + mutex->nlocks++; + ret = OK; + } + else +#endif + /* The calling thread does not hold the semaphore. The correct + * behavior for the 'robust' mutex is to verify that the holder of + * the mutex is still valid. This is protection from the case + * where the holder of the mutex has exitted without unlocking it. + */ + + if (mutex->pid > 0 && sched_gettcb(mutex->pid) == NULL) + { + DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */ + + /* A thread holds the mutex, but there is no such thread. + * POSIX requires that the 'robust' mutex return EOWNERDEAD + * in this case. It is then the caller's responsibility to + * call pthread_mutx_consistent() fo fix the mutex. + */ + + ret = EOWNERDEAD; + } + + /* The mutex is locked by another, active thread */ + + else + { + ret = EBUSY; + } + } + + /* Some other, unhandled error occurred */ + + else + { + ret = errcode; + } } sched_unlock(); diff --git a/sched/pthread/pthread_mutexunlock.c b/sched/pthread/pthread_mutexunlock.c index 56c14bb0367..9493d96e6cd 100644 --- a/sched/pthread/pthread_mutexunlock.c +++ b/sched/pthread/pthread_mutexunlock.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/pthread/pthread_mutexunlock.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -80,15 +80,12 @@ int pthread_mutex_unlock(FAR pthread_mutex_t *mutex) { - int ret = OK; + int ret = EINVAL; sinfo("mutex=0x%p\n", mutex); + DEBUGASSERT(mutex != NULL); - if (!mutex) - { - ret = EINVAL; - } - else + if (mutex != NULL) { /* Make sure the semaphore is stable while we make the following * checks. This all needs to be one atomic action. @@ -108,8 +105,7 @@ int pthread_mutex_unlock(FAR pthread_mutex_t *mutex) * the mutex." * * For the case of the non-robust PTHREAD_MUTEX_NORMAL mutex, - * the behavior is undefined. Here we treat that type as though - * it were PTHREAD_MUTEX_ERRORCHECK type and just return an error. + * the behavior is undefined. */ serr("ERROR: Holder=%d returning EPERM\n", mutex->pid); @@ -127,6 +123,7 @@ int pthread_mutex_unlock(FAR pthread_mutex_t *mutex) */ mutex->nlocks--; + ret = OK; } #endif