Re: performance of multithreading gets gradually worse under gdb

Re: performance of multithreading gets gradually worse under gdb

[Pedro Alves]

[Adding gdb-patches@]

On Friday 04 February 2011 15:25:54, Tom Tromey wrote:
> Pedro> Yeah.  And only clear current_regcache_ptid if it was deleted in the
> Pedro> first place; and only reinit the frame cache if we deleted the
> Pedro> regcache of inferior_ptid ?  Like the patch below.
> 
> Yeah, this looks reasonable to me.

Okay, thanks, I've applied it.

> One somewhat distressing thing is that my patch did not cause any
> regressions.

Indeed.

-- 
Pedro Alves

Re: performance of multithreading gets gradually worse under gdb

[Pedro Alves]

On Friday 04 February 2011 15:55:51, Pedro Alves wrote:
> [Adding gdb-patches@]

...

> Okay, thanks, I've applied it.

"it" being...

-- 
Pedro Alves

2011-02-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* regcache.c (registers_changed_ptid): Don't explictly always
	clear `current_regcache'.  Only clear current_thread_ptid and
	current_thread_arch when PTID matches.  Only reinit the frame
	cache if PTID matches the current inferior_ptid.  Move alloca(0)
	call to ...
	(registers_changed): ... here.

---
 gdb/regcache.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Index: src/gdb/regcache.c
===================================================================
--- src.orig/gdb/regcache.c	2011-02-01 15:27:37.000000000 +0000
+++ src/gdb/regcache.c	2011-02-04 11:56:49.273328004 +0000
@@ -530,6 +530,7 @@ void
 registers_changed_ptid (ptid_t ptid)
 {
   struct regcache_list *list, **list_link;
+  int wildcard = ptid_equal (ptid, minus_one_ptid);
 
   list = current_regcache;
   list_link = &current_regcache;
@@ -550,13 +551,24 @@ registers_changed_ptid (ptid_t ptid)
       list = *list_link;
     }
 
-  current_regcache = NULL;
+  if (wildcard || ptid_equal (ptid, current_thread_ptid))
+    {
+      current_thread_ptid = null_ptid;
+      current_thread_arch = NULL;
+    }
 
-  current_thread_ptid = null_ptid;
-  current_thread_arch = NULL;
+  if (wildcard || ptid_equal (ptid, inferior_ptid))
+    {
+      /* We just deleted the regcache of the current thread.  Need to
+	 forget about any frames we have cached, too.  */
+      reinit_frame_cache ();
+    }
+}
 
-  /* Need to forget about any frames we have cached, too.  */
-  reinit_frame_cache ();
+void
+registers_changed (void)
+{
+  registers_changed_ptid (minus_one_ptid);
 
   /* Force cleanup of any alloca areas if using C alloca instead of
      a builtin alloca.  This particular call is used to clean up
@@ -567,12 +579,6 @@ registers_changed_ptid (ptid_t ptid)
 }
 
 void
-registers_changed (void)
-{
-  registers_changed_ptid (minus_one_ptid);
-}
-
-void
 regcache_raw_read (struct regcache *regcache, int regnum, gdb_byte *buf)
 {
   gdb_assert (regcache != NULL && buf != NULL);

[patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Ulrich Weigand]

Pedro Alves wrote:

> 2011-02-04  Pedro Alves  <pedro@codesourcery.com>
> 
> 	gdb/
> 	* regcache.c (registers_changed_ptid): Don't explictly always
> 	clear `current_regcache'.  Only clear current_thread_ptid and
> 	current_thread_arch when PTID matches.  Only reinit the frame
> 	cache if PTID matches the current inferior_ptid.  Move alloca(0)
> 	call to ...
> 	(registers_changed): ... here.

It seems this patch broke multi-architecture debugging on Cell/B.E. since
the cached thread architecture is sometimes not invalidated when it should
be.  This happens when resume is called with a PTID that refers to all
threads of a process.

The problem is that your patch specifically handles PTIDs that refer to
one particular thread, and also handles a wildcard PTID that refers to
all threads of all processes.  But it does not handle PTIDs that refer
to all threads of one single process.

I think the code should simply use ptid_match here (as is already done
elsewhere, even in this same function).  This fixes the problem on
Cell/B.E. for me ...

Tested on powerpc-linux (Cell/B.E.).
Does this look OK to you?

Bye,
Ulrich


ChangeLog:

	* regcache.c (registers_changed_ptid): Invalidate thread architecture
	and frame caches if PTID refers to all threads of a process.


Index: gdb/regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.194
diff -u -p -r1.194 regcache.c
--- gdb/regcache.c	11 Oct 2011 18:35:25 -0000	1.194
+++ gdb/regcache.c	24 Oct 2011 11:02:46 -0000
@@ -548,7 +548,6 @@ void
 registers_changed_ptid (ptid_t ptid)
 {
   struct regcache_list *list, **list_link;
-  int wildcard = ptid_equal (ptid, minus_one_ptid);
 
   list = current_regcache;
   list_link = &current_regcache;
@@ -569,13 +568,13 @@ registers_changed_ptid (ptid_t ptid)
       list = *list_link;
     }
 
-  if (wildcard || ptid_equal (ptid, current_thread_ptid))
+  if (ptid_match (current_thread_ptid, ptid))
     {
       current_thread_ptid = null_ptid;
       current_thread_arch = NULL;
     }
 
-  if (wildcard || ptid_equal (ptid, inferior_ptid))
+  if (ptid_match (inferior_ptid, ptid))
     {
       /* We just deleted the regcache of the current thread.  Need to
 	 forget about any frames we have cached, too.  */

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Pedro Alves]

On Wednesday 26 October 2011 18:14:11, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > 2011-02-04  Pedro Alves  <pedro@codesourcery.com>
> > 
> > 	gdb/
> > 	* regcache.c (registers_changed_ptid): Don't explictly always
> > 	clear `current_regcache'.  Only clear current_thread_ptid and
> > 	current_thread_arch when PTID matches.  Only reinit the frame
> > 	cache if PTID matches the current inferior_ptid.  Move alloca(0)
> > 	call to ...
> > 	(registers_changed): ... here.
> 
> It seems this patch broke multi-architecture debugging on Cell/B.E. since
> the cached thread architecture is sometimes not invalidated when it should
> be.  This happens when resume is called with a PTID that refers to all
> threads of a process.
> 
> The problem is that your patch specifically handles PTIDs that refer to
> one particular thread, and also handles a wildcard PTID that refers to
> all threads of all processes.  But it does not handle PTIDs that refer
> to all threads of one single process.
> 
> I think the code should simply use ptid_match here (as is already done
> elsewhere, even in this same function).  This fixes the problem on
> Cell/B.E. for me ...
> 
> Tested on powerpc-linux (Cell/B.E.).
> Does this look OK to you?

Yes, thanks.

-- 
Pedro Alves

Re: [patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Ulrich Weigand]

Pedro Alves wrote:
> On Wednesday 26 October 2011 18:14:11, Ulrich Weigand wrote:
> > I think the code should simply use ptid_match here (as is already done
> > elsewhere, even in this same function).  This fixes the problem on
> > Cell/B.E. for me ...
> > 
> > Tested on powerpc-linux (Cell/B.E.).
> > Does this look OK to you?
> 
> Yes, thanks.

OK, checked in.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: performance of multithreading gets gradually worse under gdb

[Tom Tromey]

Tom> Yeah, this looks reasonable to me.

Pedro> Okay, thanks, I've applied it.

Thanks

Markus -- any way you could try gdb CVS HEAD to see if it fixes your
problem?  If it doesn't, then there are more bugs to find...

Tom

Re: performance of multithreading gets gradually worse under gdb

[Markus Alber]

 On Fri, 04 Feb 2011 10:01:57 -0700, Tom Tromey wrote:

> Markus -- any way you could try gdb CVS HEAD to see if it fixes your
> problem?  If it doesn't, then there are more bugs to find...
>
> Tom

 Will try on Monday, thanks.

Re: performance of multithreading gets gradually worse under gdb

[Markus Alber]

 On Fri, 04 Feb 2011 10:01:57 -0700, Tom Tromey wrote:
> Tom> Yeah, this looks reasonable to me.
>
> Pedro> Okay, thanks, I've applied it.
>
> Thanks
>
> Markus -- any way you could try gdb CVS HEAD to see if it fixes your
> problem?  If it doesn't, then there are more bugs to find...
>
> Tom

 Hi, I did as told and it does indeed make the situation a lot better.
 The application runs to the end (for the first time!) and it does not
 slow down.

 Thanks for that!

 The application is a lot slower though when run in gdb, than 
 stand-alone.
 I think the reason is this. During each iteration, it branches into the
 worker threads and then comes back to some part of the computation 
 which
 is performed only by one CPU. With 12 cores, the time per iteration is
 essentially spent in there. It is here that gdb competes with the 
 application,
 because both seem to be running on the same core. The average load 
 drops from
 66% to about 20% when run in gdb.

 Just for your information/in case you are interested.

 I did an oprofile as well, and here is what it says:

 samples  %        image name               app name                 
 symbol name
 1113     19.3464  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_locate_field
 1054     18.3209  libthread_db-1.0.so      libthread_db-1.0.so      
 td_thr_get_info
 847      14.7228  libthread_db-1.0.so      libthread_db-1.0.so      
 td_ta_event_getmsg
 804      13.9753  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_fetch_value_local
 568       9.8731  libthread_db-1.0.so      libthread_db-1.0.so      
 td_ta_map_lwp2thr
 519       9.0214  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_fetch_value
 484       8.4130  libthread_db-1.0.so      libthread_db-1.0.so      
 __td_ta_lookup_th_unique
 169       2.9376  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_store_value
 167       2.9028  libthread_db-1.0.so      libthread_db-1.0.so      
 td_thr_event_enable
 3         0.0521  iconv                    iconv                    
 /usr/bin/iconv
 3         0.0521  libstreamanalyzer.so.0.7.2 libstreamanalyzer.so.0.7.2 
 /usr/lib64/libstreamanalyzer.so.0.7.2
 2         0.0348  [vdso] (tgid:2539 
 range:0x7fffe81ff000-0x7fffe8200000) auditd                   [vdso] 
 (tgid:2539 range:0x7fffe81
 200000)
 2         0.0348  libpcre.so.0.0.1         libpcre.so.0.0.1         
 /lib64/libpcre.so.0.0.1
 2         0.0348  ophelp                   ophelp                   
 /usr/bin/ophelp
 1         0.0174  [vdso] (tgid:7680 
 range:0x7fff267c6000-0x7fff267c7000) kdeinit4                 [vdso] 
 (tgid:7680 range:0x7fff267
 7c7000)
 1         0.0174  [vdso] (tgid:8945 
 range:0x7fffbbbff000-0x7fffbbc00000) gconfd-2                 [vdso] 
 (tgid:8945 range:0x7fffbbb
 c00000)
 1         0.0174  auditd                   auditd                   
 /sbin/auditd
 1         0.0174  basename                 basename                 
 /bin/basename
 1         0.0174  kded_networkstatus.so    kded_networkstatus.so    
 /usr/lib64/kde4/kded_networkstatus.so
 1         0.0174  libXau.so.6.0.0          libXau.so.6.0.0          
 /usr/lib64/libXau.so.6.0.0
 1         0.0174  libXi.so.6.1.0           libXi.so.6.1.0           
 /usr/lib64/libXi.so.6.1.0
 1         0.0174  libkdeinit4_kglobalaccel.so 
 libkdeinit4_kglobalaccel.so /usr/lib64/libkdeinit4_kglobalaccel.so
 1         0.0174  libstreams.so.0.7.2      libstreams.so.0.7.2      
 /usr/lib64/libstreams.so.0.7.2
 1         0.0174  libthread_db-1.0.so      libthread_db-1.0.so      
 __do_global_ctors_aux
 1         0.0174  pickup                   pickup                   
 /usr/lib/postfix/pickup
 1         0.0174  plasma_applet_icon.so    plasma_applet_icon.so    
 /usr/lib64/kde4/plasma_applet_icon.so
 1         0.0174  qmgr                     qmgr                     
 /usr/lib/postfix/qmgr
 1         0.0174  rpcbind                  rpcbind                  
 /sbin/rpcbind
 1         0.0174  sleep                    sleep                    
 /bin/sleep
 1         0.0174  touch                    touch                    
 /bin/touch

 If you want to have it in more detail, please tell me what options 
 you'd like to have engaged.

 regards,

 Markus



 

Re: performance of multithreading gets gradually worse under gdb

[Tom Tromey]

Moving to gdb-patches.

Ulrich> Yes, I think so.  Note that we still need to (potentially) keep
Ulrich> more than one regcache per thread for multi-arch support.  We
Ulrich> may also need to reset the regcache's address space before
Ulrich> reusing it ...

In that case it seemed simpler to free the caches.

Here is what I am testing.  Let me know what you think.

Tom

2011-02-04  Tom Tromey  <tromey@redhat.com>

	* thread.c (free_thread): Call free_thread_regcache.
	* regcache.h (free_thread_regcache): Declare.
	* regcache.c (current_regcache): Remove.
	(get_thread_arch_regcache): Use thread's regcache.
	(free_thread_regcache): New function.
	(regcache_thread_ptid_changed): Use thread's regcache.
	(invalidate_registers_maybe): New function.
	(registers_changed_ptid): Use iterate_over_threads and
	invalidate_registers_maybe.
	* gdbthread.h (struct thread_info) <regcache>: New field.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index ddb7b0f..cfe1bea 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -23,6 +23,7 @@
 #define GDBTHREAD_H
 
 struct symtab;
+struct regcache_list;
 
 #include "breakpoint.h"
 #include "frame.h"
@@ -224,6 +225,10 @@ struct thread_info
   /* Function that is called to free PRIVATE.  If this is NULL, then
      xfree will be called on PRIVATE.  */
   void (*private_dtor) (struct private_thread_info *);
+
+  /* The register caches associated with this thread.  Note that this
+     type is opaque; it is entirely managed by the regcache code.  */
+  struct regcache_list *regcache;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 53e0c59..55f39fe 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -29,6 +29,7 @@
 #include "gdb_string.h"
 #include "gdbcmd.h"		/* For maintenanceprintlist.  */
 #include "observer.h"
+#include "gdbthread.h"
 
 /*
  * DATA STRUCTURE
@@ -433,9 +434,6 @@ regcache_invalidate (struct regcache *regcache, int regnum)
   regcache->register_status[regnum] = REG_UNKNOWN;
 }
 
-
-/* Global structure containing the current regcache.  */
-
 /* NOTE: this is a write-through cache.  There is no "dirty" bit for
    recording if the register values have been changed (eg. by the
    user).  Therefore all registers must be written back to the
@@ -447,17 +445,15 @@ struct regcache_list
   struct regcache_list *next;
 };
 
-static struct regcache_list *current_regcache;
-
 struct regcache *
 get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
 {
   struct regcache_list *list;
   struct regcache *new_regcache;
+  struct thread_info *tp = find_thread_ptid (ptid);
 
-  for (list = current_regcache; list; list = list->next)
-    if (ptid_equal (list->regcache->ptid, ptid)
-	&& get_regcache_arch (list->regcache) == gdbarch)
+  for (list = tp->regcache; list; list = list->next)
+    if (get_regcache_arch (list->regcache) == gdbarch)
       return list->regcache;
 
   new_regcache = regcache_xmalloc_1 (gdbarch,
@@ -467,8 +463,8 @@ get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
 
   list = xmalloc (sizeof (struct regcache_list));
   list->regcache = new_regcache;
-  list->next = current_regcache;
-  current_regcache = list;
+  list->next = tp->regcache;
+  tp->regcache = list;
 
   return new_regcache;
 }
@@ -494,6 +490,21 @@ get_current_regcache (void)
   return get_thread_regcache (inferior_ptid);
 }
 
+void
+free_thread_regcache (struct thread_info *tp)
+{
+  struct regcache_list *iter, *next;
+
+  for (iter = tp->regcache; iter; iter = next)
+    {
+      next = iter->next;
+      regcache_xfree (iter->regcache);
+      xfree (iter);
+    }
+
+  tp->regcache = NULL;
+}
+
 
 /* Observer for the target_changed event.  */
 
@@ -508,11 +519,14 @@ regcache_observer_target_changed (struct target_ops *target)
 static void
 regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
 {
+  struct thread_info *tp = find_thread_ptid (new_ptid);
   struct regcache_list *list;
 
-  for (list = current_regcache; list; list = list->next)
-    if (ptid_equal (list->regcache->ptid, old_ptid))
+  for (list = tp->regcache; list; list = list->next)
+    {
+      gdb_assert (ptid_equal (list->regcache->ptid, old_ptid));
       list->regcache->ptid = new_ptid;
+    }
 }
 
 /* Low level examining and depositing of registers.
@@ -522,6 +536,21 @@ regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
    garbage.  (a change from GDB version 3, in which the caller got the
    value from the last stop).  */
 
+/* Helper for registers_changed_ptid.  This is used as a callback to
+   iterate_over_threads.  */
+
+static int
+invalidate_registers_maybe (struct thread_info *tp, void *data)
+{
+  ptid_t *filter = data;
+
+  if (ptid_match (tp->ptid, *filter))
+    free_thread_regcache (tp);
+
+  /* Keep going.  */
+  return 0;
+}
+
 /* REGISTERS_CHANGED ()
 
    Indicate that registers may have changed, so invalidate the cache.  */
@@ -529,28 +558,7 @@ regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
 void
 registers_changed_ptid (ptid_t ptid)
 {
-  struct regcache_list *list, **list_link;
-
-  list = current_regcache;
-  list_link = &current_regcache;
-  while (list)
-    {
-      if (ptid_match (list->regcache->ptid, ptid))
-	{
-	  struct regcache_list *dead = list;
-
-	  *list_link = list->next;
-	  regcache_xfree (list->regcache);
-	  list = *list_link;
-	  xfree (dead);
-	  continue;
-	}
-
-      list_link = &list->next;
-      list = *list_link;
-    }
-
-  current_regcache = NULL;
+  iterate_over_threads (invalidate_registers_maybe, &ptid);
 
   current_thread_ptid = null_ptid;
   current_thread_arch = NULL;
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 7ae585a..be71f90 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -180,4 +180,8 @@ extern void regcache_cpy_no_passthrough (struct regcache *dest,
 extern void registers_changed (void);
 extern void registers_changed_ptid (ptid_t);
 
+/* Free the regcaches associated with the thread TP.  */
+
+extern void free_thread_regcache (struct thread_info *tp);
+
 #endif /* REGCACHE_H */
diff --git a/gdb/thread.c b/gdb/thread.c
index 62455c2..6717dd4 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -141,6 +141,8 @@ free_thread (struct thread_info *tp)
 	xfree (tp->private);
     }
 
+  free_thread_regcache (tp);
+
   xfree (tp->name);
   xfree (tp);
 }

Re: performance of multithreading gets gradually worse under gdb

[Ulrich Weigand]

Tom Tromey wrote:

> 	* thread.c (free_thread): Call free_thread_regcache.
> 	* regcache.h (free_thread_regcache): Declare.
> 	* regcache.c (current_regcache): Remove.
> 	(get_thread_arch_regcache): Use thread's regcache.
> 	(free_thread_regcache): New function.
> 	(regcache_thread_ptid_changed): Use thread's regcache.
> 	(invalidate_registers_maybe): New function.
> 	(registers_changed_ptid): Use iterate_over_threads and
> 	invalidate_registers_maybe.
> 	* gdbthread.h (struct thread_info) <regcache>: New field.

Looks good to me ...

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: performance of multithreading gets gradually worse under gdb

[Pedro Alves]

On Friday 04 February 2011 14:58:34, Ulrich Weigand wrote:
> Tom Tromey wrote:
> 
> > 	* thread.c (free_thread): Call free_thread_regcache.
> > 	* regcache.h (free_thread_regcache): Declare.
> > 	* regcache.c (current_regcache): Remove.
> > 	(get_thread_arch_regcache): Use thread's regcache.
> > 	(free_thread_regcache): New function.
> > 	(regcache_thread_ptid_changed): Use thread's regcache.
> > 	(invalidate_registers_maybe): New function.
> > 	(registers_changed_ptid): Use iterate_over_threads and
> > 	invalidate_registers_maybe.
> > 	* gdbthread.h (struct thread_info) <regcache>: New field.
> 
> Looks good to me ...

Please see my email on gdb@ before proceeding with this.

-- 
Pedro Alves