Module: xenomai-forge
Branch: next
Commit: 412a1286b42f368417027302d4017c89f138d922
URL:    
http://git.xenomai.org/?p=xenomai-forge.git;a=commit;h=412a1286b42f368417027302d4017c89f138d922

Author: Philippe Gerum <r...@xenomai.org>
Date:   Sun May 12 11:37:41 2013 +0200

copperplate/threadobj: defer TCB release until wait on barrier is over 
(valgrind)

This clears a valgrind warning which may occur in the following
situation:

Thread A                Thread B

threadobj_lock(B)
threadobj_start(B)
/* wait for B active */
                        threadobj_prologue(B)
                        /* set B active */
                        pthread_exit()
                        threadobj_free(B)
pthread_cond_wait(&B->barrier)
threadobj_unlock(B)

In such a case, thread A would tread on free memory while waking up on
the start barrier.

The thread finalizer now refrains from releasing the current TCB if
threadobj_start() is still ongoing, waiting for the latter to reclaim
the resources safely.

---

 include/copperplate/threadobj.h |   13 +++--
 lib/alchemy/task.c              |   13 ++++-
 lib/copperplate/threadobj.c     |  115 ++++++++++++++++++++++++++-------------
 lib/psos/task.c                 |    5 +-
 lib/vxworks/taskLib.c           |    6 ++-
 5 files changed, 100 insertions(+), 52 deletions(-)

diff --git a/include/copperplate/threadobj.h b/include/copperplate/threadobj.h
index 8e2c235..01a4010 100644
--- a/include/copperplate/threadobj.h
+++ b/include/copperplate/threadobj.h
@@ -118,16 +118,17 @@ void threadobj_save_timeout(struct threadobj_corespec 
*corespec,
 #define __THREAD_S_LOCKED      (1 << 5)        /* threadobj_lock() granted 
(debug only). */
 #define __THREAD_S_ACTIVE      (1 << 6)        /* Running user code. */
 #define __THREAD_S_SUSPENDED   (1 << 7)        /* Suspended via 
threadobj_suspend(). */
-#define __THREAD_S_DEBUG       (1 << 15)       /* Debug mode enabled. */
+#define __THREAD_S_SAFE                (1 << 8)        /* TCB release 
deferred. */
+#define __THREAD_S_DEBUG       (1 << 31)       /* Debug mode enabled. */
 /*
  * threadobj->run_state, locklessly updated by "current", merged
  * with ->status bits by threadobj_get_status().
  */
 #define __THREAD_S_RUNNING     0
-#define __THREAD_S_DORMANT     (1 << 8)
-#define __THREAD_S_WAIT                (1 << 9)
-#define __THREAD_S_TIMEDWAIT   (1 << 10)
-#define __THREAD_S_DELAYED     (1 << 11)
+#define __THREAD_S_DORMANT     (1 << 16)
+#define __THREAD_S_WAIT                (1 << 17)
+#define __THREAD_S_TIMEDWAIT   (1 << 18)
+#define __THREAD_S_DELAYED     (1 << 19)
 
 /* threadobj mode bits */
 #define __THREAD_M_LOCK                (1 << 0) /* Toggle scheduler lock. */
@@ -275,7 +276,7 @@ static inline void threadobj_free(struct threadobj *thobj)
 void threadobj_init(struct threadobj *thobj,
                    struct threadobj_init_data *idata);
 
-void threadobj_start(struct threadobj *thobj);
+int threadobj_start(struct threadobj *thobj);
 
 void threadobj_shadow(struct threadobj *thobj);
 
diff --git a/lib/alchemy/task.c b/lib/alchemy/task.c
index 00cf278..da09d51 100644
--- a/lib/alchemy/task.c
+++ b/lib/alchemy/task.c
@@ -544,7 +544,7 @@ int rt_task_start(RT_TASK *task,
 {
        struct alchemy_task *tcb;
        struct service svc;
-       int ret = 0;
+       int ret;
 
        COPPERPLATE_PROTECT(svc);
 
@@ -554,8 +554,15 @@ int rt_task_start(RT_TASK *task,
 
        tcb->entry = entry;
        tcb->arg = arg;
-       threadobj_start(&tcb->thobj);
-       put_alchemy_task(tcb);
+       ret = threadobj_start(&tcb->thobj);
+       if (ret == -EIDRM)
+               /*
+                * The started thread has run then exited, tcb->thobj
+                * is stale: don't touch it anymore.
+                */
+               ret = 0;
+       else
+               put_alchemy_task(tcb);
 out:
        COPPERPLATE_PROTECT(svc);
 
diff --git a/lib/copperplate/threadobj.c b/lib/copperplate/threadobj.c
index 4bbf06e..0ec95d7 100644
--- a/lib/copperplate/threadobj.c
+++ b/lib/copperplate/threadobj.c
@@ -797,6 +797,19 @@ void threadobj_init(struct threadobj *thobj,
        threadobj_init_corespec(thobj);
 }
 
+static void destroy_thread(struct threadobj *thobj)
+{
+       __RT(pthread_cond_destroy(&thobj->barrier));
+       __RT(pthread_mutex_destroy(&thobj->lock));
+       threadobj_cleanup_corespec(thobj);
+}
+
+void threadobj_destroy(struct threadobj *thobj) /* thobj->lock free */
+{
+       assert((thobj->status & (__THREAD_S_STARTED|__THREAD_S_ACTIVE)) == 0);
+       destroy_thread(thobj);
+}
+
 /*
  * NOTE: to spare us the need for passing the equivalent of a
  * syncstate argument to each thread locking operation, we hold the
@@ -815,7 +828,7 @@ void threadobj_init(struct threadobj *thobj,
  * call these services, and these have no threadobj descriptor.
  */
 
-static int start_sync(struct threadobj *thobj, int mask)
+static int wait_on_barrier(struct threadobj *thobj, int mask)
 {
        int oldstate, status;
 
@@ -833,27 +846,51 @@ static int start_sync(struct threadobj *thobj, int mask)
        return status;
 }
 
-void threadobj_start(struct threadobj *thobj)  /* thobj->lock held. */
+int threadobj_start(struct threadobj *thobj)   /* thobj->lock held. */
 {
        struct threadobj *current = threadobj_current();
+       int ret = 0, oldstate;
 
        __threadobj_check_locked(thobj);
 
        if (thobj->status & __THREAD_S_STARTED)
-               return;
+               return 0;
 
-       thobj->status |= __THREAD_S_STARTED;
+       thobj->status |= __THREAD_S_STARTED|__THREAD_S_SAFE;
        __RT(pthread_cond_signal(&thobj->barrier));
 
        if (current && thobj->priority <= current->priority)
-               return;
+               return 0;
        /*
         * Caller needs synchronization with the thread being started,
         * which has higher priority. We shall wait until that thread
         * enters the user code, or aborts prior to reaching that
         * point, whichever comes first.
+        *
+        * We must not exit until the synchronization has fully taken
+        * place, disable cancellability until then.
+        */
+       pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate);
+
+       wait_on_barrier(thobj, __THREAD_S_ACTIVE);
+       thobj->status &= ~__THREAD_S_SAFE;
+
+       /*
+        * If the started thread has exited before we woke up from the
+        * barrier, its TCB was not reclaimed, to prevent us from
+        * treading on stale memory. Reclaim it now, and tell the
+        * caller to forget about it as well.
         */
-       start_sync(thobj, __THREAD_S_ACTIVE);
+       if (thobj->run_state == __THREAD_S_DORMANT) {
+               threadobj_unlock(thobj);
+               destroy_thread(thobj);
+               threadobj_free(thobj);
+               ret = -EIDRM;
+       }
+
+       pthread_setcancelstate(oldstate, NULL);
+
+       return ret;
 }
 
 void threadobj_shadow(struct threadobj *thobj)
@@ -870,7 +907,7 @@ void threadobj_wait_start(void) /* current->lock free. */
        int status;
 
        threadobj_lock(current);
-       status = start_sync(current, __THREAD_S_STARTED|__THREAD_S_ABORTED);
+       status = wait_on_barrier(current, 
__THREAD_S_STARTED|__THREAD_S_ABORTED);
        threadobj_unlock(current);
 
        /*
@@ -950,6 +987,23 @@ int threadobj_prologue(struct threadobj *thobj, const char 
*name)
        return 0;
 }
 
+int threadobj_sleep(struct timespec *ts)
+{
+       struct threadobj *current = threadobj_current();
+       int ret;
+
+       /*
+        * clock_nanosleep() returns -EINTR upon threadobj_unblock()
+        * with both Cobalt and Mercury cores.
+        */
+       current->run_state = __THREAD_S_DELAYED;
+       threadobj_save_timeout(&current->core, ts);
+       ret = -__RT(clock_nanosleep(CLOCK_COPPERPLATE, TIMER_ABSTIME, ts, 
NULL));
+       current->run_state = __THREAD_S_RUNNING;
+
+       return ret;
+}
+
 static void cancel_sync(struct threadobj *thobj) /* thobj->lock held */
 {
        pthread_t tid = thobj->tid;
@@ -1019,23 +1073,6 @@ static void cancel_sync(struct threadobj *thobj) /* 
thobj->lock held */
        }
 }
 
-int threadobj_sleep(struct timespec *ts)
-{
-       struct threadobj *current = threadobj_current();
-       int ret;
-
-       /*
-        * clock_nanosleep() returns -EINTR upon threadobj_unblock()
-        * with both Cobalt and Mercury cores.
-        */
-       current->run_state = __THREAD_S_DELAYED;
-       threadobj_save_timeout(&current->core, ts);
-       ret = -__RT(clock_nanosleep(CLOCK_COPPERPLATE, TIMER_ABSTIME, ts, 
NULL));
-       current->run_state = __THREAD_S_RUNNING;
-
-       return ret;
-}
-
 /* thobj->lock held on entry, released on return */
 int threadobj_cancel(struct threadobj *thobj)
 {
@@ -1058,13 +1095,6 @@ int threadobj_cancel(struct threadobj *thobj)
        return 0;
 }
 
-static void destroy_thread(struct threadobj *thobj)
-{
-       __RT(pthread_cond_destroy(&thobj->barrier));
-       __RT(pthread_mutex_destroy(&thobj->lock));
-       threadobj_cleanup_corespec(thobj);
-}
-
 static void finalize_thread(void *p) /* thobj->lock free */
 {
        struct threadobj *thobj = p;
@@ -1094,15 +1124,22 @@ static void finalize_thread(void *p) /* thobj->lock 
free */
                /* Release the killer from threadobj_cancel(). */
                __STD(sem_post)(thobj->cancel_sem);
 
-       destroy_thread(thobj);
-       threadobj_free(thobj);
-       threadobj_set_current(NULL);
-}
+       thobj->run_state = __THREAD_S_DORMANT;
 
-void threadobj_destroy(struct threadobj *thobj) /* thobj->lock free */
-{
-       assert((thobj->status & (__THREAD_S_STARTED|__THREAD_S_ACTIVE)) == 0);
-       destroy_thread(thobj);
+       /*
+        * Do not reclaim the TCB core resources if another thread is
+        * waiting for us to start, pending on
+        * wait_on_barrier(). Instead, hand it over to this thread.
+        */
+       threadobj_lock(thobj);
+       if ((thobj->status & __THREAD_S_SAFE) == 0) {
+               threadobj_unlock(thobj);
+               destroy_thread(thobj);
+               threadobj_free(thobj);
+       } else
+               threadobj_unlock(thobj);
+
+       threadobj_set_current(NULL);
 }
 
 int threadobj_unblock(struct threadobj *thobj) /* thobj->lock held */
diff --git a/lib/psos/task.c b/lib/psos/task.c
index 27accef..0a0ef40 100644
--- a/lib/psos/task.c
+++ b/lib/psos/task.c
@@ -351,8 +351,9 @@ u_long t_start(u_long tid,
                task->args.arg3 = 0;
        }
        task->mode = mode;
-       threadobj_start(&task->thobj);
-       put_psos_task(task);
+       ret = threadobj_start(&task->thobj);
+       if (ret != -EIDRM)
+               put_psos_task(task);
 
        return SUCCESS;
 }
diff --git a/lib/vxworks/taskLib.c b/lib/vxworks/taskLib.c
index ffd9ce2..6096694 100644
--- a/lib/vxworks/taskLib.c
+++ b/lib/vxworks/taskLib.c
@@ -436,14 +436,16 @@ out:
 STATUS taskActivate(TASK_ID tid)
 {
        struct wind_task *task;
+       int ret;
 
        task = get_wind_task(tid);
        if (task == NULL)
                return ERROR;
 
        task->tcb->status &= ~WIND_SUSPEND;
-       threadobj_start(&task->thobj);
-       put_wind_task(task);
+       ret = threadobj_start(&task->thobj);
+       if (ret != -EIDRM)
+               put_wind_task(task);
 
        return OK;
 }


_______________________________________________
Xenomai-git mailing list
Xenomai-git@xenomai.org
http://www.xenomai.org/mailman/listinfo/xenomai-git

Reply via email to