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

Author: Philippe Gerum <r...@xenomai.org>
Date:   Sat Apr 26 15:07:41 2014 +0200

copperplate/notifier: sanitize and simplify - again

Those changes build on the fact that the notification support is there
exclusively for implementing the thread suspend/resume mechanism over
Mercury. Therefore we may drop any code which is not directly aimed at
supporting this feature.

Assuming this, locking may now be handled directly from the call
sites, all located in the threadobj implementation.

As a bonus, this change set also fixes the issue discussed there:
http://www.xenomai.org/pipermail/xenomai/2014-April/030804.html

---

 include/copperplate/notifier.h  |   16 ++--
 include/copperplate/threadobj.h |    1 -
 lib/copperplate/notifier.c      |  169 ++++++++++++---------------------------
 lib/copperplate/threadobj.c     |   69 +++++++---------
 4 files changed, 86 insertions(+), 169 deletions(-)

diff --git a/include/copperplate/notifier.h b/include/copperplate/notifier.h
index 536782d..13bf588 100644
--- a/include/copperplate/notifier.h
+++ b/include/copperplate/notifier.h
@@ -19,16 +19,12 @@
 #ifndef _COPPERPLATE_NOTIFIER_H
 #define _COPPERPLATE_NOTIFIER_H
 
-#include <pthread.h>
 #include <boilerplate/list.h>
 
 struct notifier {
-       pthread_mutex_t lock;
-       int notified;
        pid_t owner;
        int psfd[2];
        int pwfd[2];
-       void (*callback)(const struct notifier *nf);
        struct pvholder link;
 };
 
@@ -36,17 +32,17 @@ struct notifier {
 extern "C" {
 #endif
 
-int notifier_init(struct notifier *nf,
-                 void (*callback)(const struct notifier *nf),
-                 int owned);
+int notifier_init(struct notifier *nf, pid_t pid);
 
 void notifier_destroy(struct notifier *nf);
 
-int notifier_signal(struct notifier *nf);
+void notifier_signal(struct notifier *nf);
 
-int notifier_wait(const struct notifier *nf);
+void notifier_wait(const struct notifier *nf);
 
-int notifier_release(struct notifier *nf);
+void notifier_disable(struct notifier *nf);
+
+void notifier_release(struct notifier *nf);
 
 void notifier_pkg_init(void);
 
diff --git a/include/copperplate/threadobj.h b/include/copperplate/threadobj.h
index 5438790..bc5be99 100644
--- a/include/copperplate/threadobj.h
+++ b/include/copperplate/threadobj.h
@@ -128,7 +128,6 @@ void threadobj_save_timeout(struct threadobj_corespec 
*corespec,
 #define __THREAD_S_ACTIVE      (1 << 5)        /* Running user code. */
 #define __THREAD_S_SUSPENDED   (1 << 6)        /* Suspended via 
threadobj_suspend(). */
 #define __THREAD_S_SAFE                (1 << 7)        /* TCB release 
deferred. */
-#define __THREAD_S_ZOMBIE      (1 << 8)        /* Deletion process ongoing. */
 #define __THREAD_S_DEBUG       (1 << 31)       /* Debug mode enabled. */
 /*
  * threadobj->run_state, locklessly updated by "current", merged
diff --git a/lib/copperplate/notifier.c b/lib/copperplate/notifier.c
index 7fa48f3..4b3988a 100644
--- a/lib/copperplate/notifier.c
+++ b/lib/copperplate/notifier.c
@@ -50,25 +50,20 @@ static void notifier_sighandler(int sig, siginfo_t 
*siginfo, void *uc)
        pvlist_for_each_entry(nf, &notifier_list, link) {
                if (nf->psfd[0] != siginfo->si_fd)
                        continue;
+
+               matched = 1;
                /*
-                * Ignore misdirected notifications. We want those to
-                * hit the thread owning the notification object, but
-                * it may happen that the kernel picks another thread
-                * for receiving a subsequent signal while we are
-                * blocked in the callback code. In such a case, we
-                * just dismiss the notification, and expect the
-                * actual owner to detect the pending notification
-                * once the callback returns to the read() loop.
+                * Paranoid: misdirected notifications should never
+                * happen with F_SETOWN_EX invoked for the
+                * notification fildes.
                 */
-               matched = 1;
-               if (nf->owner && nf->owner != tid)
+               if (nf->owner != tid)
                        continue;
 
                for (;;) {
-                       ret = __STD(read(nf->psfd[0], &c, 1)); 
+                       ret = read(nf->psfd[0], &c, 1); 
                        if (ret > 0)
-                               /* Callee must run async-safe code only. */
-                               nf->callback(nf);
+                               notifier_wait(nf);
                        else if (ret == 0 || errno != EINTR)
                                break;
                }
@@ -77,9 +72,9 @@ static void notifier_sighandler(int sig, siginfo_t *siginfo, 
void *uc)
        }
 
        /*
-        * We may have received a valid notification on the wrong
-        * thread: bail out silently, assuming the recipient will find
-        * out eventually.
+        * Paranoid: we may have received a valid notification on the
+        * wrong thread: bail out silently, assuming the recipient
+        * will find out eventually.
         */
        if (matched)
                return;
@@ -119,11 +114,8 @@ static void unlock_notifier_list(sigset_t *oset)
        write_unlock(&notifier_lock);
 }
 
-int notifier_init(struct notifier *nf,
-                 void (*callback)(const struct notifier *nf),
-                 int owned)
+int notifier_init(struct notifier *nf, pid_t pid)
 {
-       pthread_mutexattr_t mattr;
        struct f_owner_ex owner;
        sigset_t oset;
        int fd, ret;
@@ -135,20 +127,12 @@ int notifier_init(struct notifier *nf,
 
        if (pipe(nf->pwfd) < 0) {
                ret = -errno;
-               __STD(close(nf->psfd[0]));
-               __STD(close(nf->psfd[1]));
+               close(nf->psfd[0]);
+               close(nf->psfd[1]);
                goto fail;
        }
 
-       nf->callback = callback;
-       nf->notified = 0;
-       nf->owner = owned ? copperplate_get_tid() : 0;
-       __STD(pthread_mutexattr_init(&mattr));
-       __RT(pthread_mutexattr_settype(&mattr, mutex_type_attribute));
-       __STD(pthread_mutexattr_setprotocol(&mattr, PTHREAD_PRIO_INHERIT));
-       __STD(pthread_mutexattr_setpshared(&mattr, PTHREAD_PROCESS_PRIVATE));
-       __STD(pthread_mutex_init(&nf->lock, &mattr));
-       __STD(pthread_mutexattr_destroy(&mattr));
+       nf->owner = pid;
 
        push_cleanup_lock(&notifier_lock);
        lock_notifier_list(&oset);
@@ -189,89 +173,45 @@ void notifier_destroy(struct notifier *nf)
        pvlist_remove(&nf->link);
        unlock_notifier_list(&oset);
        pop_cleanup_lock(&notifier_lock);
-       __STD(close(nf->psfd[0]));
-       __STD(close(nf->psfd[1]));
-       __STD(close(nf->pwfd[0]));
-       __STD(close(nf->pwfd[1]));
-       __STD(pthread_mutex_destroy(&nf->lock));
+       close(nf->psfd[0]);
+       close(nf->psfd[1]);
+       close(nf->pwfd[0]); /* May fail if disabled. */
+       close(nf->pwfd[1]);
 }
 
-int notifier_signal(struct notifier *nf)
+void notifier_signal(struct notifier *nf)
 {
-       int fd, ret, kick = 1;
        char c = 0;
+       int ret;
 
-       ret = write_lock_nocancel(&nf->lock);
-       if (ret)
-               return __bt(ret);
-
-       fd = nf->psfd[1];
-
-       if (!nf->notified)
-               nf->notified = 1;
-       else
-               kick = 0;
-
-       write_unlock(&nf->lock);
-
-       /*
-        * XXX: we must release the lock before we write to the pipe,
-        * since we may be immediately preempted by the notification
-        * signal in case we notify the current thread.
-        */
-       if (kick) {
-               do
-                       ret = __STD(write(fd, &c, 1));
-               while (ret == -1 && errno == EINTR);
-       }
+       do
+               ret = write(nf->psfd[1], &c, 1);
+       while (ret == -1 && errno == EINTR);
+}
 
-       return 0;
+void notifier_disable(struct notifier *nf)
+{
+       close(nf->pwfd[0]);
 }
 
-int notifier_release(struct notifier *nf)
+void notifier_release(struct notifier *nf)
 {
-       int fd, ret, kick = 1;
        char c = 1;
+       int ret;
 
-       /*
-        * The notifier struct is associated to the caller thread, so
-        * if the caller is cancelled, the notification struct becomes
-        * useless. We may only take a read lock here.
-        */
-       ret = write_lock_nocancel(&nf->lock);
-       if (ret)
-               return __bt(ret);
-
-       fd = nf->pwfd[1];
-
-       if (nf->notified)
-               nf->notified = 0;
-       else
-               kick = 0;
-
-       write_unlock(&nf->lock);
-
-       if (kick) {
-               do
-                       ret = __STD(write(fd, &c, 1)); 
-               while (ret == -1 && errno == EINTR);
-       }
-
-       return 0;
+       do
+               ret = write(nf->pwfd[1], &c, 1);
+       while (ret == -1 && errno == EINTR);
 }
 
-int notifier_wait(const struct notifier *nf) /* sighandler context */
+void notifier_wait(const struct notifier *nf) /* sighandler context */
 {
        int ret;
        char c;
 
        do
-               ret = __STD(read(nf->pwfd[0], &c, 1)); 
+               ret = read(nf->pwfd[0], &c, 1);
        while (ret == -1 && errno == EINTR);
-
-       assert(ret == 1); (void)ret;
-
-       return 0;
 }
 
 void notifier_pkg_init(void)
@@ -279,35 +219,24 @@ void notifier_pkg_init(void)
        pthread_mutexattr_t mattr;
        struct sigaction sa;
 
-       __STD(pthread_mutexattr_init(&mattr));
-       __RT(pthread_mutexattr_settype(&mattr, mutex_type_attribute));
-       __STD(pthread_mutexattr_setprotocol(&mattr, PTHREAD_PRIO_INHERIT));
-       __STD(pthread_mutexattr_setpshared(&mattr, PTHREAD_PROCESS_PRIVATE));
-       __STD(pthread_mutex_init(&notifier_lock, &mattr));
-       __STD(pthread_mutexattr_destroy(&mattr));
+       pthread_mutexattr_init(&mattr);
+       pthread_mutexattr_settype(&mattr, mutex_type_attribute);
+       pthread_mutexattr_setprotocol(&mattr, PTHREAD_PRIO_INHERIT);
+       pthread_mutexattr_setpshared(&mattr, PTHREAD_PROCESS_PRIVATE);
+       pthread_mutex_init(&notifier_lock, &mattr);
+       pthread_mutexattr_destroy(&mattr);
        /*
-        * XXX: We have four requirements here:
+        * We have two basic requirements for the notification
+        * scheme implementing the suspend/resume mechanism:
         *
         * - we have to rely on Linux signals for notification, which
-        * guarantees that the target thread will get the message as
-        * soon as possible, regardless of what it was doing when
-        * notified (syscall wait, pure runtime etc.).
-        *
-        * - we must process the notifier callback fully on behalf of
-        * the target thread, since client code may rely on this
-        * assumption.  E.g. offloading the callback code to some
-        * server thread kicked from the signal handler would be a bad
-        * idea in that sense.
-        *
-        * - a notifier callback should be allowed to block using a
-        * dedicated service, until a release notification is
-        * sent. Since the callback runs over a signal handler, we
-        * have to implement this feature only using async-safe
-        * services.
+        * guarantees that the target thread will receive the suspend
+        * request asap, regardless of what it was doing when notified
+        * (syscall wait, pure runtime etc.).
         *
-        * - we must allow a single thread to listen to multiple
-        * notification events, and the destination of each event
-        * should be easily identified.
+        * - we must process the suspension signal on behalf of the
+        * target thread, as we want that thread to block upon
+        * receipt.
         */
        memset(&sa, 0, sizeof(sa));
        sa.sa_sigaction = &notifier_sighandler;
diff --git a/lib/copperplate/threadobj.c b/lib/copperplate/threadobj.c
index f4eb8cc..e885cff 100644
--- a/lib/copperplate/threadobj.c
+++ b/lib/copperplate/threadobj.c
@@ -450,29 +450,6 @@ static inline void pkg_init_corespec(void)
        notifier_pkg_init();
 }
 
-static void notifier_callback(const struct notifier *nf)
-{
-       struct threadobj *current;
-
-       current = container_of(nf, struct threadobj, core.notifier);
-       assert(current == threadobj_current());
-
-       /*
-        * In the Mercury case, we mark the thread as suspended only
-        * when the notifier handler is entered, not from
-        * threadobj_suspend().
-        */
-       threadobj_lock(current);
-       if ((current->status & __THREAD_S_ZOMBIE) == 0) {
-               current->status |= __THREAD_S_SUSPENDED;
-               threadobj_unlock(current);
-               notifier_wait(nf); /* Wait for threadobj_resume(). */
-               threadobj_lock(current);
-               current->status &= ~__THREAD_S_SUSPENDED;
-       }
-       threadobj_unlock(current);
-}
-
 static inline void threadobj_init_corespec(struct threadobj *thobj)
 {
        pthread_condattr_t cattr;
@@ -499,7 +476,7 @@ static inline int threadobj_setup_corespec(struct threadobj 
*thobj)
        int ret;
 
        prctl(PR_SET_NAME, (unsigned long)thobj->name, 0, 0, 0);
-       ret = notifier_init(&thobj->core.notifier, notifier_callback, 1);
+       ret = notifier_init(&thobj->core.notifier, threadobj_get_pid(thobj));
        if (ret)
                return __bt(ret);
 
@@ -546,28 +523,45 @@ static inline void threadobj_run_corespec(struct 
threadobj *thobj)
 
 static inline void threadobj_cancel_corespec(struct threadobj *thobj) /* 
thobj->lock held */
 {
+       struct notifier *nf = &thobj->core.notifier;
+
+       /*
+        * Any ongoing or future notify_wait() will return immediately
+        * on error with EBADF.
+        */
+       notifier_disable(nf);
 }
 
 int threadobj_suspend(struct threadobj *thobj) /* thobj->lock held */
 {
        struct notifier *nf = &thobj->core.notifier;
-       int ret;
 
-       threadobj_unlock(thobj); /* FIXME: racy */
-       ret = notifier_signal(nf);
-       threadobj_lock(thobj);
+       __threadobj_check_locked(thobj);
 
-       return __bt(ret);
+       if (thobj == threadobj_current()) {
+               thobj->status |= __THREAD_S_SUSPENDED;
+               threadobj_unlock(thobj);
+               notifier_wait(nf);
+               threadobj_lock(thobj);
+       } else if ((thobj->status & __THREAD_S_SUSPENDED) == 0) {
+               thobj->status |= __THREAD_S_SUSPENDED;
+               notifier_signal(nf);
+       }
+
+       return 0;
 }
 
 int threadobj_resume(struct threadobj *thobj) /* thobj->lock held */
 {
        __threadobj_check_locked(thobj);
 
-       if (thobj == threadobj_current())
-               return 0;
+       if (thobj != threadobj_current() &&
+           (thobj->status & __THREAD_S_SUSPENDED) != 0) {
+               thobj->status &= ~__THREAD_S_SUSPENDED;
+               notifier_release(&thobj->core.notifier);
+       }
 
-       return __bt(notifier_release(&thobj->core.notifier));
+       return 0;
 }
 
 int threadobj_sleep(struct timespec *ts)
@@ -1201,11 +1195,11 @@ static void cancel_sync(struct threadobj *thobj) /* 
thobj->lock held */
 
        /*
         * We have to allocate the cancel sync sema4 in the main heap
-        * dynamically, so that it always live in valid memory when we
-        * wait on it and the cancelled thread posts it. This has to
-        * be true regardless of whether --enable-pshared is in
-        * effect, or thobj becomes stale after the finalizer has run
-        * (we cannot host this sema4 in thobj for this reason).
+        * dynamically, so that it always lives in valid memory when
+        * we wait on it. This has to be true regardless of whether
+        * --enable-pshared is in effect, or thobj becomes stale after
+        * the finalizer has run (we cannot host this sema4 in thobj
+        * for this reason).
         */
        sem = xnmalloc(sizeof(*sem));
        if (sem == NULL)
@@ -1214,7 +1208,6 @@ static void cancel_sync(struct threadobj *thobj) /* 
thobj->lock held */
                __STD(sem_init(sem, sem_scope_attribute, 0));
 
        thobj->cancel_sem = sem;
-       thobj->status |= __THREAD_S_ZOMBIE;
 
        /*
         * If the thread to delete is warming up, wait until it


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

Reply via email to