Update.
authorUlrich Drepper <drepper@redhat.com>
Wed, 14 Jun 2000 06:13:45 +0000 (06:13 +0000)
committerUlrich Drepper <drepper@redhat.com>
Wed, 14 Jun 2000 06:13:45 +0000 (06:13 +0000)
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.

linuxthreads/ChangeLog
linuxthreads/sysdeps/pthread/posix-timer.h
linuxthreads/sysdeps/pthread/timer_delete.c
linuxthreads/sysdeps/pthread/timer_getoverr.c
linuxthreads/sysdeps/pthread/timer_gettime.c
linuxthreads/sysdeps/pthread/timer_routines.c
linuxthreads/sysdeps/pthread/timer_settime.c

index 795c064..121eceb 100644 (file)
@@ -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.
index bb66c2d..feeff39 100644 (file)
@@ -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);
index 9e83ae1..4636bf7 100644 (file)
@@ -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;
     }
index f3aee45..8630f57 100644 (file)
@@ -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);
 
index 2db89a0..43b0759 100644 (file)
@@ -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
index 520f6ee..ddf02fa 100644 (file)
@@ -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;
     }
index 63b117f..858edc7 100644 (file)
@@ -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;