Update.

2000-06-13  Kaz Kylheku  <kaz@ashi.footprints.net>

	A few optimizations.  Got rid of unnecessary wakeups of timer threads,
	tightened up some critical regions and micro-optimized some list
	manipulation code.

	* sysdeps/pthread/timer_routines.c (__timer_thread_queue_timer):
	Returns int value now to indicate whether timer was queued at head.
	* sysdeps/pthread/posix-timer.h: Likewise.
	* sysdeps/pthread/timer_settime.c (timer_settime): Takes advantage of
	new return value from __timer_thread_queue_timer to avoid waking
	up timer thread unnecessarily.

	* sysdeps/pthread/posix-timer.h (timer_id2ptr): No longer checks
	inuse flag, because this requires mutex to be held.  Callers updated
	to do the check when they have the mutex.
	* sysdeps/pthread/timer_getoverr.c: Add check for inuse here.

	* sysdeps/pthread/timer_settime.c (timer_settime): Tighter critical
	regions: avoids making system calls while holding timer mutex, and
	a few computations were moved outside of the mutex as well.
	* sysdeps/pthread/timer_gettime.c (timer_gettime): Likewise.

	* sysdeps/pthread/posix-timer.h (list_unlink_ip): Function name changed
	to list_unlink_ip, meaning idempotent.  Pointer manipulation
	changed to get better better code out of gcc.
	* sysdeps/pthread/timer_routines.c (list_unlink): Non-idempotent
	version of list_unlink added here.
	* sysdeps/pthread/timer_delete.c: Use appropriate list unlink
	function in all places: idempotent one for timers, non-idempotent
	one for thread nodes.
	* sysdeps/pthread/timer_settime: Likewise.
	* sysdeps/pthread/timer_routines.c: Likewise.

7 files changed, 116 insertions, 36 deletions

diff --git a/linuxthreads/ChangeLog b/linuxthreads/ChangeLog
index 795c064..121eceb 100644
--- a/linuxthreads/ChangeLog
+++ b/linuxthreads/ChangeLog
@@ -1,3 +1,37 @@
+2000-06-13  Kaz Kylheku  <kaz@ashi.footprints.net>
+
+	A few optimizations.  Got rid of unnecessary wakeups of timer threads,
+	tightened up some critical regions and micro-optimized some list
+	manipulation code.
+
+	* sysdeps/pthread/timer_routines.c (__timer_thread_queue_timer):
+	Returns int value now to indicate whether timer was queued at head.
+	* sysdeps/pthread/posix-timer.h: Likewise.
+	* sysdeps/pthread/timer_settime.c (timer_settime): Takes advantage of
+	new return value from __timer_thread_queue_timer to avoid waking
+	up timer thread unnecessarily.
+
+	* sysdeps/pthread/posix-timer.h (timer_id2ptr): No longer checks
+	inuse flag, because this requires mutex to be held.  Callers updated
+	to do the check when they have the mutex.
+	* sysdeps/pthread/timer_getoverr.c: Add check for inuse here.
+
+	* sysdeps/pthread/timer_settime.c (timer_settime): Tighter critical
+	regions: avoids making system calls while holding timer mutex, and
+	a few computations were moved outside of the mutex as well.
+	* sysdeps/pthread/timer_gettime.c (timer_gettime): Likewise.
+
+	* sysdeps/pthread/posix-timer.h (list_unlink_ip): Function name changed
+	to list_unlink_ip, meaning idempotent.  Pointer manipulation
+	changed to get better better code out of gcc.
+	* sysdeps/pthread/timer_routines.c (list_unlink): Non-idempotent
+	version of list_unlink added here.
+	* sysdeps/pthread/timer_delete.c: Use appropriate list unlink
+	function in all places: idempotent one for timers, non-idempotent
+	one for thread nodes.
+	* sysdeps/pthread/timer_settime: Likewise.
+	* sysdeps/pthread/timer_routines.c: Likewise.
+
 2000-06-13  Ulrich Drepper  <drepper@redhat.com>
 
 	* sysdeps/unix/sysv/linux/bits/posix_opt.h (_POSIX_TIMERS): Define.
diff --git a/linuxthreads/sysdeps/pthread/posix-timer.h b/linuxthreads/sysdeps/pthread/posix-timer.h
index bb66c2d..feeff39 100644
--- a/linuxthreads/sysdeps/pthread/posix-timer.h
+++ b/linuxthreads/sysdeps/pthread/posix-timer.h
@@ -93,7 +93,7 @@ extern struct thread_node __timer_signal_thread_tclk;
 static inline struct timer_node *
 timer_id2ptr (timer_t timerid)
 {
-  if (timerid >= 0 && timerid < TIMER_MAX && __timer_array[timerid].inuse)
+  if (timerid >= 0 && timerid < TIMER_MAX)
     return &__timer_array[timerid];
 
   return NULL;
@@ -155,10 +155,17 @@ timespec_sub (struct timespec *diff, const struct timespec *left,
 
 /* We need one of the list functions in the other modules.  */
 static inline void
-list_unlink (struct list_links *list)
+list_unlink_ip (struct list_links *list)
 {
-  list->next->prev = list->prev;
-  list->prev->next = list->next;
+  struct list_links *lnext = list->next, *lprev = list->prev;
+
+  lnext->prev = lprev;
+  lprev->next = lnext;
+
+  /* The suffix ip means idempotent; list_unlink_ip can be called
+   * two or more times on the same node.
+   */
+
   list->next = list;
   list->prev = list;
 }
@@ -173,6 +180,6 @@ extern struct thread_node *__timer_thread_find_matching (const pthread_attr_t *d
 extern struct thread_node *__timer_thread_alloc (const pthread_attr_t *desired_attr, clockid_t);
 extern void __timer_dealloc (struct timer_node *timer);
 extern void __timer_thread_dealloc (struct thread_node *thread);
-extern void __timer_thread_queue_timer (struct thread_node *thread,
+extern int __timer_thread_queue_timer (struct thread_node *thread,
 				       struct timer_node *insert);
 extern void __timer_thread_wakeup (struct thread_node *thread);
diff --git a/linuxthreads/sysdeps/pthread/timer_delete.c b/linuxthreads/sysdeps/pthread/timer_delete.c
index 9e83ae1..4636bf7 100644
--- a/linuxthreads/sysdeps/pthread/timer_delete.c
+++ b/linuxthreads/sysdeps/pthread/timer_delete.c
@@ -36,7 +36,7 @@ timer_delete (timerid)
   pthread_mutex_lock (&__timer_mutex);
 
   timer = timer_id2ptr (timerid);
-  if (timer == NULL)
+  if (timer == NULL || !timer->inuse)
     /* Invalid timer ID or the timer is not in use.  */
     errno = EINVAL;
   else
@@ -58,7 +58,7 @@ timer_delete (timerid)
         }
 
       /* Remove timer from whatever queue it may be on and deallocate it.  */
-      list_unlink (&timer->links);
+      list_unlink_ip (&timer->links);
       __timer_dealloc (timer);
       retval = 0;
     }
diff --git a/linuxthreads/sysdeps/pthread/timer_getoverr.c b/linuxthreads/sysdeps/pthread/timer_getoverr.c
index f3aee45..8630f57 100644
--- a/linuxthreads/sysdeps/pthread/timer_getoverr.c
+++ b/linuxthreads/sysdeps/pthread/timer_getoverr.c
@@ -29,16 +29,15 @@ int
 timer_getoverrun (timerid)
      timer_t timerid;
 {
+  struct timer_node *timer;
   int retval = -1;
 
   pthread_mutex_lock (&__timer_mutex);
 
-  if (timer_id2ptr (timerid) == NULL)
+  if ((timer = timer_id2ptr (timerid)) == NULL || !timer->inuse)
     errno = EINVAL;
   else
-    {
-      retval = 0; /* TODO: overrun counting not supported */
-    }
+    retval = 0; /* TODO: overrun counting not supported */
 
   pthread_mutex_lock (&__timer_mutex);
 
diff --git a/linuxthreads/sysdeps/pthread/timer_gettime.c b/linuxthreads/sysdeps/pthread/timer_gettime.c
index 2db89a0..43b0759 100644
--- a/linuxthreads/sysdeps/pthread/timer_gettime.c
+++ b/linuxthreads/sysdeps/pthread/timer_gettime.c
@@ -37,7 +37,7 @@ timer_gettime (timerid, value)
   pthread_mutex_lock (&__timer_mutex);
 
   timer = timer_id2ptr (timerid);
-  if (timer == NULL)
+  if (timer == NULL && !timer->inuse)
     /* Invalid timer ID or the timer is not in use.  */
     errno = EINVAL;
   else
@@ -46,7 +46,9 @@ timer_gettime (timerid, value)
 
       if (timer->armed)
 	{
+	  pthread_mutex_unlock (&__timer_mutex);
 	  clock_gettime (timer->clock, &now);
+	  pthread_mutex_lock (&__timer_mutex);
 	  timespec_sub (&value->it_value, &timer->expirytime, &now);
 	}
       else
diff --git a/linuxthreads/sysdeps/pthread/timer_routines.c b/linuxthreads/sysdeps/pthread/timer_routines.c
index 520f6ee..ddf02fa 100644
--- a/linuxthreads/sysdeps/pthread/timer_routines.c
+++ b/linuxthreads/sysdeps/pthread/timer_routines.c
@@ -90,6 +90,20 @@ list_insbefore (struct list_links *list, struct list_links *newp)
   list_append (list, newp);
 }
 
+/*
+ * Like list_unlink_ip, except that calling it on a node that
+ * is already unlinked is disastrous rather than a noop.
+ */
+
+static inline void
+list_unlink (struct list_links *list)
+{
+  struct list_links *lnext = list->next, *lprev = list->prev;
+
+  lnext->prev = lprev;
+  lprev->next = lnext;
+}
+
 static inline struct list_links *
 list_first (struct list_links *list)
 {
@@ -279,10 +293,7 @@ thread_cleanup (void *val)
       thread->current_timer = 0;
 
       if (list_isempty (&thread->timer_queue))
-	{
-	  list_unlink (&thread->links);
 	  __timer_thread_dealloc (thread);
-	}
       else
 	(void) __timer_thread_start (thread);
 
@@ -394,7 +405,7 @@ thread_func (void *arg)
 	      if (timespec_compare (&now, &timer->expirytime) < 0)
 		break;
 
-	      list_unlink (first);
+	      list_unlink_ip (first);
 
 	      if (__builtin_expect (timer->value.it_interval.tv_sec, 0) != 0
 		  || timer->value.it_interval.tv_nsec != 0)
@@ -432,12 +443,16 @@ thread_func (void *arg)
 }
 
 
-/* Enqueue a timer in wakeup order in the thread's timer queue.  */
-void
+/* Enqueue a timer in wakeup order in the thread's timer queue. 
+   Returns 1 if the timer was inserted at the head of the queue,
+   causing the queue's next wakeup time to change. */
+
+int
 __timer_thread_queue_timer (struct thread_node *thread,
 			    struct timer_node *insert)
 {
   struct list_links *iter;
+  int athead = 1;
 
   for (iter = list_first (&thread->timer_queue);
        iter != list_null (&thread->timer_queue);
@@ -447,9 +462,11 @@ __timer_thread_queue_timer (struct thread_node *thread,
 
       if (timespec_compare (&insert->expirytime, &timer->expirytime) < 0)
 	  break;
+      athead = 0;
     }
 
   list_insbefore (iter, &insert->links);
+  return athead;
 }
 
 
@@ -533,7 +550,7 @@ __timer_alloc (void)
   if (node != list_null (&timer_free_list))
     {
       struct timer_node *timer = timer_links2ptr (node);
-      list_unlink (node);
+      list_unlink_ip (node);
       timer->inuse = 1;
       return timer;
     }
diff --git a/linuxthreads/sysdeps/pthread/timer_settime.c b/linuxthreads/sysdeps/pthread/timer_settime.c
index 63b117f..858edc7 100644
--- a/linuxthreads/sysdeps/pthread/timer_settime.c
+++ b/linuxthreads/sysdeps/pthread/timer_settime.c
@@ -35,11 +35,9 @@ timer_settime (timerid, flags, value, ovalue)
   struct timer_node *timer;
   struct thread_node *thread = NULL;
   struct timespec now;
-  int have_now = 0;
+  int have_now = 0, need_wakeup = 0;
   int retval = -1;
 
-  pthread_mutex_lock (&__timer_mutex);
-
   timer = timer_id2ptr (timerid);
   if (timer == NULL)
     {
@@ -56,14 +54,40 @@ timer_settime (timerid, flags, value, ovalue)
       goto bail;
     }
 
+  /* Will need to know current time since this is a relative timer;
+     might as well make the system call outside of the lock now! */
+
+  if ((flags & TIMER_ABSTIME) == 0)
+    {
+      clock_gettime (timer->clock, &now);
+      have_now = 1;
+    }
+
+  pthread_mutex_lock (&__timer_mutex);
+
+  /* One final check of timer validity; this one is possible only
+     until we have the mutex, which guards the inuse flag. */
+
+  if (!timer->inuse)
+    {
+      errno = EINVAL;
+      goto unlock_bail;
+    }
+
   if (ovalue != NULL)
     {
       ovalue->it_interval = timer->value.it_interval;
 
       if (timer->armed)
 	{
-	  clock_gettime (timer->clock, &now);
-	  have_now = 1;
+	  if (! have_now)
+	    {
+	      pthread_mutex_unlock (&__timer_mutex);
+	      clock_gettime (timer->clock, &now);
+	      have_now = 1;
+	      pthread_mutex_lock (&__timer_mutex);
+	    }
+
 	  timespec_sub (&ovalue->it_value, &timer->expirytime, &now);
 	}
       else
@@ -75,11 +99,12 @@ timer_settime (timerid, flags, value, ovalue)
 
   timer->value = *value;
 
-  list_unlink (&timer->links);
+  list_unlink_ip (&timer->links);
   timer->armed = 0;
 
   thread = timer->thread;
 
+  /* A value of { 0, 0 } causes the timer to be stopped. */
   if (value->it_value.tv_sec != 0
       || __builtin_expect (value->it_value.tv_nsec != 0, 1))
     {
@@ -87,25 +112,21 @@ timer_settime (timerid, flags, value, ovalue)
 	/* The user specified the expiration time.  */
 	timer->expirytime = value->it_value;
       else
-	{
-	  if (! have_now)
-	    clock_gettime (timer->clock, &now);
-
-	  timespec_add (&timer->expirytime, &now, &value->it_value);
-        }
+	timespec_add (&timer->expirytime, &now, &value->it_value);
 
-      __timer_thread_queue_timer (thread, timer);
+      /* Only need to wake up the thread if timer is inserted
+	 at the head of the queue. */
+      need_wakeup = __timer_thread_queue_timer (thread, timer);
       timer->armed = 1;
     }
 
   retval = 0;
 
-bail:
+unlock_bail:
   pthread_mutex_unlock (&__timer_mutex);
 
-  /* TODO: optimize this. Only need to wake up the thread if inserting
-     a new timer at the head of the queue.  */
-  if (thread != NULL)
+bail:
+  if (thread != NULL && need_wakeup)
     __timer_thread_wakeup (thread);
 
   return retval;