Skip to contentSkip to conversation

Clean up XNWAKEN / wwake tracking

PATCH In review Jan Kiszka 2 participants 4 messages Aug 19, 2008 – Aug 20, 2008

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 Clean up XNWAKEN / wwake tracking

Current lock stealing implementation encodes the "new owner woken up, but not yet scheduled" state redundantly. First, the XNWAKEN thread bit is set and, second, the wwake field in xnthread_t points to the xnsynch_t object that the thread is about to own.

Apparently, no technical benefit can be expected from this. It rather adds some, though minor, overhead to frequently used core services. Moreover such redundancy can cause confusion for the uninformed reader who tries to understand the implementation.

This patch encodes the state purely via wwake (XNWAKEN set => wwake != NULL). Compared to the first version posted, this one also clears the state again in case xnsynch_sleep_on is redone after some lock robbery (compensating the removed XNWAKEN clearance in xnpod_suspend_thread).

Jan

---
 include/nucleus/thread.h |    1 -
 ksrc/nucleus/pod.c       |    2 +-
 ksrc/nucleus/synch.c     |   11 ++++-------
 3 files changed, 5 insertions(+), 9 deletions(-)
include/nucleus/thread.h +0 -1
Index: b/include/nucleus/thread.h
===================================================================
@@ -112,7 +112,6 @@
 #define XNRMID    0x00000002 /**< Pending on a removed resource */
 #define XNBREAK   0x00000004 /**< Forcibly awaken from a wait state */
 #define XNKICKED  0x00000008 /**< Kicked upon Linux signal (shadow only) */
-#define XNWAKEN   0x00000010 /**< Thread waken up upon resource availability */
 #define XNROBBED  0x00000020 /**< Robbed from resource ownership */
 #define XNATOMIC  0x00000040 /**< In atomic switch from secondary to primary mode */
 #define XNAFFSET  0x00000080 /**< CPU affinity changed from primary mode */
ksrc/nucleus/pod.c +1 -1
Index: b/ksrc/nucleus/pod.c
===================================================================
@@ -1406,7 +1406,7 @@ void xnpod_suspend_thread(xnthread_t *th
 		}
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
 
-		xnthread_clear_info(thread, XNRMID | XNTIMEO | XNBREAK | XNWAKEN | XNROBBED);
+		xnthread_clear_info(thread, XNRMID | XNTIMEO | XNBREAK | XNROBBED);
 	}
 
 	/* Don't start the timer for a thread indefinitely delayed by
ksrc/nucleus/synch.c +4 -7
Index: b/ksrc/nucleus/synch.c
===================================================================
@@ -199,7 +199,7 @@ redo:
 	}
 
 	if (thread->cprio > owner->cprio) {
-		if (xnthread_test_info(owner, XNWAKEN) && owner->wwake == synch) {
+		if (owner->wwake == synch) {
 			/* Ownership is still pending, steal the resource. */
 			synch->owner = thread;
 			xnthread_clear_info(thread, XNRMID | XNTIMEO | XNBREAK);

@@ -225,6 +225,9 @@ redo:
 
 	xnpod_suspend_thread(thread, XNPEND, timeout, timeout_mode, synch);
 
+	/* We are awake, no one must steal our lock anymore. */
+	thread->wwake = NULL;
+
 	if (xnthread_test_info(thread, XNRMID | XNTIMEO | XNBREAK))
 		goto unlock_and_exit;
 

@@ -241,10 +244,6 @@ redo:
 	}
 
       unlock_and_exit:
-
-	thread->wwake = NULL;
-	xnthread_clear_info(thread, XNWAKEN);
-
 	xnlock_put_irqrestore(&nklock, s);
 }
 

@@ -389,7 +388,6 @@ xnthread_t *xnsynch_wakeup_one_sleeper(x
 		thread->wchan = NULL;
 		thread->wwake = synch;
 		synch->owner = thread;
-		xnthread_set_info(thread, XNWAKEN);
 		trace_mark(xn_nucleus_synch_wakeup_one,
 			   "thread %p thread_name %s synch %p",
 			   thread, xnthread_name(thread), synch);

@@ -503,7 +501,6 @@ xnpholder_t *xnsynch_wakeup_this_sleeper
 	thread->wchan = NULL;
 	thread->wwake = synch;
 	synch->owner = thread;
-	xnthread_set_info(thread, XNWAKEN);
 	trace_mark(xn_nucleus_synch_wakeup_all,
 		   "thread %p thread_name %s synch %p",
 		   thread, xnthread_name(thread), synch);
#1 lore raw

ksrc/nucleus/synch.c redo: · line 230

Quoted 3 lines · Jan Kiszka
ksrc/nucleus/synch.c +3 -0
+	/* We are awake, no one must steal our lock anymore. */
+	thread->wwake = NULL;
+

This is wrong, whether or not no one must steal our lock anymore will be decided at the "redo" label, when we "test and set", the synch owner.

Signature · 1 line
                                                 Gilles.
#2 lore raw
Quoted 8 lines · Gilles Chanteperdrix

ksrc/nucleus/synch.c redo: · line 230

Quoted 3 lines · Jan Kiszka
ksrc/nucleus/synch.c +3 -0
+	/* We are awake, no one must steal our lock anymore. */
+	thread->wwake = NULL;
+

This is wrong, whether or not no one must steal our lock anymore will be decided at the "redo" label, when we "test and set", the synch owner.

Ok. This was not the latest patch, sorry for the noise.

Signature · 1 line
                                                 Gilles.
author
#3 lore raw
In reply to Gilles Chanteperdrix
Quoted 8 lines · Gilles Chanteperdrix

ksrc/nucleus/synch.c redo: · line 230

Quoted 3 lines · Jan Kiszka
ksrc/nucleus/synch.c +3 -0
+	/* We are awake, no one must steal our lock anymore. */
+	thread->wwake = NULL;
+

This is wrong, whether or not no one must steal our lock anymore will be decided at the "redo" label, when we "test and set", the synch owner.

Maybe I should have added "(in case we actually got it)". If not, that assignment is a nop. If we redo the whole thing, we would have cleared XNWAKEN in xnpod_suspend_thread before suspension, so the effect should be the same.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux