RFA [threads]: Thread cache

RFA [threads]: Thread cache

[Daniel Jacobowitz]

I've figured out how to fix print-threads.exp (see my ramblings on gdb@
yesterday for a bad description of the problem; better coming soon). 
However, to do it, I discovered that it was actually _required_ that we
cache certain information from libthread_db, instead of merely beneficial.

So I implemented the cache.  This patch is the entire cache mechanism,
except for updating the comment at the top of the file saying we need one.
Before I get to the patch itself, some numbers:

My test for these things is starting GDB, loading Mozilla, running until the
first dialog box, clicking exit, and waiting until the GDB prompt comes
back.  CPU time, not wall clock time (I'm not that reliable!).

Before: from 13.7 to 15.0 seconds.  The test is not the most reliable, as
anywhere from seven to ten threads seem to be created before it asks me what
profile I want [why?]; but always at least seven.  Strace reveals on the
order of 283,500 pread() calls, i.e. target memory reads.

After: 10.6 seconds, and 74400 pread() calls, i.e. less by almost a factor
of four.

[I have other patches in progress to shave possibly as much as another four
seconds off this testcase.]


Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
convert the others too; I will do that in a separate patch if this one is
approved, and see how much more it takes off the runtime] with calls to
wrapper functions which cache the data in the struct private_thread_info.
The cache is invalidated at every resume(); there's some information that we
could keep if we are guaranteed a 1-to-1 threads implementation with no
migration, like LinuxThreads or NPTL, but I'm being conservative for now.

The comment above thread_get_info_callback is slightly misleading; it's not
used as a callback for the thread_db iterator routine until my next patch. 
For now it's just a helper for thread_from_lwp.

The call to target_pid_to_str is moved below the call to add_thread in
attach_thread(), since the cache requires that a struct thread_info * exist
for the ptid being printed.

Oh, and in thread_db_pid_to_str I replace an error () with putting "Missing"
into the string; there's no point in target_pid_to_str failing, since it's
only used for display.

I also remove some dead code which used to share the cache structure with
lin-lwp.c; it's been commented out for a year.

The rest is mechanical changes to use the new functions.

Here it is.  Comments?  OK?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-01-10  Daniel Jacobowitz  <drow@mvista.com>

	* lin-lwp.c (struct private_thread_info, find_lwp_callback): Remove.
	(resume_callback): Remove dead code.

	* thread-db.c (attach_thread): Prototype.
	(struct private_thread_info): Remove lwpid.  Add thread handle (th),
	thread information (ti), and valid flags (th_valid, ti_valid).
	(attach_thread): Move target_pid_to_str call to after the thread
	is added to GDB's list.  Initialize the cache.
	(thread_get_info_callback, thread_db_map_id2thr)
	(thread_db_get_info): New functions.
	(thread_from_lwp, lwp_from_thread, thread_db_fetch_registers)
	(thread_db_store_registers, thread_db_thread_alive)
	(thread_db_get_thread_local_address): Use them.
	(thread_db_pid_to_str): Likewise.  Return "Missing" instead
	of calling error() for threads in unknown state.

	(clear_lwpid_callback): New function.
	(thread_db_resume): Use it to clear the cache.

--- gdb/thread-db.c	2003-01-10 15:15:11.000000000 -0500
+++ gdb/thread-db.c	2003-01-10 15:09:03.000000000 -0500
@@ -126,6 +126,8 @@
 
 /* Prototypes for local functions.  */
 static void thread_db_find_new_threads (void);
+static void attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
+			   const td_thrinfo_t *ti_p, int verbose);
 \f
 
 /* Building process ids.  */
@@ -141,10 +143,17 @@
 #define BUILD_THREAD(tid, pid)	ptid_build (pid, 0, tid)
 \f
 
+/* Use "struct private_thread_info" to cache thread state.  This is
+   a substantial optimization.  */
+
 struct private_thread_info
 {
-  /* Cached LWP id.  Must come first, see lin-lwp.c.  */
-  lwpid_t lwpid;
+  /* Cached thread state.  */
+  unsigned int th_valid : 1;
+  unsigned int ti_valid : 1;
+
+  td_thrhandle_t th;
+  td_thrinfo_t ti;
 };
 \f
 
@@ -228,15 +237,100 @@
     }
 }
 \f
+/* A callback function for td_ta_thr_iter, which we use to map all
+   threads to LWPs.  
+
+   THP is a handle to the current thread; if INFOP is not NULL, the
+   struct thread_info associated with this thread is returned in
+   *INFOP.  */
+
+static int
+thread_get_info_callback (const td_thrhandle_t *thp, void *infop)
+{
+  td_thrinfo_t ti;
+  td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
+
+  err = td_thr_get_info_p (thp, &ti);
+  if (err != TD_OK)
+    error ("thread_get_info_callback: cannot get thread info: %s", 
+	   thread_db_err_str (err));
+
+  /* Fill the cache.  */
+  thread_ptid = BUILD_THREAD (ti.ti_tid, GET_PID (inferior_ptid));
+  thread_info = find_thread_pid (thread_ptid);
+
+  if (thread_info == NULL)
+    {
+      /* New thread.  Attach to it now (why wait?).  */
+      attach_thread (thread_ptid, thp, &ti, 1);
+      thread_info = find_thread_pid (thread_ptid);
+      gdb_assert (thread_info != NULL);
+    }
+
+  memcpy (&thread_info->private->th, thp, sizeof (*thp));
+  thread_info->private->th_valid = 1;
+  memcpy (&thread_info->private->ti, &ti, sizeof (ti));
+  thread_info->private->ti_valid = 1;
+
+  if (infop != NULL)
+    *(struct thread_info **) infop = thread_info;
+
+  return 0;
+}
+
+/* Accessor functions for the thread_db information, with caching.  */
 
+static void
+thread_db_map_id2thr (struct thread_info *thread_info, int fatal)
+{
+  td_err_e err;
+
+  if (thread_info->private->th_valid)
+    return;
+
+  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (thread_info->ptid),
+			    &thread_info->private->th);
+  if (err != TD_OK)
+    {
+      if (fatal)
+	error ("Cannot find thread %ld: %s",
+	       (long) GET_THREAD (thread_info->ptid), thread_db_err_str (err));
+    }
+  else
+    thread_info->private->th_valid = 1;
+}
+
+static td_thrinfo_t *
+thread_db_get_info (struct thread_info *thread_info)
+{
+  td_err_e err;
+
+  if (thread_info->private->ti_valid)
+    return &thread_info->private->ti;
+
+  if (! thread_info->private->th_valid)
+    thread_db_map_id2thr (thread_info, 1);
+
+  err = td_thr_get_info_p (&thread_info->private->th, &thread_info->private->ti);
+  if (err != TD_OK)
+    error ("thread_db_get_info: cannot get thread info: %s", 
+	   thread_db_err_str (err));
+
+  thread_info->private->ti_valid = 1;
+  return &thread_info->private->ti;
+}
+\f
 /* Convert between user-level thread ids and LWP ids.  */
 
 static ptid_t
 thread_from_lwp (ptid_t ptid)
 {
-  td_thrinfo_t ti;
   td_thrhandle_t th;
   td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
 
   if (GET_LWP (ptid) == 0)
     ptid = BUILD_LWP (GET_PID (ptid), GET_PID (ptid));
@@ -248,35 +342,26 @@
     error ("Cannot find user-level thread for LWP %ld: %s",
 	   GET_LWP (ptid), thread_db_err_str (err));
 
-  err = td_thr_get_info_p (&th, &ti);
-  if (err != TD_OK)
-    error ("thread_from_lwp: cannot get thread info: %s", 
-	   thread_db_err_str (err));
+  thread_info = NULL;
+  thread_get_info_callback (&th, &thread_info);
+  gdb_assert (thread_info && thread_info->private->ti_valid);
 
-  return BUILD_THREAD (ti.ti_tid, GET_PID (ptid));
+  return BUILD_THREAD (thread_info->private->ti.ti_tid, GET_PID (ptid));
 }
 
 static ptid_t
 lwp_from_thread (ptid_t ptid)
 {
-  td_thrinfo_t ti;
-  td_thrhandle_t th;
-  td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
 
   if (!is_thread (ptid))
     return ptid;
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (ptid), thread_db_err_str (err));
-
-  err = td_thr_get_info_p (&th, &ti);
-  if (err != TD_OK)
-    error ("lwp_from_thread: cannot get thread info: %s", 
-	   thread_db_err_str (err));
+  thread_info = find_thread_pid (ptid);
+  thread_db_get_info (thread_info);
 
-  return BUILD_LWP (ti.ti_lid, GET_PID (ptid));
+  return BUILD_LWP (thread_info->private->ti.ti_lid, GET_PID (ptid));
 }
 \f
 
@@ -581,13 +666,13 @@
 
   check_thread_signals ();
 
-  if (verbose)
-    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
-
   /* Add the thread to GDB's thread list.  */
   tp = add_thread (ptid);
   tp->private = xmalloc (sizeof (struct private_thread_info));
-  tp->private->lwpid = ti_p->ti_lid;
+  memset (tp->private, 0, sizeof (struct private_thread_info));
+
+  if (verbose)
+    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
 
   if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
     return;			/* A zombie thread -- do not attach.  */
@@ -644,6 +729,19 @@
   target_beneath->to_detach (args, from_tty);
 }
 
+static int
+clear_lwpid_callback (struct thread_info *thread, void *dummy)
+{
+  /* If we know that our thread implementation is 1-to-1, we could save
+     a certain amount of information; it's not clear how much, so we
+     are always conservative.  */
+
+  thread->private->th_valid = 0;
+  thread->private->ti_valid = 0;
+
+  return 0;
+}
+
 static void
 thread_db_resume (ptid_t ptid, int step, enum target_signal signo)
 {
@@ -654,6 +752,9 @@
   else if (is_thread (ptid))
     ptid = lwp_from_thread (ptid);
 
+  /* Clear cached data which may not be valid after the resume.  */
+  iterate_over_threads (clear_lwpid_callback, NULL);
+
   target_beneath->to_resume (ptid, step, signo);
 
   do_cleanups (old_chain);
@@ -787,7 +888,7 @@
 static void
 thread_db_fetch_registers (int regno)
 {
-  td_thrhandle_t th;
+  struct thread_info *thread_info;
   prgregset_t gregset;
   gdb_prfpregset_t fpregset;
   td_err_e err;
@@ -799,17 +900,15 @@
       return;
     }
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (inferior_ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
+  thread_info = find_thread_pid (inferior_ptid);
+  thread_db_map_id2thr (thread_info, 1);
 
-  err = td_thr_getgregs_p (&th, gregset);
+  err = td_thr_getgregs_p (&thread_info->private->th, gregset);
   if (err != TD_OK)
     error ("Cannot fetch general-purpose registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
 
-  err = td_thr_getfpregs_p (&th, &fpregset);
+  err = td_thr_getfpregs_p (&thread_info->private->th, &fpregset);
   if (err != TD_OK)
     error ("Cannot get floating-point registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
@@ -824,10 +923,10 @@
 static void
 thread_db_store_registers (int regno)
 {
-  td_thrhandle_t th;
   prgregset_t gregset;
   gdb_prfpregset_t fpregset;
   td_err_e err;
+  struct thread_info *thread_info;
 
   if (!is_thread (inferior_ptid))
     {
@@ -836,10 +935,8 @@
       return;
     }
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (inferior_ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
+  thread_info = find_thread_pid (inferior_ptid);
+  thread_db_map_id2thr (thread_info, 1);
 
   if (regno != -1)
     {
@@ -853,11 +950,11 @@
   fill_gregset ((gdb_gregset_t *) gregset, -1);
   fill_fpregset (&fpregset, -1);
 
-  err = td_thr_setgregs_p (&th, gregset);
+  err = td_thr_setgregs_p (&thread_info->private->th, gregset);
   if (err != TD_OK)
     error ("Cannot store general-purpose registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-  err = td_thr_setfpregs_p (&th, &fpregset);
+  err = td_thr_setfpregs_p (&thread_info->private->th, &fpregset);
   if (err != TD_OK)
     error ("Cannot store floating-point registers  for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
@@ -915,24 +1012,31 @@
 thread_db_thread_alive (ptid_t ptid)
 {
   td_thrhandle_t th;
-  td_thrinfo_t ti;
   td_err_e err;
 
   if (is_thread (ptid))
     {
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	return 0;
+      struct thread_info *thread_info;
+      thread_info = find_thread_pid (ptid);
 
-      err = td_thr_validate_p (&th);
-      if (err != TD_OK)
+      thread_db_map_id2thr (thread_info, 0);
+      if (! thread_info->private->th_valid)
 	return 0;
 
-      err = td_thr_get_info_p (&th, &ti);
+      err = td_thr_validate_p (&thread_info->private->th);
       if (err != TD_OK)
 	return 0;
 
-      if (ti.ti_state == TD_THR_UNKNOWN || ti.ti_state == TD_THR_ZOMBIE)
+      if (! thread_info->private->ti_valid)
+	{
+	  err = td_thr_get_info_p (&thread_info->private->th, &thread_info->private->ti);
+	  if (err != TD_OK)
+	    return 0;
+	  thread_info->private->ti_valid = 1;
+	}
+
+      if (thread_info->private->ti.ti_state == TD_THR_UNKNOWN
+	  || thread_info->private->ti.ti_state == TD_THR_ZOMBIE)
 	return 0;		/* A zombie thread.  */
 
       return 1;
@@ -986,29 +1090,29 @@
   if (is_thread (ptid))
     {
       static char buf[64];
-      td_thrhandle_t th;
-      td_thrinfo_t ti;
+      td_thrinfo_t *ti_p;
       td_err_e err;
+      struct thread_info *thread_info;
 
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	error ("Cannot find thread %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
+      thread_info = find_thread_pid (ptid);
+      thread_db_map_id2thr (thread_info, 0);
+      if (! thread_info->private->th_valid)
+	{
+	  snprintf (buf, sizeof (buf), "Thread %ld (Missing)", GET_THREAD (ptid));
+	  return buf;
+	}
 
-      err = td_thr_get_info_p (&th, &ti);
-      if (err != TD_OK)
-	error ("thread_db_pid_to_str: cannot get thread info for %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
+      ti_p = thread_db_get_info (thread_info);
 
-      if (ti.ti_state == TD_THR_ACTIVE && ti.ti_lid != 0)
+      if (ti_p->ti_state == TD_THR_ACTIVE && ti_p->ti_lid != 0)
 	{
 	  snprintf (buf, sizeof (buf), "Thread %ld (LWP %d)",
-		    (long) ti.ti_tid, ti.ti_lid);
+		    (long) ti_p->ti_tid, ti_p->ti_lid);
 	}
       else
 	{
 	  snprintf (buf, sizeof (buf), "Thread %ld (%s)",
-		    (long) ti.ti_tid, thread_db_state_str (ti.ti_state));
+		    (long) ti_p->ti_tid, thread_db_state_str (ti_p->ti_state));
 	}
 
       return buf;
@@ -1031,9 +1135,9 @@
     {
       int objfile_is_library = (objfile->flags & OBJF_SHARED);
       td_err_e err;
-      td_thrhandle_t th;
       void *address;
       CORE_ADDR lm;
+      struct thread_info *thread_info;
 
       /* glibc doesn't provide the needed interface.  */
       if (! td_thr_tls_get_addr_p)
@@ -1054,13 +1158,12 @@
 	}
 
       /* Get info about the thread.  */
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	error ("Cannot find thread %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
-      
+      thread_info = find_thread_pid (ptid);
+      thread_db_map_id2thr (thread_info, 1);
+
       /* Finally, get the address of the variable.  */
-      err = td_thr_tls_get_addr_p (&th, (void *) lm, offset, &address);
+      err = td_thr_tls_get_addr_p (&thread_info->private->th, (void *) lm,
+				   offset, &address);
 
 #ifdef THREAD_DB_HAS_TD_NOTALLOC
       /* The memory hasn't been allocated, yet.  */
Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.41
diff -u -p -r1.41 lin-lwp.c
--- lin-lwp.c	9 Jan 2003 19:14:46 -0000	1.41
+++ lin-lwp.c	10 Jan 2003 20:25:13 -0000
@@ -536,25 +536,6 @@ lin_lwp_detach (char *args, int from_tty
 }
 \f
 
-struct private_thread_info
-{
-  int lwpid;
-};
-
-/* Return non-zero if TP corresponds to the LWP specified by DATA
-   (which is assumed to be a pointer to a `struct lwp_info'.  */
-
-static int
-find_lwp_callback (struct thread_info *tp, void *data)
-{
-  struct lwp_info *lp = data;
-
-  if (tp->private->lwpid == GET_LWP (lp->ptid))
-    return 1;
-
-  return 0;
-}
-
 /* Resume LP.  */
 
 static int
@@ -563,27 +544,6 @@ resume_callback (struct lwp_info *lp, vo
   if (lp->stopped && lp->status == 0)
     {
       struct thread_info *tp;
-
-#if 0
-      /* FIXME: kettenis/2000-08-26: This should really be handled
-         properly by core GDB.  */
-
-      tp = find_thread_pid (lp->ptid);
-      if (tp == NULL)
-	tp = iterate_over_threads (find_lwp_callback, lp);
-      gdb_assert (tp);
-
-      /* If we were previously stepping the thread, and now continue
-         the thread we must invalidate the stepping range.  However,
-         if there is a step_resume breakpoint for this thread, we must
-         preserve the stepping range to make it possible to continue
-         stepping once we hit it.  */
-      if (tp->step_range_end && tp->step_resume_breakpoint == NULL)
-	{
-	  gdb_assert (lp->step);
-	  tp->step_range_start = tp->step_range_end = 0;
-	}
-#endif
 
       child_resume (pid_to_ptid (GET_LWP (lp->ptid)), 0, TARGET_SIGNAL_0);
       if (debug_lin_lwp)

Re: RFA [threads]: Thread cache

[Mark Kettenis]

Daniel Jacobowitz <drow@mvista.com> writes:

> I've figured out how to fix print-threads.exp (see my ramblings on gdb@
> yesterday for a bad description of the problem; better coming soon). 
> However, to do it, I discovered that it was actually _required_ that we
> cache certain information from libthread_db, instead of merely beneficial.
>
> So I implemented the cache.  This patch is the entire cache mechanism,
> except for updating the comment at the top of the file saying we need one.
> Before I get to the patch itself, some numbers:

This looks good!  Please check it in, regardless of the things I say
further on in this message.

> Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
> and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
> convert the others too; I will do that in a separate patch if this one is
> approved, and see how much more it takes off the runtime] with calls to
> wrapper functions which cache the data in the struct private_thread_info.
> The cache is invalidated at every resume(); there's some information that we
> could keep if we are guaranteed a 1-to-1 threads implementation with no
> migration, like LinuxThreads or NPTL, but I'm being conservative for now.

Note that the thread_db.h interface provides TD_SWITCHTO and
TD_SWITCHFROM events.  I'd be perfectly happy if you'd cache info
about the LWP a particular user-level thread is bound to if you'd
invalidate this info upon receiving those events (which should never
happen in a 1-to-1 threads implementation.

That said, would re-enabling TD_DEATH events somehow make things more
robust for you?  TD_DEATH was broken in glibc 2.1.3, but anybody who's
doing any serious threads development should be using a more recent
glibc.

> The call to target_pid_to_str is moved below the call to add_thread in
> attach_thread(), since the cache requires that a struct thread_info * exist
> for the ptid being printed.

Makes sense.

> Oh, and in thread_db_pid_to_str I replace an error () with putting "Missing"
> into the string; there's no point in target_pid_to_str failing, since it's
> only used for display.

Fine with me.

Thanks!

Mark

Re: RFA [threads]: Thread cache

[Andrew Cagney]

> I also remove some dead code which used to share the cache structure with
> lin-lwp.c; it's been commented out for a year.

Don't forget, as always, that should be separated out and committed 
independently.

Andrew

Re: RFA [threads]: Thread cache

[Daniel Jacobowitz]

On Sat, Jan 11, 2003 at 01:13:55PM +0100, Mark Kettenis wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> 
> > I've figured out how to fix print-threads.exp (see my ramblings on gdb@
> > yesterday for a bad description of the problem; better coming soon). 
> > However, to do it, I discovered that it was actually _required_ that we
> > cache certain information from libthread_db, instead of merely beneficial.
> >
> > So I implemented the cache.  This patch is the entire cache mechanism,
> > except for updating the comment at the top of the file saying we need one.
> > Before I get to the patch itself, some numbers:
> 
> This looks good!  Please check it in, regardless of the things I say
> further on in this message.
> 
> > Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
> > and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
> > convert the others too; I will do that in a separate patch if this one is
> > approved, and see how much more it takes off the runtime] with calls to
> > wrapper functions which cache the data in the struct private_thread_info.
> > The cache is invalidated at every resume(); there's some information that we
> > could keep if we are guaranteed a 1-to-1 threads implementation with no
> > migration, like LinuxThreads or NPTL, but I'm being conservative for now.
> 
> Note that the thread_db.h interface provides TD_SWITCHTO and
> TD_SWITCHFROM events.  I'd be perfectly happy if you'd cache info
> about the LWP a particular user-level thread is bound to if you'd
> invalidate this info upon receiving those events (which should never
> happen in a 1-to-1 threads implementation.
> 
> That said, would re-enabling TD_DEATH events somehow make things more
> robust for you?  TD_DEATH was broken in glibc 2.1.3, but anybody who's
> doing any serious threads development should be using a more recent
> glibc.

I don't think that TD_DEATH would solve the problem.  Let me check when
they're actually delivered... no, TD_DEATH is reported immediately
before the terminated flag is set, i.e. the thread becomes a "zombie". 
The problem is that we need to continue debugging the thread until it's
actually _gone_.  Hmm, enabling TD_DEATH might be useful in that it
would simplify the ad-hoc-ness of the zombie handling; it could prevent
having to iterate over all LWPs, which would be nice.  I'll thik about
it.

Now, relying on SWITCHTO/SWITCHFROM has some potential.  The difficulty
is that using them would slow down debugging of a M-to-N implementation
substantially, I wager.  But we could at least do something like "assume
1-to-1 for caching purposes until we get a SWITCHTO/SWITCHFROM".  Hmm,
I need to think about that too.

Is there formal documentation for the libthread_db interface somewhere? 
Glibc certainly doesn't have any.  For instance, I'd like to know if I
can safely cache the thread handles across resumes; if I could, this
would be much much much much easier to do efficiently.  We could get
the thread handle and LWP when the thread is created, and then hold the
thread handle, and optionally hold the LWP.  I am pretty sure this is
safe given glibc, but I don't know in general.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA [threads]: Thread cache

[Daniel Jacobowitz]

On Sat, Jan 11, 2003 at 01:13:55PM +0100, Mark Kettenis wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> 
> > I've figured out how to fix print-threads.exp (see my ramblings on gdb@
> > yesterday for a bad description of the problem; better coming soon). 
> > However, to do it, I discovered that it was actually _required_ that we
> > cache certain information from libthread_db, instead of merely beneficial.
> >
> > So I implemented the cache.  This patch is the entire cache mechanism,
> > except for updating the comment at the top of the file saying we need one.
> > Before I get to the patch itself, some numbers:
> 
> This looks good!  Please check it in, regardless of the things I say
> further on in this message.

Oh, and: checked in, in two parts.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA [threads]: Thread cache

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Sat, Jan 11, 2003 at 01:13:55PM +0100, Mark Kettenis wrote:
> > Daniel Jacobowitz <drow@mvista.com> writes:
> >
> > > I've figured out how to fix print-threads.exp (see my ramblings on gdb@
> > > yesterday for a bad description of the problem; better coming soon).
> > > However, to do it, I discovered that it was actually _required_ that we
> > > cache certain information from libthread_db, instead of merely beneficial.
> > >
> > > So I implemented the cache.  This patch is the entire cache mechanism,
> > > except for updating the comment at the top of the file saying we need one.
> > > Before I get to the patch itself, some numbers:
> >
> > This looks good!  Please check it in, regardless of the things I say
> > further on in this message.
> >
> > > Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
> > > and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
> > > convert the others too; I will do that in a separate patch if this one is
> > > approved, and see how much more it takes off the runtime] with calls to
> > > wrapper functions which cache the data in the struct private_thread_info.
> > > The cache is invalidated at every resume(); there's some information that we
> > > could keep if we are guaranteed a 1-to-1 threads implementation with no
> > > migration, like LinuxThreads or NPTL, but I'm being conservative for now.
> >
> > Note that the thread_db.h interface provides TD_SWITCHTO and
> > TD_SWITCHFROM events.  I'd be perfectly happy if you'd cache info
> > about the LWP a particular user-level thread is bound to if you'd
> > invalidate this info upon receiving those events (which should never
> > happen in a 1-to-1 threads implementation.
> >
> > That said, would re-enabling TD_DEATH events somehow make things more
> > robust for you?  TD_DEATH was broken in glibc 2.1.3, but anybody who's
> > doing any serious threads development should be using a more recent
> > glibc.
> 
> I don't think that TD_DEATH would solve the problem.  Let me check when
> they're actually delivered... no, TD_DEATH is reported immediately
> before the terminated flag is set, i.e. the thread becomes a "zombie".
> The problem is that we need to continue debugging the thread until it's
> actually _gone_.  Hmm, enabling TD_DEATH might be useful in that it
> would simplify the ad-hoc-ness of the zombie handling; it could prevent
> having to iterate over all LWPs, which would be nice.  I'll thik about
> it.

If we think that the TD_DEATH event is coming at the wrong time, 
we can certainly ask the glibc maintainers to change it.
GDB is the only consumer so far...

> 
> Now, relying on SWITCHTO/SWITCHFROM has some potential.  The difficulty
> is that using them would slow down debugging of a M-to-N implementation
> substantially, I wager.  But we could at least do something like "assume
> 1-to-1 for caching purposes until we get a SWITCHTO/SWITCHFROM".  Hmm,
> I need to think about that too.

Insider info -- glibc is extremely unlikely to ever switch
to an m-by-n model.  I've talked with Uli Drepper, and although
he once planned on doing that, he now believes that there is no
possible gain to be had from it, and that it's an essentially
faulty approach.

> 
> Is there formal documentation for the libthread_db interface somewhere?
> Glibc certainly doesn't have any. 

Log into a solaris box and do "man libthread_db".

> For instance, I'd like to know if I
> can safely cache the thread handles across resumes; 

Even if that happened to be safe now, it would be an
extremely bad assumption that it would continue to be
safe.

> if I could, this
> would be much much much much easier to do efficiently.  We could get
> the thread handle and LWP when the thread is created, and then hold the
> thread handle, and optionally hold the LWP.  I am pretty sure this is
> safe given glibc, but I don't know in general.

I think in general not.

Michael

PS: Do ask me questions like this -- I've had extensive talks with
Uli about libthread_db.  He and I basically implemented it together
(him on the glibc side, me on the gdb side).

Re: RFA [threads]: Thread cache

[Daniel Jacobowitz]

On Mon, Jan 13, 2003 at 04:04:20PM -0800, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> > 
> > On Sat, Jan 11, 2003 at 01:13:55PM +0100, Mark Kettenis wrote:
> > > Daniel Jacobowitz <drow@mvista.com> writes:
> > >
> > > > I've figured out how to fix print-threads.exp (see my ramblings on gdb@
> > > > yesterday for a bad description of the problem; better coming soon).
> > > > However, to do it, I discovered that it was actually _required_ that we
> > > > cache certain information from libthread_db, instead of merely beneficial.
> > > >
> > > > So I implemented the cache.  This patch is the entire cache mechanism,
> > > > except for updating the comment at the top of the file saying we need one.
> > > > Before I get to the patch itself, some numbers:
> > >
> > > This looks good!  Please check it in, regardless of the things I say
> > > further on in this message.
> > >
> > > > Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
> > > > and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
> > > > convert the others too; I will do that in a separate patch if this one is
> > > > approved, and see how much more it takes off the runtime] with calls to
> > > > wrapper functions which cache the data in the struct private_thread_info.
> > > > The cache is invalidated at every resume(); there's some information that we
> > > > could keep if we are guaranteed a 1-to-1 threads implementation with no
> > > > migration, like LinuxThreads or NPTL, but I'm being conservative for now.
> > >
> > > Note that the thread_db.h interface provides TD_SWITCHTO and
> > > TD_SWITCHFROM events.  I'd be perfectly happy if you'd cache info
> > > about the LWP a particular user-level thread is bound to if you'd
> > > invalidate this info upon receiving those events (which should never
> > > happen in a 1-to-1 threads implementation.
> > >
> > > That said, would re-enabling TD_DEATH events somehow make things more
> > > robust for you?  TD_DEATH was broken in glibc 2.1.3, but anybody who's
> > > doing any serious threads development should be using a more recent
> > > glibc.
> > 
> > I don't think that TD_DEATH would solve the problem.  Let me check when
> > they're actually delivered... no, TD_DEATH is reported immediately
> > before the terminated flag is set, i.e. the thread becomes a "zombie".
> > The problem is that we need to continue debugging the thread until it's
> > actually _gone_.  Hmm, enabling TD_DEATH might be useful in that it
> > would simplify the ad-hoc-ness of the zombie handling; it could prevent
> > having to iterate over all LWPs, which would be nice.  I'll thik about
> > it.
> 
> If we think that the TD_DEATH event is coming at the wrong time, 
> we can certainly ask the glibc maintainers to change it.
> GDB is the only consumer so far...

We could probably use TD_REAP for some of this also.

> > Now, relying on SWITCHTO/SWITCHFROM has some potential.  The difficulty
> > is that using them would slow down debugging of a M-to-N implementation
> > substantially, I wager.  But we could at least do something like "assume
> > 1-to-1 for caching purposes until we get a SWITCHTO/SWITCHFROM".  Hmm,
> > I need to think about that too.
> 
> Insider info -- glibc is extremely unlikely to ever switch
> to an m-by-n model.  I've talked with Uli Drepper, and although
> he once planned on doing that, he now believes that there is no
> possible gain to be had from it, and that it's an essentially
> faulty approach.

Yep.  However, as much as I may personally avoid it, NGPT has some
current backing; it works with our current thread-db code, more or
less.  (I'm going on hearsay here; I haven't tested it so I don't know
how good the support is.)  Do we want it to keep working?

> > Is there formal documentation for the libthread_db interface somewhere?
> > Glibc certainly doesn't have any. 
> 
> Log into a solaris box and do "man libthread_db".

Ah, that's something at least.

> > For instance, I'd like to know if I
> > can safely cache the thread handles across resumes; 
> 
> Even if that happened to be safe now, it would be an
> extremely bad assumption that it would continue to be
> safe.
> 
> > if I could, this
> > would be much much much much easier to do efficiently.  We could get
> > the thread handle and LWP when the thread is created, and then hold the
> > thread handle, and optionally hold the LWP.  I am pretty sure this is
> > safe given glibc, but I don't know in general.
> 
> I think in general not.

Hmm.  The Solaris documentation suggests that this is valid; I have no
way to check whether it actually is, and there is no explicit
description of the lifetime of a thread handle, but it doesn't describe
them as being of limited life.  It's a handle to "the thread object"
itself.

Do you think that we could arrange for this assumption to continue
holding?  I think it will in the glibc implementation, for a while at
least, because just the thread agent and pthread_descr are used as the
thread handle; the pthread_descr can't change in the lifetime of a
thread.

> 
> Michael
> 
> PS: Do ask me questions like this -- I've had extensive talks with
> Uli about libthread_db.  He and I basically implemented it together
> (him on the glibc side, me on the gdb side).

Aha, perfect!

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA [threads]: Thread cache

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Mon, Jan 13, 2003 at 04:04:20PM -0800, Michael Snyder wrote:
> > Daniel Jacobowitz wrote:

> > > For instance, I'd like to know if I
> > > can safely cache the thread handles across resumes; if I could, this
> > > would be much much much much easier to do efficiently.  We could get
> > > the thread handle and LWP when the thread is created, and then hold the
> > > thread handle, and optionally hold the LWP.  I am pretty sure this is
> > > safe given glibc, but I don't know in general.
> >
> > I think in general not.
> 
> Hmm.  The Solaris documentation suggests that this is valid; I have no
> way to check whether it actually is, and there is no explicit
> description of the lifetime of a thread handle, but it doesn't describe
> them as being of limited life.  It's a handle to "the thread object"
> itself.

Dan, 

I passed your question along to Ulrich Drepper, and he says that, 
if by "thread handle" you mean the th_unique value, then yes, 
those are persistant until the thread exits.  If you mean the
td_thrhandle_t value, though, then no, they are not persistant.
They are allocated by libthread-db as needed, then thrown away.