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

Author: Philippe Gerum <r...@xenomai.org>
Date:   Fri Nov 14 19:10:02 2014 +0100

cobalt/rtdm/mutex: fix rtdm_mutex_timedlock()

The fast path in rtdm_mutex_timedlock() was terminally broken by the
not-so-recent addition of fast locks to RTDM-based kernel mutexes. As
a matter of fact, the xnthread_try_grab() helper suffered multiple
breakages, including the lack of update to the underlying fastlock
word to reflect a successful locking operation.

To this end, we introduce xnsynch_try_acquire() which properly
attempts to grab the requested mutex on behalf of the current thread,
and call it as a drop-in replacement for xnthread_try_grab().  Callers
fall back to xnsynch_acquire() on failure to put a fast grab on the
mutex.

---

 include/cobalt/kernel/synch.h     |    2 +
 include/cobalt/kernel/thread.h    |   16 -----
 kernel/cobalt/rtdm/drvlib.c       |   68 ++++++++----------
 kernel/cobalt/synch.c             |  141 ++++++++++++++++++++++++++-----------
 kernel/cobalt/trace/cobalt-core.h |    5 ++
 5 files changed, 136 insertions(+), 96 deletions(-)

diff --git a/include/cobalt/kernel/synch.h b/include/cobalt/kernel/synch.h
index 98d7a43..4333521 100644
--- a/include/cobalt/kernel/synch.h
+++ b/include/cobalt/kernel/synch.h
@@ -153,6 +153,8 @@ int xnsynch_acquire(struct xnsynch *synch,
                    xnticks_t timeout,
                    xntmode_t timeout_mode);
 
+int xnsynch_try_acquire(struct xnsynch *synch);
+
 struct xnthread *xnsynch_release(struct xnsynch *synch,
                                 struct xnthread *thread);
 
diff --git a/include/cobalt/kernel/thread.h b/include/cobalt/kernel/thread.h
index 68a8ca9..72168f4 100644
--- a/include/cobalt/kernel/thread.h
+++ b/include/cobalt/kernel/thread.h
@@ -317,22 +317,6 @@ void xnthread_set_sync_window(struct xnthread *thread, int 
state_bits)
        }
 }
 
-static inline int xnthread_try_grab(struct xnthread *thread,
-                                   struct xnsynch *synch)
-{
-       XENO_BUGON(COBALT, xnsynch_fastlock_p(synch));
-
-       if (xnsynch_owner(synch) != NULL)
-               return 0;
-
-       xnsynch_set_owner(synch, thread);
-
-       if (xnthread_test_state(thread, XNWEAK))
-               thread->res_count++;
-
-       return 1;
-}
-
 static inline int normalize_priority(int prio)
 {
        return prio < MAX_RT_PRIO ? prio : MAX_RT_PRIO - 1;
diff --git a/kernel/cobalt/rtdm/drvlib.c b/kernel/cobalt/rtdm/drvlib.c
index e9ef6ce..414e262 100644
--- a/kernel/cobalt/rtdm/drvlib.c
+++ b/kernel/cobalt/rtdm/drvlib.c
@@ -1248,58 +1248,50 @@ EXPORT_SYMBOL_GPL(rtdm_mutex_lock);
 int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout,
                         rtdm_toseq_t *timeout_seq)
 {
-       struct xnthread *curr_thread;
-       int err = 0;
+       struct xnthread *curr;
+       int ret;
        spl_t s;
 
        if (!XENO_ASSERT(COBALT, !xnsched_unblockable_p()))
                return -EPERM;
 
-       curr_thread = xnthread_current();
-       trace_cobalt_driver_mutex_wait(mutex, curr_thread);
+       curr = xnthread_current();
+       trace_cobalt_driver_mutex_wait(mutex, curr);
 
        xnlock_get_irqsave(&nklock, s);
 
-       if (unlikely(mutex->synch_base.status & RTDM_SYNCH_DELETED))
-               err = -EIDRM;
-       else if (!xnthread_try_grab(curr_thread, &mutex->synch_base)) {
-               /* Redefinition to clarify XENO_ASSERT output */
-               #define mutex_owner xnsynch_owner(&mutex->synch_base)
-               if (!XENO_ASSERT(COBALT, mutex_owner != curr_thread)) {
-                       err = -EDEADLK;
-                       goto unlock_out;
-               }
-
-               /* non-blocking mode */
-               if (timeout < 0) {
-                       err = -EWOULDBLOCK;
-                       goto unlock_out;
-               }
+       if (unlikely(mutex->synch_base.status & RTDM_SYNCH_DELETED)) {
+               ret = -EIDRM;
+               goto out;
+       }
 
-restart:
-               if (timeout_seq && (timeout > 0)) {
-                       /* timeout sequence */
-                       xnsynch_acquire(&mutex->synch_base, *timeout_seq,
-                                       XN_ABSOLUTE);
-               } else
-                       /* infinite or relative timeout */
-                       xnsynch_acquire(&mutex->synch_base, timeout, 
XN_RELATIVE);
+       ret = xnsynch_try_acquire(&mutex->synch_base);
+       if (ret != -EBUSY)
+               goto out;
 
-               if (unlikely(xnthread_test_info(curr_thread,
-                                               XNTIMEO | XNRMID | XNBREAK))) {
-                       if (xnthread_test_info(curr_thread, XNTIMEO))
-                               err = -ETIMEDOUT;
-                       else if (xnthread_test_info(curr_thread, XNRMID))
-                               err = -EIDRM;
-                       else /*  XNBREAK */
-                               goto restart;
-               }
+       if (timeout < 0) {
+               ret = -EWOULDBLOCK;
+               goto out;
        }
 
-unlock_out:
+       for (;;) {
+               if (timeout_seq && timeout > 0) /* timeout sequence */
+                       ret = xnsynch_acquire(&mutex->synch_base, *timeout_seq,
+                                             XN_ABSOLUTE);
+               else            /* infinite or relative timeout */
+                       ret = xnsynch_acquire(&mutex->synch_base, timeout,
+                                             XN_RELATIVE);
+               if (ret == 0)
+                       break;
+               if (ret & XNBREAK)
+                       continue;
+               ret = ret & XNTIMEO ? -ETIMEDOUT : -EIDRM;
+               break;
+       }
+out:
        xnlock_put_irqrestore(&nklock, s);
 
-       return err;
+       return ret;
 }
 
 EXPORT_SYMBOL_GPL(rtdm_mutex_timedlock);
diff --git a/kernel/cobalt/synch.c b/kernel/cobalt/synch.c
index e6de7ca..410ed73 100644
--- a/kernel/cobalt/synch.c
+++ b/kernel/cobalt/synch.c
@@ -300,6 +300,59 @@ static void xnsynch_renice_thread(struct xnthread *thread,
 }
 
 /**
+ * @fn int xnsynch_try_acquire(struct xnsynch *synch);
+ * @brief Try acquiring the ownership of a synchronization object.
+ *
+ * This service should be called by upper interfaces wanting the
+ * current thread to acquire the ownership of the given resource. If
+ * the resource is already assigned to another thread, the call
+ * returns with an error code.
+ *
+ * This service must be used only with synchronization objects that
+ * track ownership (XNSYNCH_OWNER set.
+ *
+ * @param synch The descriptor address of the synchronization object
+ * to acquire.
+ *
+ * @return Zero is returned if @a synch has been successfully
+ * acquired. Otherwise:
+ *
+ * - -EDEADLK is returned if @a synch is currently held by the calling
+ * thread.
+ *
+ * - -EBUSY is returned if @a synch is currently held by another
+ * thread.
+ *
+ * @coretags{primary-only}
+ */
+int xnsynch_try_acquire(struct xnsynch *synch)
+{
+       struct xnthread *curr;
+       atomic_t *lockp;
+       xnhandle_t h;
+
+       primary_mode_only();
+
+       XENO_BUGON(COBALT, (synch->status & XNSYNCH_OWNER) == 0);
+
+       curr = xnthread_current();
+       lockp = xnsynch_fastlock(synch);
+       trace_cobalt_synch_try_acquire(synch, curr);
+
+       h = atomic_cmpxchg(lockp, XN_NO_HANDLE, curr->handle);
+       if (h != XN_NO_HANDLE)
+               return xnhandle_get_id(h) == curr->handle ?
+                       -EDEADLK : -EBUSY;
+
+       xnsynch_set_owner(synch, curr);
+       if (xnthread_test_state(curr, XNWEAK))
+               curr->res_count++;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(xnsynch_try_acquire);
+
+/**
  * @fn int xnsynch_acquire(struct xnsynch *synch, xnticks_t timeout, xntmode_t 
timeout_mode);
  * @brief Acquire the ownership of a synchronization object.
  *
@@ -332,12 +385,16 @@ static void xnsynch_renice_thread(struct xnthread *thread,
  * signal/unblock conditions which might have happened while waiting.
  *
  * @coretags{primary-only, might-switch}
+ *
+ * @note Unlike xnsynch_try_acquire(), this call does NOT check for
+ * invalid recursive locking request, which means that such request
+ * will always cause a deadlock for the caller.
  */
 int xnsynch_acquire(struct xnsynch *synch, xnticks_t timeout,
                    xntmode_t timeout_mode)
 {
-       xnhandle_t threadh, fastlock, old;
-       struct xnthread *thread, *owner;
+       struct xnthread *curr, *owner;
+       xnhandle_t currh, h, oldh;
        atomic_t *lockp;
        spl_t s;
 
@@ -345,17 +402,18 @@ int xnsynch_acquire(struct xnsynch *synch, xnticks_t 
timeout,
 
        XENO_BUGON(COBALT, (synch->status & XNSYNCH_OWNER) == 0);
 
-       thread = xnthread_current();
-       threadh = thread->handle;
+       curr = xnthread_current();
+       currh = curr->handle;
        lockp = xnsynch_fastlock(synch);
-       trace_cobalt_synch_acquire(synch, thread);
+       trace_cobalt_synch_acquire(synch, curr);
 redo:
-       fastlock = atomic_cmpxchg(lockp, XN_NO_HANDLE, threadh);
-
-       if (likely(fastlock == XN_NO_HANDLE)) {
-               if (xnthread_test_state(thread, XNWEAK))
-                       thread->res_count++;
-               xnthread_clear_info(thread, XNRMID | XNTIMEO | XNBREAK);
+       /* Basic form of xnsynch_try_acquire(). */
+       h = atomic_cmpxchg(lockp, XN_NO_HANDLE, currh);
+       if (likely(h == XN_NO_HANDLE)) {
+               xnsynch_set_owner(synch, curr);
+               if (xnthread_test_state(curr, XNWEAK))
+                       curr->res_count++;
+               xnthread_clear_info(curr, XNRMID | XNTIMEO | XNBREAK);
                return 0;
        }
 
@@ -368,53 +426,52 @@ redo:
         * avoid cmpxchg where possible. Only if it appears not to be
         * set, start with cmpxchg directly.
         */
-       if (xnsynch_fast_is_claimed(fastlock)) {
-               old = atomic_read(lockp);
+       if (xnsynch_fast_is_claimed(h)) {
+               oldh = atomic_read(lockp);
                goto test_no_owner;
        }
 
        do {
-               old = atomic_cmpxchg(lockp, fastlock,
-                                    xnsynch_fast_claimed(fastlock));
-               if (likely(old == fastlock))
+               oldh = atomic_cmpxchg(lockp, h, xnsynch_fast_claimed(h));
+               if (likely(oldh == h))
                        break;
        test_no_owner:
-               if (old == XN_NO_HANDLE) {
+               if (oldh == XN_NO_HANDLE) {
                        /* Mutex released from another cpu. */
                        xnlock_put_irqrestore(&nklock, s);
                        goto redo;
                }
-               fastlock = old;
-       } while (!xnsynch_fast_is_claimed(fastlock));
+               h = oldh;
+       } while (!xnsynch_fast_is_claimed(h));
 
-       owner = xnthread_lookup(fastlock);
+       owner = xnthread_lookup(h);
        if (owner == NULL) {
                /*
                 * The handle is broken, therefore pretend that the
                 * synch object was deleted to signal an error.
                 */
-               xnthread_set_info(thread, XNRMID);
+               xnthread_set_info(curr, XNRMID);
                goto out;
        }
 
        xnsynch_set_owner(synch, owner);
-       xnsynch_detect_relaxed_owner(synch, thread);
+       xnsynch_detect_relaxed_owner(synch, curr);
 
        if ((synch->status & XNSYNCH_PRIO) == 0) { /* i.e. FIFO */
-               list_add_tail(&thread->plink, &synch->pendq);
+               list_add_tail(&curr->plink, &synch->pendq);
                goto block;
        }
 
-       if (thread->wprio > owner->wprio) {
+       if (curr->wprio > owner->wprio) {
                if (xnthread_test_info(owner, XNWAKEN) && owner->wwake == 
synch) {
                        /* Ownership is still pending, steal the resource. */
-                       synch->owner = thread;
-                       xnthread_clear_info(thread, XNRMID | XNTIMEO | XNBREAK);
+                       synch->owner = curr;
+                       xnthread_clear_info(curr, XNRMID | XNTIMEO | XNBREAK);
                        xnthread_set_info(owner, XNROBBED);
                        goto grab;
                }
 
-               list_add_priff(thread, &synch->pendq, wprio, plink);
+               list_add_priff(curr, &synch->pendq, wprio, plink);
 
                if (synch->status & XNSYNCH_PIP) {
                        if (!xnthread_test_state(owner, XNBOOST)) {
@@ -427,21 +484,21 @@ redo:
                        else
                                synch->status |= XNSYNCH_CLAIMED;
 
-                       synch->wprio = thread->wprio;
+                       synch->wprio = curr->wprio;
                        list_add_priff(synch, &owner->claimq, wprio, link);
-                       xnsynch_renice_thread(owner, thread);
+                       xnsynch_renice_thread(owner, curr);
                }
        } else
-               list_add_priff(thread, &synch->pendq, wprio, plink);
+               list_add_priff(curr, &synch->pendq, wprio, plink);
 block:
-       xnthread_suspend(thread, XNPEND, timeout, timeout_mode, synch);
-       thread->wwake = NULL;
-       xnthread_clear_info(thread, XNWAKEN);
+       xnthread_suspend(curr, XNPEND, timeout, timeout_mode, synch);
+       curr->wwake = NULL;
+       xnthread_clear_info(curr, XNWAKEN);
 
-       if (xnthread_test_info(thread, XNRMID | XNTIMEO | XNBREAK))
+       if (xnthread_test_info(curr, XNRMID | XNTIMEO | XNBREAK))
                goto out;
 
-       if (xnthread_test_info(thread, XNROBBED)) {
+       if (xnthread_test_info(curr, XNROBBED)) {
                /*
                 * Somebody stole us the ownership while we were ready
                 * to run, waiting for the CPU: we need to wait again
@@ -451,27 +508,27 @@ block:
                        xnlock_put_irqrestore(&nklock, s);
                        goto redo;
                }
-               timeout = xntimer_get_timeout_stopped(&thread->rtimer);
+               timeout = xntimer_get_timeout_stopped(&curr->rtimer);
                if (timeout > 1) { /* Otherwise, it's too late. */
                        xnlock_put_irqrestore(&nklock, s);
                        goto redo;
                }
-               xnthread_set_info(thread, XNTIMEO);
+               xnthread_set_info(curr, XNTIMEO);
                goto out;
        }
  grab:
-       if (xnthread_test_state(thread, XNWEAK))
-               thread->res_count++;
+       if (xnthread_test_state(curr, XNWEAK))
+               curr->res_count++;
 
        if (xnsynch_pended_p(synch))
-               threadh = xnsynch_fast_claimed(threadh);
+               currh = xnsynch_fast_claimed(currh);
 
        /* Set new ownership for this mutex. */
-       atomic_set(lockp, threadh);
+       atomic_set(lockp, currh);
 out:
        xnlock_put_irqrestore(&nklock, s);
 
-       return (int)xnthread_test_info(thread, XNRMID|XNTIMEO|XNBREAK);
+       return (int)xnthread_test_info(curr, XNRMID|XNTIMEO|XNBREAK);
 }
 EXPORT_SYMBOL_GPL(xnsynch_acquire);
 
diff --git a/kernel/cobalt/trace/cobalt-core.h 
b/kernel/cobalt/trace/cobalt-core.h
index 34c93c3..b0035e3 100644
--- a/kernel/cobalt/trace/cobalt-core.h
+++ b/kernel/cobalt/trace/cobalt-core.h
@@ -555,6 +555,11 @@ DEFINE_EVENT(synch_wait_event, cobalt_synch_sleepon,
        TP_ARGS(synch, thread)
 );
 
+DEFINE_EVENT(synch_wait_event, cobalt_synch_try_acquire,
+       TP_PROTO(struct xnsynch *synch, struct xnthread *thread),
+       TP_ARGS(synch, thread)
+);
+
 DEFINE_EVENT(synch_wait_event, cobalt_synch_acquire,
        TP_PROTO(struct xnsynch *synch, struct xnthread *thread),
        TP_ARGS(synch, thread)


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

Reply via email to