lib: make `show thread...` commands mt-aware

This patch fixes up show thread commands so that they know about
and operate on all extant thread_masters, since we can now have multiple
running in any given application.

This change also eliminates a heap use after free that appears when
using a single cpu_record shared among multiple threads. Since struct
thread's have pointers to bits of memory that are freed when the global
statistics hash table is freed, later accesses are invalid. By moving
the stats hash to be unique to each thread_master this problem is
sidestepped.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

2 files changed, 163 insertions, 164 deletions

diff --git a/lib/thread.c b/lib/thread.c
index feeffd31e..d3eb29313 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -47,18 +47,15 @@ DEFINE_MTYPE_STATIC(LIB, THREAD_STATS,  "Thread stats")
       write (m->io_pipe[1], &wakebyte, 1); \
   } while (0);
 
+/* control variable for initializer */
 pthread_once_t init_once = PTHREAD_ONCE_INIT;
-static pthread_mutex_t cpu_record_mtx = PTHREAD_MUTEX_INITIALIZER;
-static struct hash *cpu_record = NULL;
 pthread_key_t thread_current;
 
-static unsigned long
-timeval_elapsed (struct timeval a, struct timeval b)
-{
-  return (((a.tv_sec - b.tv_sec) * TIMER_SECOND_MICRO)
-	  + (a.tv_usec - b.tv_usec));
-}
+pthread_mutex_t masters_mtx = PTHREAD_MUTEX_INITIALIZER;
+static struct list *masters;
+
 
+/* CLI start ---------------------------------------------------------------- */
 static unsigned int
 cpu_record_hash_key (struct cpu_thread_history *a)
 {
@@ -108,12 +105,12 @@ vty_out_cpu_thread_history(struct vty* vty,
 }
 
 static void
-cpu_record_hash_print(struct hash_backet *bucket, 
-		      void *args[])
+cpu_record_hash_print(struct hash_backet *bucket, void *args[])
 {
   struct cpu_thread_history *totals = args[0];
   struct vty *vty = args[1];
   thread_type *filter = args[2];
+
   struct cpu_thread_history *a = bucket->data;
 
   if ( !(a->types & *filter) )
@@ -134,169 +131,159 @@ cpu_record_print(struct vty *vty, thread_type filter)
 {
   struct cpu_thread_history tmp;
   void *args[3] = {&tmp, vty, &filter};
+  struct thread_master *m;
+  struct listnode *ln;
+  int n = 0;
 
   memset(&tmp, 0, sizeof tmp);
   tmp.funcname = "TOTAL";
   tmp.types = filter;
 
-  vty_outln (vty, "%21s %18s %18s",
-	  "", "CPU (user+system):", "Real (wall-clock):");
-  vty_out(vty, "Active   Runtime(ms)   Invoked Avg uSec Max uSecs");
-  vty_out(vty, " Avg uSec Max uSecs");
-  vty_outln (vty, "  Type  Thread");
-
-  pthread_mutex_lock (&cpu_record_mtx);
+  pthread_mutex_lock (&masters_mtx);
   {
-    hash_iterate(cpu_record,
-                 (void(*)(struct hash_backet*,void*))cpu_record_hash_print,
-                 args);
+    for (ALL_LIST_ELEMENTS_RO (masters, ln, m)) {
+
+      vty_out (vty, VTYNL);
+      vty_outln(vty, "Showing statistics for pthread %d", n++);
+      vty_outln(vty, "-----------------------------------------------");
+      vty_outln(vty, "%21s %18s %18s",
+              "", "CPU (user+system):", "Real (wall-clock):");
+      vty_out(vty, "Active   Runtime(ms)   Invoked Avg uSec Max uSecs");
+      vty_out(vty, " Avg uSec Max uSecs");
+      vty_outln(vty, "  Type  Thread");
+
+      hash_iterate(m->cpu_record,
+                   (void (*)(struct hash_backet *, void *))
+                   cpu_record_hash_print,
+                   args);
+      vty_out(vty, VTYNL);
+    }
   }
-  pthread_mutex_unlock (&cpu_record_mtx);
+  pthread_mutex_unlock (&masters_mtx);
 
   if (tmp.total_calls > 0)
     vty_out_cpu_thread_history(vty, &tmp);
 }
 
-DEFUN (show_thread_cpu,
-       show_thread_cpu_cmd,
-       "show thread cpu [FILTER]",
-       SHOW_STR
-       "Thread information\n"
-       "Thread CPU usage\n"
-       "Display filter (rwtexb)\n")
-{
-  int idx_filter = 3;
-  int i = 0;
-  thread_type filter = (thread_type) -1U;
-
-  if (argc > 3)
-    {
-      filter = 0;
-      while (argv[idx_filter]->arg[i] != '\0')
-	{
-	  switch ( argv[idx_filter]->arg[i] )
-	    {
-	    case 'r':
-	    case 'R':
-	      filter |= (1 << THREAD_READ);
-	      break;
-	    case 'w':
-	    case 'W':
-	      filter |= (1 << THREAD_WRITE);
-	      break;
-	    case 't':
-	    case 'T':
-	      filter |= (1 << THREAD_TIMER);
-	      break;
-	    case 'e':
-	    case 'E':
-	      filter |= (1 << THREAD_EVENT);
-	      break;
-	    case 'x':
-	    case 'X':
-	      filter |= (1 << THREAD_EXECUTE);
-	      break;
-	    default:
-	      break;
-	    }
-	  ++i;
-	}
-      if (filter == 0)
-	{
-	  vty_outln (vty, "Invalid filter \"%s\" specified,"
-                  " must contain at least one of 'RWTEXB'",
-		  argv[idx_filter]->arg);
-	  return CMD_WARNING;
-	}
-    }
-
-  cpu_record_print(vty, filter);
-  return CMD_SUCCESS;
-}
-
 static void
-cpu_record_hash_clear (struct hash_backet *bucket, 
-		      void *args)
+cpu_record_hash_clear (struct hash_backet *bucket, void *args[])
 {
-  thread_type *filter = args;
+  thread_type *filter = args[0];
+  struct hash *cpu_record = args[1];
+
   struct cpu_thread_history *a = bucket->data;
 
   if ( !(a->types & *filter) )
        return;
   
-  pthread_mutex_lock (&cpu_record_mtx);
-  {
-    hash_release (cpu_record, bucket->data);
-  }
-  pthread_mutex_unlock (&cpu_record_mtx);
+  hash_release (cpu_record, bucket->data);
 }
 
 static void
 cpu_record_clear (thread_type filter)
 {
   thread_type *tmp = &filter;
+  struct thread_master *m;
+  struct listnode *ln;
 
-  pthread_mutex_lock (&cpu_record_mtx);
+  pthread_mutex_lock (&masters_mtx);
   {
-    hash_iterate (cpu_record,
-                  (void (*) (struct hash_backet*,void*)) cpu_record_hash_clear,
-                  tmp);
+    for (ALL_LIST_ELEMENTS_RO (masters, ln, m)) {
+      pthread_mutex_lock (&m->mtx);
+      {
+        void *args[2] = { tmp, m->cpu_record };
+        hash_iterate (m->cpu_record,
+                      (void (*) (struct hash_backet*,void*))
+                      cpu_record_hash_clear,
+                      args);
+      }
+      pthread_mutex_unlock (&m->mtx);
+    }
+  }
+  pthread_mutex_unlock (&masters_mtx);
+}
+
+static thread_type
+parse_filter (const char *filterstr)
+{
+  int i = 0;
+  int filter = 0;
+
+  while (filterstr[i] != '\0')
+    {
+      switch (filterstr[i])
+        {
+        case 'r':
+        case 'R':
+          filter |= (1 << THREAD_READ);
+          break;
+        case 'w':
+        case 'W':
+          filter |= (1 << THREAD_WRITE);
+          break;
+        case 't':
+        case 'T':
+          filter |= (1 << THREAD_TIMER);
+          break;
+        case 'e':
+        case 'E':
+          filter |= (1 << THREAD_EVENT);
+          break;
+        case 'x':
+        case 'X':
+          filter |= (1 << THREAD_EXECUTE);
+          break;
+        default:
+          break;
+        }
+      ++i;
+    }
+  return filter;
+}
+
+DEFUN (show_thread_cpu,
+       show_thread_cpu_cmd,
+       "show thread cpu [FILTER]",
+       SHOW_STR
+       "Thread information\n"
+       "Thread CPU usage\n"
+       "Display filter (rwtexb)\n")
+{
+  thread_type filter = (thread_type) -1U;
+  int idx = 0;
+
+  if (argv_find (argv, argc, "FILTER", &idx)) {
+    filter = parse_filter (argv[idx]->arg);
+    if (!filter) {
+      vty_outln(vty, "Invalid filter \"%s\" specified; must contain at least"
+                     "one of 'RWTEXB'%s", argv[idx]->arg);
+      return CMD_WARNING;
+    }
   }
-  pthread_mutex_unlock (&cpu_record_mtx);
+
+  cpu_record_print(vty, filter);
+  return CMD_SUCCESS;
 }
 
 DEFUN (clear_thread_cpu,
        clear_thread_cpu_cmd,
        "clear thread cpu [FILTER]",
-       "Clear stored data\n"
+       "Clear stored data in all pthreads\n"
        "Thread information\n"
        "Thread CPU usage\n"
        "Display filter (rwtexb)\n")
 {
-  int idx_filter = 3;
-  int i = 0;
   thread_type filter = (thread_type) -1U;
-
-  if (argc > 3)
-    {
-      filter = 0;
-      while (argv[idx_filter]->arg[i] != '\0')
-	{
-	  switch ( argv[idx_filter]->arg[i] )
-	    {
-	    case 'r':
-	    case 'R':
-	      filter |= (1 << THREAD_READ);
-	      break;
-	    case 'w':
-	    case 'W':
-	      filter |= (1 << THREAD_WRITE);
-	      break;
-	    case 't':
-	    case 'T':
-	      filter |= (1 << THREAD_TIMER);
-	      break;
-	    case 'e':
-	    case 'E':
-	      filter |= (1 << THREAD_EVENT);
-	      break;
-	    case 'x':
-	    case 'X':
-	      filter |= (1 << THREAD_EXECUTE);
-	      break;
-	    default:
-	      break;
-	    }
-	  ++i;
-	}
-      if (filter == 0)
-	{
-	  vty_outln (vty, "Invalid filter \"%s\" specified,"
-                  " must contain at least one of 'RWTEXB'",
-		  argv[idx_filter]->arg);
-	  return CMD_WARNING;
-	}
+  int idx = 0;
+
+  if (argv_find (argv, argc, "FILTER", &idx)) {
+    filter = parse_filter (argv[idx]->arg);
+    if (!filter) {
+      vty_outln(vty, "Invalid filter \"%s\" specified; must contain at least"
+                     "one of 'RWTEXB'%s", argv[idx]->arg);
+      return CMD_WARNING;
     }
+  }
 
   cpu_record_clear (filter);
   return CMD_SUCCESS;
@@ -308,6 +295,8 @@ thread_cmd_init (void)
   install_element (VIEW_NODE, &show_thread_cpu_cmd);
   install_element (ENABLE_NODE, &clear_thread_cpu_cmd);
 }
+/* CLI end ------------------------------------------------------------------ */
+
 
 static int
 thread_timer_cmp(void *a, void *b)
@@ -339,10 +328,8 @@ cancelreq_del (void *cr)
 /* initializer, only ever called once */
 static void initializer ()
 {
-  if (cpu_record == NULL)
-    cpu_record = hash_create ((unsigned int (*) (void *))cpu_record_hash_key,
-                              (int (*) (const void *, const void *))
-                              cpu_record_hash_cmp);
+  if (!masters)
+    masters = list_new();
 
   pthread_key_create (&thread_current, NULL);
 }
@@ -354,17 +341,18 @@ thread_master_create (void)
   struct thread_master *rv;
   struct rlimit limit;
 
-  getrlimit(RLIMIT_NOFILE, &limit);
-
   pthread_once (&init_once, &initializer);
 
   rv = XCALLOC (MTYPE_THREAD_MASTER, sizeof (struct thread_master));
   if (rv == NULL)
     return NULL;
 
+  /* Initialize master mutex */
   pthread_mutex_init (&rv->mtx, NULL);
   pthread_cond_init (&rv->cancel_cond, NULL);
 
+  /* Initialize I/O task data structures */
+  getrlimit(RLIMIT_NOFILE, &limit);
   rv->fd_limit = (int)limit.rlim_cur;
   rv->read = XCALLOC (MTYPE_THREAD, sizeof (struct thread *) * rv->fd_limit);
   if (rv->read == NULL)
@@ -372,7 +360,6 @@ thread_master_create (void)
       XFREE (MTYPE_THREAD_MASTER, rv);
       return NULL;
     }
-
   rv->write = XCALLOC (MTYPE_THREAD, sizeof (struct thread *) * rv->fd_limit);
   if (rv->write == NULL)
     {
@@ -381,20 +368,32 @@ thread_master_create (void)
       return NULL;
     }
 
+  rv->cpu_record = hash_create ((unsigned int (*) (void *))cpu_record_hash_key,
+                                (int (*) (const void *, const void *))
+                                cpu_record_hash_cmp);
+
+
   /* Initialize the timer queues */
   rv->timer = pqueue_create();
   rv->timer->cmp = thread_timer_cmp;
   rv->timer->update = thread_timer_update;
+
+  /* Initialize thread_fetch() settings */
   rv->spin = true;
   rv->handle_signals = true;
+
+  /* Set pthread owner, should be updated by actual owner */
   rv->owner = pthread_self();
   rv->cancel_req = list_new ();
   rv->cancel_req->del = cancelreq_del;
   rv->canceled = true;
+
+  /* Initialize pipe poker */
   pipe (rv->io_pipe);
   set_nonblocking (rv->io_pipe[0]);
   set_nonblocking (rv->io_pipe[1]);
 
+  /* Initialize data structures for poll() */
   rv->handler.pfdsize = rv->fd_limit;
   rv->handler.pfdcount = 0;
   rv->handler.pfds = XCALLOC (MTYPE_THREAD_MASTER,
@@ -402,6 +401,13 @@ thread_master_create (void)
   rv->handler.copy = XCALLOC (MTYPE_THREAD_MASTER,
                               sizeof (struct pollfd) * rv->handler.pfdsize);
 
+  /* add to list */
+  pthread_mutex_lock (&masters_mtx);
+  {
+    listnode_add (masters, rv);
+  }
+  pthread_mutex_unlock (&masters_mtx);
+
   return rv;
 }
 
@@ -551,20 +557,13 @@ thread_master_free (struct thread_master *m)
   close (m->io_pipe[1]);
   list_delete (m->cancel_req);
 
+  hash_clean (m->cpu_record, cpu_record_hash_free);
+  hash_free (m->cpu_record);
+  m->cpu_record = NULL;
+
   XFREE (MTYPE_THREAD_MASTER, m->handler.pfds);
   XFREE (MTYPE_THREAD_MASTER, m->handler.copy);
   XFREE (MTYPE_THREAD_MASTER, m);
-
-  pthread_mutex_lock (&cpu_record_mtx);
-  {
-    if (cpu_record)
-      {
-        hash_clean (cpu_record, cpu_record_hash_free);
-        hash_free (cpu_record);
-        cpu_record = NULL;
-      }
-  }
-  pthread_mutex_unlock (&cpu_record_mtx);
 }
 
 /* Return remain time in second. */
@@ -636,12 +635,8 @@ thread_get (struct thread_master *m, u_char type,
     {
       tmp.func = func;
       tmp.funcname = funcname;
-      pthread_mutex_lock (&cpu_record_mtx);
-      {
-        thread->hist = hash_get (cpu_record, &tmp,
-                                 (void * (*) (void *))cpu_record_hash_alloc);
-      }
-      pthread_mutex_unlock (&cpu_record_mtx);
+      thread->hist = hash_get (m->cpu_record, &tmp,
+                               (void * (*) (void *))cpu_record_hash_alloc);
     }
   thread->hist->total_active++;
   thread->func = func;
@@ -1408,6 +1403,13 @@ thread_fetch (struct thread_master *m, struct thread *fetch)
   return fetch;
 }
 
+static unsigned long
+timeval_elapsed (struct timeval a, struct timeval b)
+{
+  return (((a.tv_sec - b.tv_sec) * TIMER_SECOND_MICRO)
+	  + (a.tv_usec - b.tv_usec));
+}
+
 unsigned long
 thread_consumed_time (RUSAGE_T *now, RUSAGE_T *start, unsigned long *cputime)
 {
@@ -1523,12 +1525,8 @@ funcname_thread_execute (struct thread_master *m,
 
   tmp.func = dummy.func = func;
   tmp.funcname = dummy.funcname = funcname;
-  pthread_mutex_lock (&cpu_record_mtx);
-  {
-    dummy.hist = hash_get (cpu_record, &tmp,
-                           (void * (*) (void *))cpu_record_hash_alloc);
-  }
-  pthread_mutex_unlock (&cpu_record_mtx);
+  dummy.hist = hash_get (m->cpu_record, &tmp,
+                         (void * (*) (void *))cpu_record_hash_alloc);
 
   dummy.schedfrom = schedfrom;
   dummy.schedfrom_line = fromln;
diff --git a/lib/thread.h b/lib/thread.h
index 1760a930f..12c85fc14 100644
--- a/lib/thread.h
+++ b/lib/thread.h
@@ -80,6 +80,7 @@ struct thread_master
   struct list *cancel_req;
   bool canceled;
   pthread_cond_t cancel_cond;
+  struct hash *cpu_record;
   int io_pipe[2];
   int fd_limit;
   struct fd_handler handler;