public inbox for libc-hacker@sourceware.org
 help / color / mirror / Atom feed
* second thoughts on using dl_iterate_phdr() for cache-validation
@ 2004-03-31  8:53 David Mosberger
  2004-03-31 16:04 ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-03-31  8:53 UTC (permalink / raw)
  To: libc-hacker; +Cc: davidm

Doing some more careful performance analysis with libunwind, I'm
finding that the dl_iterate_phdr() call needed to verify that the
phdr-list didn't change is rather expensive.  Specifically, the time
needed to initialize an "unwind cursor" (via unw_init_local()) is as
follows:

  without dl_iterate_phdr() callback (unsafe):	140 ns
  with dl_iterate_phdr(), without -lpthread:	200 ns
  with dl_iterate_phdr(), with -lpthread:	300 ns

This is rather more than I expected and a slow-down of more than a
factor of 2 for multi-threaded apps is bit more than I'm willing to
bear since it could really affect the usability of libunwind for
things such as allocation tracking or stack-trace-based sampling.

The profile for the "without -lpthread" case looks like this:

% time   self  cumul  calls self/call tot/call name
 60.44  13.05  13.05   101M      129n     204n _ULia64_init_local
 17.19   3.71  16.76  99.6M     37.3n    66.7n dl_iterate_phdr
  4.65   1.00  17.76   101M     9.97n    9.97n rtld_lock_default_lock_recursive
  4.64   1.00  18.77  99.0M     10.1n    10.1n rtld_lock_default_unlock_recursive

The profile for the "with -lpthread" case looks like this (this was
measured on a different machine, so the total time of 223 ns is not
comparable to the 300 ns mentioned above; the relative times are fine,
though):

% time   self  cumul  calls self/call tot/call name
 47.93  11.25  11.25  99.6M      113n     223n _ULia64_init_local
 18.35   4.31  15.56   100M     43.0n    43.0n pthread_mutex_lock
 11.81   2.77  18.33   100M     27.7n    27.7n __pthread_mutex_unlock_usercnt
 11.65   2.73  21.06   100M     27.3n     103n __GI___dl_iterate_phdr

For brevity, I didn't include the call-graphs, but they are pretty
easy: all calls to dl_iterate_phdr() are indirectly due to the
cache-validation done by _ULia64_init_local() and almost all
lock-related calls are due to dl_iterate_phdr().

I suppose I could add a libunwind-hack to disable cache-validation,
but that seems like a step backward since it would make caching unsafe
again.

In case it matters, the first profile was obtained with libc v2.3.2
and the second profile was obtained with the CVS libc (as of a few
days ago).

Can this be improved?

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-03-31  8:53 second thoughts on using dl_iterate_phdr() for cache-validation David Mosberger
@ 2004-03-31 16:04 ` Roland McGrath
  2004-04-01  2:33   ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-03-31 16:04 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

To avoid that locking overhead you need to do your short-circuit case
without taking any locks.  I presume you don't care about races producing
false short circuits, since addresses you care about shouldn't be coming or
going while you're unwinding.  The only thing I see is to give you a way to
see _dl_load_adds without calling __dl_iterate_phdr and taking locks, which
is what you asked for in the first place.  We really can't give you an
exported variable symbol, so it would have to be a new function interface.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-03-31 16:04 ` Roland McGrath
@ 2004-04-01  2:33   ` David Mosberger
  2004-11-04 23:41     ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-04-01  2:33 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Wed, 31 Mar 2004 00:04:25 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> The only thing I see is to give you a way to see
  Roland> _dl_load_adds without calling __dl_iterate_phdr and taking
  Roland> locks, which is what you asked for in the first place.  We
  Roland> really can't give you an exported variable symbol, so it
  Roland> would have to be a new function interface.

How about something along the lines of this:

-----------
/* The value returned by this function increments by at least 1 when
   objects are added to the list visited by dl_iterate_phdr().  This
   function is not serialized with respect to dl_iterate_phdr().  If
   serialization is needed, this function should be called from within
   a dl_itereate_phdr() callback.  */
extern unsigned long long dl_phdr_additions_counter (void);

/* The vaue returned by this function increments by at least 1 when
   objects are removed from the list visited by dl_iterate_phdr().
   This function is not serialized with respect to dl_iterate_phdr().
   If serialization is needed, this function should be called from
   within a dl_itereate_phdr() callback.  */
extern unsigned long long dl_phdr_removals_counter (void);
-----------

I considered using a single function with an argument which specifies
the counter to read, but then I thought that many programs may only
read one or the other counter.  Plus if there is two counters, someone
might want to add a third counter in the future and if that needs to
be supported, there would have to be a way for detecting whether a
libc supports the new counter, etc., so I thought having two separate
routines might not be such a bad idea after all.

A preliminary but complete patch is below and it dropped the cost of
unw_init_local() from 223 to 119ns (on the faster machine I profiled
yesterday).  The code of unw_init_local() itself accounts for 113ns
and the overhead of 6ns is about in line with a shared library call.
This performance-level seems very acceptable to me.  Note that the way
things work in libunwind, I don't have to worry about concurrent
dl_close() operations and hence it is safe to read the counter without
locking.

Some questions/comments:

 - I always get confused about the versioning stuff in glibc.
   I added the new functions to elf/Versions, but am not sure
   this is really correct.

 - I wasn't sure what the right place for the new functions is.
   For now, I put them into dl-iteratephdr.c which is probably
   a bad choice.  Furthermore, I'm not sure about name-space
   pollution issues.  Advice on the naming etc. would be welcome.

 - For now, I didn't remove the dl_adds/dl_subs members from the
   dl_iterate_phdr() callback.  With the new interface, they won't be
   needed anymore and I suspect nobody other than libunwind would be
   affected if we removed them (and even old libunwind versions would
   continue work properly).  If you agree, I'll cook up a second patch
   to remove this stuff once the new interface is in.

Thanks,

	--david

Index: elf/Versions
===================================================================
RCS file: /cvs/glibc/libc/elf/Versions,v
retrieving revision 1.51
diff -u -r1.51 Versions
--- elf/Versions	6 Mar 2004 08:04:12 -0000	1.51
+++ elf/Versions	1 Apr 2004 02:18:54 -0000
@@ -11,6 +11,10 @@
   GLIBC_2.2.4 {
     dl_iterate_phdr;
   }
+  GLIBC_2.3.2 {
+    dl_phdr_additions_counter;
+    dl_phdr_removals_counter;
+  }
 %ifdef EXPORT_UNWIND_FIND_FDE
   GCC_3.0 {
     __register_frame_info_bases; __deregister_frame_info_bases;
Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.103
diff -u -r1.103 dl-close.c
--- elf/dl-close.c	7 Mar 2004 08:38:43 -0000	1.103
+++ elf/dl-close.c	1 Apr 2004 02:18:55 -0000
@@ -319,6 +319,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.11
diff -u -r1.11 dl-iteratephdr.c
--- elf/dl-iteratephdr.c	27 Jan 2004 21:29:22 -0000	1.11
+++ elf/dl-iteratephdr.c	1 Apr 2004 02:18:55 -0000
@@ -23,6 +23,19 @@
 #include <stddef.h>
 #include <bits/libc-lock.h>
 
+
+unsigned long long
+dl_phdr_additions_counter (void)
+{
+  return GL(dl_load_adds);
+}
+
+unsigned long long
+dl_phdr_removals_counter (void)
+{
+  return GL(dl_load_subs);
+}
+
 static void
 cancel_handler (void *arg __attribute__((unused)))
 {
Index: elf/dl-support.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-support.c,v
retrieving revision 1.83
diff -u -r1.83 dl-support.c
--- elf/dl-support.c	6 Mar 2004 09:06:15 -0000	1.83
+++ elf/dl-support.c	1 Apr 2004 02:18:55 -0000
@@ -75,6 +75,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/tst-dlmodcount.c
===================================================================
RCS file: /cvs/glibc/libc/elf/tst-dlmodcount.c,v
retrieving revision 1.3
diff -u -r1.3 tst-dlmodcount.c
--- elf/tst-dlmodcount.c	26 Mar 2004 09:48:53 -0000	1.3
+++ elf/tst-dlmodcount.c	1 Apr 2004 02:18:55 -0000
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_counter (), dl_phdr_removals_counter ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_counter (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_counter (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_counter ();
+  last_subs = dl_phdr_removals_counter ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
===================================================================
RCS file: /cvs/glibc/libc/include/link.h,v
retrieving revision 1.31
diff -u -r1.31 link.h
--- include/link.h	27 Mar 2004 03:22:29 -0000	1.31
+++ include/link.h	1 Apr 2004 02:18:55 -0000
@@ -307,4 +307,18 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1 when
+   objects are added to the list visited by dl_iterate_phdr().  This
+   function is not serialized with respect to dl_iterate_phdr().  If
+   serialization is needed, this function should be called from within
+   a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_additions_counter (void);
+
+/* The vaue returned by this function increments by at least 1 when
+   objects are removed from the list visited by dl_iterate_phdr().
+   This function is not serialized with respect to dl_iterate_phdr().
+   If serialization is needed, this function should be called from
+   within a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_removals_counter (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.100
diff -u -r1.100 ldsodefs.h
--- sysdeps/generic/ldsodefs.h	27 Mar 2004 03:23:51 -0000	1.100
+++ sysdeps/generic/ldsodefs.h	1 Apr 2004 02:18:59 -0000
@@ -241,6 +241,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-04-01  2:33   ` David Mosberger
@ 2004-11-04 23:41     ` David Mosberger
  2004-11-04 23:53       ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-04 23:41 UTC (permalink / raw)
  To: Roland McGrath, libc-hacker; +Cc: davidm

This is a follow-up on a discussion that went dead around March 31.
There were no objections to the patch I presented then and if that's
still the case, I'd appreciate if this patch could be checked in.  It
speeds up dynamic-object cache-validition a great deal (~ a factor of
2, mainly because dl_iterate_phdr() requires taking an expensive
lock).

BTW: i assume an interface along the lines of:

 enum dl_counter {
   DL_PHDR_ADDITIONS,
   DL_PHDR_REMOVALS
 };

 unsigned long long *dl_get_counter_address (enum dl_counter which_counter);

is out of the question.  If not, this would be even better, since the
cache-validation overhead would drop to a simple load & compare.

Thanks,

	--david

---
ChangeLog

2004-11-04  David Mosberger  <davidm@hpl.hp.com>

	* elf/Versions (dl_phdr_additions_count): New export
	(dl_phdr_removals_count): Likewise.
	* elf/dl-close.c (_dl_close): Increment dl_load_subs;
	* elf/dl-iteratephdr.c (dl_phdr_additions_count): New function.
	(dl_phdr_removals_count): Likewise.
	* elf/dl-support.c: New variable.
	* elf/tst-dlmodcount.c: New file.
	* include/link.h (dl_phdr_additions_count): Declare.
	(dl_phdr_removals_count): Likewise.
	* sysdeps/generic/ldsodefs.h (struct rtld_global): Add member
	  "_dl_load_subs".

Index: elf/Versions
===================================================================
RCS file: /cvs/glibc/libc/elf/Versions,v
retrieving revision 1.52
diff -u -r1.52 Versions
--- elf/Versions	19 Oct 2004 22:20:33 -0000	1.52
+++ elf/Versions	4 Nov 2004 23:38:29 -0000
@@ -11,6 +11,10 @@
   GLIBC_2.2.4 {
     dl_iterate_phdr;
   }
+  GLIBC_2.3.2 {
+    dl_phdr_additions_count;
+    dl_phdr_removals_count;
+  }
 %ifdef EXPORT_UNWIND_FIND_FDE
   GCC_3.0 {
     __register_frame_info_bases; __deregister_frame_info_bases;
Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.107
diff -u -r1.107 dl-close.c
--- elf/dl-close.c	14 Oct 2004 09:20:57 -0000	1.107
+++ elf/dl-close.c	4 Nov 2004 23:38:29 -0000
@@ -368,6 +368,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.12
diff -u -r1.12 dl-iteratephdr.c
--- elf/dl-iteratephdr.c	14 Oct 2004 02:02:06 -0000	1.12
+++ elf/dl-iteratephdr.c	4 Nov 2004 23:38:29 -0000
@@ -23,6 +23,19 @@
 #include <stddef.h>
 #include <bits/libc-lock.h>
 
+
+unsigned long long
+dl_phdr_additions_count (void)
+{
+  return GL(dl_load_adds);
+}
+
+unsigned long long
+dl_phdr_removals_count (void)
+{
+  return GL(dl_load_subs);
+}
+
 static void
 cancel_handler (void *arg __attribute__((unused)))
 {
Index: elf/dl-support.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-support.c,v
retrieving revision 1.86
diff -u -r1.86 dl-support.c
--- elf/dl-support.c	14 Oct 2004 02:06:18 -0000	1.86
+++ elf/dl-support.c	4 Nov 2004 23:38:29 -0000
@@ -73,6 +73,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/tst-dlmodcount.c
===================================================================
RCS file: /cvs/glibc/libc/elf/tst-dlmodcount.c,v
retrieving revision 1.3
diff -u -r1.3 tst-dlmodcount.c
--- elf/tst-dlmodcount.c	26 Mar 2004 09:48:53 -0000	1.3
+++ elf/tst-dlmodcount.c	4 Nov 2004 23:38:29 -0000
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_count (), dl_phdr_removals_count ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_count (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_count (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_count ();
+  last_subs = dl_phdr_removals_count ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
===================================================================
RCS file: /cvs/glibc/libc/include/link.h,v
retrieving revision 1.33
diff -u -r1.33 link.h
--- include/link.h	14 Oct 2004 01:57:54 -0000	1.33
+++ include/link.h	4 Nov 2004 23:38:30 -0000
@@ -317,4 +317,18 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1 when
+   objects are added to the list visited by dl_iterate_phdr().  This
+   function is not serialized with respect to dl_iterate_phdr().  If
+   serialization is needed, this function should be called from within
+   a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_additions_count (void);
+
+/* The vaue returned by this function increments by at least 1 when
+   objects are removed from the list visited by dl_iterate_phdr().
+   This function is not serialized with respect to dl_iterate_phdr().
+   If serialization is needed, this function should be called from
+   within a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_removals_count (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.105
diff -u -r1.105 ldsodefs.h
--- sysdeps/generic/ldsodefs.h	19 Oct 2004 22:19:17 -0000	1.105
+++ sysdeps/generic/ldsodefs.h	4 Nov 2004 23:38:31 -0000
@@ -250,6 +250,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-04 23:41     ` David Mosberger
@ 2004-11-04 23:53       ` Roland McGrath
  2004-11-05  0:40         ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-04 23:53 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

No new ABI is going to go in very soon.  We have frozen the GLIBC_2.3.4 ABI
already.  (Your patch adds symbols to a version set that has been fixed for
a long time, which is never a kind of patch that would be accepted.)

unsigned long long int is not an atomic type on all platforms, so your code
as written is not acceptable without adding locking for machines where it's
not.  It seems preferable to just used unsigned long int instead, which it
is safe to increment and examine simultaneously on all the platforms we have.

An interface to return the address of such variables is highly dubious,
since it constrains the implementation and does so permanently for the future.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-04 23:53       ` Roland McGrath
@ 2004-11-05  0:40         ` David Mosberger
  2004-11-05  1:10           ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-05  0:40 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Thu, 4 Nov 2004 15:53:55 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> No new ABI is going to go in very soon.  We have frozen the
  Roland> GLIBC_2.3.4 ABI already.

That's unfortunate.
Is there a mechanism to queue this patch so it doesn't get lost again
when 2.3.5 is opened up?

  Roland> unsigned long long int is not an atomic type on all
  Roland> platforms, so your code as written is not acceptable without
  Roland> adding locking for machines where it's not.

The incrementing is always done under protection of a lock.  The reading
is not, but on those machines where reading an "unsigned long long int"
isn't atomic, the effect is no worse than when using "unsigned int".
And on those machines where it is atomic, "unsigned long long int" pretty
much guarantees that the counter will never overflow.

Do you still think the patch is unacceptable in this regard?

  Roland> An interface to return the address of such variables is
  Roland> highly dubious, since it constrains the implementation and
  Roland> does so permanently for the future.

OK, I see your point.

Thanks,

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  0:40         ` David Mosberger
@ 2004-11-05  1:10           ` Roland McGrath
  2004-11-05  1:28             ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-05  1:10 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

> Is there a mechanism to queue this patch so it doesn't get lost again
> when 2.3.5 is opened up?

It never "got lost".  It's your baby, and you didn't follow up on it before
now.  We use bugzilla for keeping track of things, but something can sit
there unattended just as well if you don't stay on top of it.

> The incrementing is always done under protection of a lock.  The reading
> is not, but on those machines where reading an "unsigned long long int"
> isn't atomic, the effect is no worse than when using "unsigned int".
> And on those machines where it is atomic, "unsigned long long int" pretty
> much guarantees that the counter will never overflow.

Either it's a counter with a robust well-defined semantics, or it's not.
If it's not reliably usable as a counter, then there is no reason to call
it one.    

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  1:10           ` Roland McGrath
@ 2004-11-05  1:28             ` David Mosberger
  2004-11-05  1:36               ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-05  1:28 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Thu, 4 Nov 2004 17:10:51 -0800, Roland McGrath <roland@redhat.com> said:

  >> Is there a mechanism to queue this patch so it doesn't get lost
  >> again when 2.3.5 is opened up?

  Roland> It never "got lost".  It's your baby, and you didn't follow
  Roland> up on it before now.  We use bugzilla for keeping track of
  Roland> things, but something can sit there unattended just as well
  Roland> if you don't stay on top of it.

So when should I check back?

  >> The incrementing is always done under protection of a lock.  The
  >> reading is not, but on those machines where reading an "unsigned
  >> long long int" isn't atomic, the effect is no worse than when
  >> using "unsigned int".  And on those machines where it is atomic,
  >> "unsigned long long int" pretty much guarantees that the counter
  >> will never overflow.

  Roland> Either it's a counter with a robust well-defined semantics,
  Roland> or it's not.  If it's not reliably usable as a counter, then
  Roland> there is no reason to call it one.

Fine; I don't mind changing it back to "unsigned long int".

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  1:28             ` David Mosberger
@ 2004-11-05  1:36               ` Roland McGrath
  2004-11-05  2:09                 ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-05  1:36 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

> So when should I check back?

You should take responsibility for the work you want done and continue
improving it until you've submitted a patch that has nothing wrong with it.
Please don't pretend there is some mystery about how to contribute effectively.

> Fine; I don't mind changing it back to "unsigned long int".

What is the documentation about the meaning of the values and the effects
of overflow?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  1:36               ` Roland McGrath
@ 2004-11-05  2:09                 ` David Mosberger
  2004-11-05  3:27                   ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-05  2:09 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Thu, 4 Nov 2004 17:36:25 -0800, Roland McGrath <roland@redhat.com> said:

  >> So when should I check back?

  Roland> You should take responsibility for the work you want done
  Roland> and continue improving it until you've submitted a patch
  Roland> that has nothing wrong with it.  Please don't pretend there
  Roland> is some mystery about how to contribute effectively.

I'm not sure I understand.  You said the tree is currently closed for
new interfaces.  All I'm asking is when you're expecting the tree to
reopen again.

  >> Fine; I don't mind changing it back to "unsigned long int".

  Roland> What is the documentation about the meaning of the values
  Roland> and the effects of overflow?

As for the meaning of the values, see the documentation in link.h:

----
/* The value returned by this function increments by at least 1 when
   objects are added to the list visited by dl_iterate_phdr().  This
   function is not serialized with respect to dl_iterate_phdr().  If
   serialization is needed, this function should be called from within
   a dl_itereate_phdr() callback.  */
extern unsigned long int dl_phdr_additions_counter (void);

/* The vaue returned by this function increments by at least 1 when
   objects are removed from the list visited by dl_iterate_phdr().
   This function is not serialized with respect to dl_iterate_phdr().
   If serialization is needed, this function should be called from
   within a dl_itereate_phdr() callback.  */
extern unsigned long int dl_phdr_removals_counter (void);
----

I'll add a comment about the possibility of overflows.

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  2:09                 ` David Mosberger
@ 2004-11-05  3:27                   ` Roland McGrath
  2004-11-06  0:21                     ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-05  3:27 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

> I'm not sure I understand.  You said the tree is currently closed for
> new interfaces.  All I'm asking is when you're expecting the tree to
> reopen again.

Actually, what I said is that the GLIBC_2.3.4 ABI is frozen.  It doesn't
necessarily work as you say.  If there is a complete, desireable, and
working new interface ready to go in, there is no "opening date" for it.
It just won't be in the ABI version that is already frozen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-05  3:27                   ` Roland McGrath
@ 2004-11-06  0:21                     ` David Mosberger
  2004-11-06  2:32                       ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-06  0:21 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Thu, 4 Nov 2004 19:27:43 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> Actually, what I said is that the GLIBC_2.3.4 ABI is frozen.
  Roland> It doesn't necessarily work as you say.  If there is a
  Roland> complete, desireable, and working new interface ready to go
  Roland> in, there is no "opening date" for it.  It just won't be in
  Roland> the ABI version that is already frozen.

OK, I did misunderstand you then.

How does the patch below look?

I decided to stick with 64-bit counters, since dealing with overflows
is probably not worth the trouble (either we'd be emulating 64-bit
counters, which would needlessly hurt 64-bit platforms or we'd need a
call-back interface, which is tricky due to memory-management and
multi-threading/locking considerations).  The implementation below is
safe for 32-bit platforms because it falls back on locked accesses.
That will hurt, but we can fix that at least for those platforms which
can do atomic 64-bit loads and stores (on x86, we could certainly use
CMPXCHG8; perhaps there are even better instructions for doing 64-bit
atomic read and store, since that's all we really need here).

If the approach below looks reasonable to you, I'll finish the patch
up and do more testing (particularly on a 32-bit platform) and
documentation.

	--david

Index: Versions.def
--- Versions.def
+++ Versions.def
@@ -35,6 +35,7 @@
   GLIBC_2.0
   GLIBC_2.1
   GLIBC_2.3.3
+  GLIBC_2.3.5
 }
 libm {
   GLIBC_2.0
Index: elf/Makefile
--- elf/Makefile
+++ elf/Makefile
@@ -21,7 +21,7 @@
 subdir		:= elf
 
 headers		= elf.h bits/elfclass.h link.h
-routines	= $(dl-routines) dl-open dl-close dl-support dl-iteratephdr \
+routines	= $(dl-routines) dl-counters dl-open dl-close dl-support dl-iteratephdr \
 		  dl-addr enbl-secure dl-profstub \
 		  dl-origin dl-libc dl-sym dl-tsd
 
Index: elf/Versions
--- elf/Versions
+++ elf/Versions
@@ -11,6 +11,10 @@
   GLIBC_2.2.4 {
     dl_iterate_phdr;
   }
+  GLIBC_2.3.5 {
+    dl_phdr_additions_counter;
+    dl_phdr_removals_counter;
+  }
 %ifdef EXPORT_UNWIND_FIND_FDE
   GCC_3.0 {
     __register_frame_info_bases; __deregister_frame_info_bases;
Index: elf/dl-close.c
--- elf/dl-close.c
+++ elf/dl-close.c
@@ -368,6 +368,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-counters.c
--- /dev/null
+++ elf/dl-counters.c
@@ -0,0 +1,68 @@
+/* Access to dl-object addition/removal counters.
+   Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by David Mosberger <davidm@hpl.hp.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If not,
+   write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#include <errno.h>
+#include <ldsodefs.h>
+#include <stddef.h>
+#include <bits/libc-lock.h>
+
+static void
+cancel_handler (void *arg __attribute__((unused)))
+{
+  __rtld_lock_unlock_recursive (GL(dl_load_lock));
+}
+
+static unsigned long long int
+locked_read (unsigned long long int *ptr)
+{
+  unsigned long long int ret;
+
+  __rtld_lock_lock_recursive (GL(dl_load_lock));
+  __libc_cleanup_push (cancel_handler, 0);
+
+  ret = *ptr;
+
+  __libc_cleanup_pop (0);
+  __rtld_lock_unlock_recursive (GL(dl_load_lock));
+
+  return ret;
+}
+
+unsigned long long int
+__dl_phdr_additions_counter (void)
+{
+  if (sizeof (unsigned long long int) > sizeof (unsigned long int))
+    /* read of "unsigned long long int" may not be atomic; do it the hard way */
+    return locked_read (&GL(dl_load_adds));
+  else
+    return GL(dl_load_adds);
+}
+weak_alias (__dl_phdr_additions_counter, dl_phdr_additions_counter);
+
+unsigned long long int
+__dl_phdr_removals_counter (void)
+{
+  if (sizeof (unsigned long long int) > sizeof (unsigned long int))
+    /* read of "unsigned long long int" may not be atomic; do it the hard way */
+    return locked_read (&GL(dl_load_subs));
+  else
+    return GL(dl_load_subs);
+}
+weak_alias (__dl_phdr_removals_counter, dl_phdr_removals_counter);
Index: elf/dl-support.c
--- elf/dl-support.c
+++ elf/dl-support.c
@@ -73,6 +73,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/tst-dlmodcount.c
--- elf/tst-dlmodcount.c
+++ elf/tst-dlmodcount.c
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_counter (), dl_phdr_removals_counter ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_counter (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_counter (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_counter ();
+  last_subs = dl_phdr_removals_counter ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
--- include/link.h
+++ include/link.h
@@ -317,4 +317,23 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are added to the list visited by dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_additions_counter (void);
+
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are removed from the list visited by
+   dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_removals_counter (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
--- sysdeps/generic/ldsodefs.h
+++ sysdeps/generic/ldsodefs.h
@@ -250,6 +250,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-06  0:21                     ` David Mosberger
@ 2004-11-06  2:32                       ` Roland McGrath
  2004-11-09  0:15                         ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-06  2:32 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

There is no point in adding the new interface at all if it has the same
locking overhead.  The <atomic.h> atomic_increment macro works on 64 bits
when that is doable.  For platforms where it doesn't work, you can use
nonblocking synchronization a la seqcount.  I don't think we have any good
compile-time test for whether atomic_increment can handle 64 bits, but
probably all but i386 can.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-06  2:32                       ` Roland McGrath
@ 2004-11-09  0:15                         ` David Mosberger
  2004-11-09  4:40                           ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-09  0:15 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Fri, 5 Nov 2004 18:32:18 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> For platforms where it doesn't work, you can use nonblocking
  Roland> synchronization a la seqcount.

Are you referring to the Linux kernel seqlock or something else?

Thanks,

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-09  0:15                         ` David Mosberger
@ 2004-11-09  4:40                           ` Roland McGrath
  2004-11-10  5:40                             ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-09  4:40 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

> >>>>> On Fri, 5 Nov 2004 18:32:18 -0800, Roland McGrath <roland@redhat.com> said:
> 
>   Roland> For platforms where it doesn't work, you can use nonblocking
>   Roland> synchronization a la seqcount.
> 
> Are you referring to the Linux kernel seqlock or something else?

I was referring to the Linux kernel's "seqcount" macros, as an example of
the well-known technique.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-09  4:40                           ` Roland McGrath
@ 2004-11-10  5:40                             ` David Mosberger
  2004-11-10 20:36                               ` David Mosberger
  2004-11-10 20:45                               ` Roland McGrath
  0 siblings, 2 replies; 22+ messages in thread
From: David Mosberger @ 2004-11-10  5:40 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Mon, 8 Nov 2004 20:40:15 -0800, Roland McGrath <roland@redhat.com> said:

  >> >>>>> On Fri, 5 Nov 2004 18:32:18 -0800, Roland McGrath
  >> <roland@redhat.com> said:

  Roland> For platforms where it doesn't work, you can use nonblocking
  Roland> synchronization a la seqcount.

Below is a version that uses Keith Owens' (et al) seqlock approach to
support the counters on platforms where "unsigned long long" is bigger
than "unsigned long".  Since the writers are already serialized via a
lock, this should be very efficient and still correct.

Please apply, if there are no further objections.

Thanks,

	--david

 ChangeLog                  |   19 +++++
 Versions.def               |    1 
 elf/Makefile               |    2 
 elf/Versions               |    4 +
 elf/dl-close.c             |    1 
 elf/dl-counters.c          |  121 +++++++++++++++++++++++++++++++++++
 elf/dl-object.c            |    2 
 elf/dl-support.c           |    3 
 elf/rtld.c                 |    2 
 elf/tst-dlmodcount.c       |   36 ++++++++++
 include/link.h             |   19 +++++
 sysdeps/generic/ldsodefs.h |    6 +
 16 files changed, 280 insertions(+), 130 deletions(-)

ChangeLog

2004-11-09  David Mosberger  <davidm@hpl.hp.com>

	* Versions.def (ld): Mention GLIBC_2.3.5.
	* elf/Makefile (dl-routines): Mention dl-counters.
	* elf/Versions (dl_phdr_additions_counter): New export.
	(dl_phdr_removals_counter): Likewise.
	* elf/dl-counters.c: New file.
	* elf/tst-dlmodcount.c: Add tests for dl_phdr_additions_counter()
	and dl_phdr_removals_counter().
	* elf/dl-support.c (_dl_load_subs): New variable.
	* elf/dl-close.c (_dl_close): Increment dl_load_subs via a call to
	_dl_increment_counter().
	* elf/rtld.c (dl_main): Likewise.
	* include/link.h (dl_phdr_additions_counter): Declare.
	(dl_phdr_removals_counter): Likewise.
	* sysdeps/generic/ldsodefs.h (struct rtld_global): Add member
	"_dl_load_subs".
	(_dl_increment_counter): Declare as hidden & internal function.

Index: Versions.def
--- Versions.def
+++ Versions.def
@@ -98,6 +98,7 @@
   GLIBC_2.0
   GLIBC_2.1
   GLIBC_2.3
+  GLIBC_2.3.5
   GLIBC_PRIVATE
 }
 libthread_db {
Index: elf/Makefile
--- elf/Makefile
+++ elf/Makefile
@@ -27,7 +27,7 @@
 
 # The core dynamic linking functions are in libc for the static and
 # profiled libraries.
-dl-routines	= $(addprefix dl-,load cache lookup object reloc deps \
+dl-routines	= $(addprefix dl-,load cache counters lookup object reloc deps \
 			          runtime error init fini debug misc \
 				  version profile conflict tls origin \
 				  execstack caller)
Index: elf/Versions
--- elf/Versions
+++ elf/Versions
@@ -43,6 +43,10 @@
     # runtime interface to TLS
     __tls_get_addr;
   }
+  GLIBC_2.3.5 {
+    dl_phdr_additions_counter;
+    dl_phdr_removals_counter;
+  }
   GLIBC_PRIVATE {
     # Those are in the dynamic linker, but used by libc.so.
     __libc_enable_secure;
Index: elf/dl-close.c
--- elf/dl-close.c
+++ elf/dl-close.c
@@ -368,6 +368,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  _dl_increment_counter (&GL(dl_load_subs));
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-counters.c
--- /dev/null
+++ elf/dl-counters.c
@@ -0,0 +1,121 @@
+/* Access to dl-object addition/removal counters.
+   Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by David Mosberger <davidm@hpl.hp.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If not,
+   write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#include <errno.h>
+#include <ldsodefs.h>
+#include <stddef.h>
+#include <atomic.h>
+
+/* This seqlock is analogous to the one by Keith Owens found in the
+   Linux kernel, except that we don't need a spinlock since we know
+   that calls to _dl_increment_counter() are already serialized via
+   dl_load_lock.  */
+
+struct seqlock
+  {
+    unsigned long int sequence;
+  };
+
+
+/* Return TRUE if loads and stores to "unsigned long long int"
+   variables are atomic.  According to Roland McGrath, Libc assumes
+   this to be always true for variables of type "unsigned long"...  */
+
+static inline int
+long_long_is_atomic (void)
+{
+  return sizeof (unsigned long long int) == sizeof (unsigned long int);
+}
+
+static inline void
+write_seqlock (struct seqlock *sl)
+{
+  ++sl->sequence;
+  atomic_write_barrier ();
+}
+
+static inline void
+write_sequnlock (struct seqlock *sl)
+{
+  atomic_write_barrier ();
+  ++sl->sequence;
+}
+
+static inline unsigned long int
+read_seqbegin (struct seqlock *sl)
+{
+  unsigned long int seq = sl->sequence;
+  atomic_read_barrier ();
+  return seq;
+}
+
+static inline int
+read_seqretry (struct seqlock *sl, unsigned long int seq)
+{
+  atomic_read_barrier ();
+  return (seq & 1) | (sl->sequence ^ seq);
+}
+
+static struct seqlock sl;
+
+static inline unsigned long long int
+read_counter (unsigned long long int *cp)
+{
+  unsigned long long int val;
+  unsigned long int seq;
+
+  if (long_long_is_atomic ())
+    val = *cp;
+  else
+    do
+      {
+	seq = read_seqbegin (&sl);
+	val = *cp;
+      }
+    while (read_seqretry (&sl, seq));
+  return val;
+}
+
+void
+internal_function
+_dl_increment_counter (unsigned long long int *cp)
+{
+  if (!long_long_is_atomic ())
+    write_seqlock (&sl);
+
+  ++*cp;
+
+  if (!long_long_is_atomic ())
+    write_sequnlock (&sl);
+}
+
+unsigned long long int
+__dl_phdr_additions_counter (void)
+{
+  return read_counter (&GL(dl_load_adds));
+}
+weak_alias (__dl_phdr_additions_counter, dl_phdr_additions_counter);
+
+unsigned long long int
+__dl_phdr_removals_counter (void)
+{
+  return read_counter (&GL(dl_load_subs));
+}
+weak_alias (__dl_phdr_removals_counter, dl_phdr_removals_counter);
Index: elf/dl-object.c
--- elf/dl-object.c
+++ elf/dl-object.c
@@ -85,7 +85,7 @@
   else
     GL(dl_ns)[nsid]._ns_loaded = new;
   ++GL(dl_ns)[nsid]._ns_nloaded;
-  ++GL(dl_load_adds);
+  _dl_increment_counter (&GL(dl_load_adds));
 
   /* If we have no loader the new object acts as it.  */
   if (loader == NULL)
Index: elf/dl-support.c
--- elf/dl-support.c
+++ elf/dl-support.c
@@ -73,6 +73,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/rtld.c
--- elf/rtld.c
+++ elf/rtld.c
@@ -1085,7 +1085,7 @@
   main_map->l_next = &GL(dl_rtld_map);
   GL(dl_rtld_map).l_prev = main_map;
   ++GL(dl_ns)[LM_ID_BASE]._ns_nloaded;
-  ++GL(dl_load_adds);
+  _dl_increment_counter (&GL(dl_load_adds));
 
   /* If LD_USE_LOAD_BIAS env variable has not been seen, default
      to not using bias for non-prelinked PIEs and libraries
Index: elf/tst-dlmodcount.c
--- elf/tst-dlmodcount.c
+++ elf/tst-dlmodcount.c
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_counter (), dl_phdr_removals_counter ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_counter (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_counter (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_counter ();
+  last_subs = dl_phdr_removals_counter ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
--- include/link.h
+++ include/link.h
@@ -317,4 +317,23 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are added to the list visited by dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_additions_counter (void);
+
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are removed from the list visited by
+   dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_removals_counter (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
--- sysdeps/generic/ldsodefs.h
+++ sysdeps/generic/ldsodefs.h
@@ -250,6 +250,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;
@@ -909,6 +912,9 @@
 extern int _dl_check_caller (const void *caller, enum allowmask mask)
      attribute_hidden;
 
+extern void _dl_increment_counter (unsigned long long int *cp);
+     internal_function attribute_hidden;
+
 __END_DECLS
 
 #endif /* ldsodefs.h */

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-10  5:40                             ` David Mosberger
@ 2004-11-10 20:36                               ` David Mosberger
  2004-11-10 20:45                               ` Roland McGrath
  1 sibling, 0 replies; 22+ messages in thread
From: David Mosberger @ 2004-11-10 20:36 UTC (permalink / raw)
  To: Roland McGrath, libc-hacker

Hold off on that patch --- I just discovered a symbol visibility problem.
Should have an update later today.

Sorry about that.

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-10  5:40                             ` David Mosberger
  2004-11-10 20:36                               ` David Mosberger
@ 2004-11-10 20:45                               ` Roland McGrath
  2004-11-11  6:17                                 ` David Mosberger
  1 sibling, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-10 20:45 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

That looks pretty reasonable.  But we will in fact probably hold off for a
while longer on adding any new interfaces rather than branching sooner.
We are still getting a steady stream of good stabilizing fixes on the trunk.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-10 20:45                               ` Roland McGrath
@ 2004-11-11  6:17                                 ` David Mosberger
  2004-11-11 22:24                                   ` Roland McGrath
  0 siblings, 1 reply; 22+ messages in thread
From: David Mosberger @ 2004-11-11  6:17 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Wed, 10 Nov 2004 12:45:50 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> That looks pretty reasonable.

OK, thanks.  I attached an updated patch which fixes the
symbol-problem I was referring to in the earlier mail.  I tested this
patch on a freshly checked out libc tree, just to make sure it's
applying properly and builds and works as expected.  There were no
(new) failures with the patch applied, so this one should be good to
go.

  Roland> But we will in fact probably hold off for a while longer on
  Roland> adding any new interfaces rather than branching sooner.  We
  Roland> are still getting a steady stream of good stabilizing fixes
  Roland> on the trunk.

That's a bit disappointing.  Earlier you said:

  Roland> If there is a complete, desireable, and working new
  Roland> interface ready to go in, there is no "opening date" for it.

Is the attached patch falling short of being complete, desireable, and
working?

Thanks,

	--david

---------------------------------------------------------------------
ChangeLog

2004-11-09  David Mosberger  <davidm@hpl.hp.com>

	* Versions.def (ld): Mention GLIBC_2.3.5.
	* elf/Makefile (dl-routines): Mention dl-counters.
	* elf/Versions (dl_phdr_additions_counter): New export.
	(dl_phdr_removals_counter): Likewise.
	(_dl_increment_counter): Export as a GLIBC_PRIVATE symbol (needed
	by dl-close.c)
	* elf/dl-counters.c: New file.
	* elf/tst-dlmodcount.c: Add tests for dl_phdr_additions_counter()
	and dl_phdr_removals_counter().
	* elf/dl-support.c (_dl_load_subs): New variable.
	* elf/dl-close.c (_dl_close): Increment dl_load_subs via a call to
	_dl_increment_counter().
	* elf/rtld.c (dl_main): Likewise.
	* include/link.h (dl_phdr_additions_counter): Declare.
	(dl_phdr_removals_counter): Likewise.
	* sysdeps/generic/ldsodefs.h (struct rtld_global): Add member
	"_dl_load_subs".
	(_dl_increment_counter): Declare as hidden & internal function.

Index: Versions.def
--- Versions.def
+++ Versions.def
@@ -98,6 +98,7 @@
   GLIBC_2.0
   GLIBC_2.1
   GLIBC_2.3
+  GLIBC_2.3.5
   GLIBC_PRIVATE
 }
 libthread_db {
Index: elf/Makefile
--- elf/Makefile
+++ elf/Makefile
@@ -27,7 +27,7 @@
 
 # The core dynamic linking functions are in libc for the static and
 # profiled libraries.
-dl-routines	= $(addprefix dl-,load cache lookup object reloc deps \
+dl-routines	= $(addprefix dl-,load cache counters lookup object reloc deps \
 			          runtime error init fini debug misc \
 				  version profile conflict tls origin \
 				  execstack caller)
Index: elf/Versions
--- elf/Versions
+++ elf/Versions
@@ -43,6 +43,10 @@
     # runtime interface to TLS
     __tls_get_addr;
   }
+  GLIBC_2.3.5 {
+    dl_phdr_additions_counter;
+    dl_phdr_removals_counter;
+  }
   GLIBC_PRIVATE {
     # Those are in the dynamic linker, but used by libc.so.
     __libc_enable_secure;
@@ -54,6 +58,7 @@
     _dl_get_tls_static_info; _dl_allocate_tls_init;
     _dl_tls_setup; _dl_rtld_di_serinfo;
     _dl_make_stack_executable;
+    _dl_increment_counter;
     # Only here for gdb while a better method is developed.
     _dl_debug_state;
   }
Index: elf/dl-close.c
--- elf/dl-close.c
+++ elf/dl-close.c
@@ -368,6 +368,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  _dl_increment_counter (&GL(dl_load_subs));
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-counters.c
--- /dev/null
+++ elf/dl-counters.c
@@ -0,0 +1,121 @@
+/* Access to dl-object addition/removal counters.
+   Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by David Mosberger <davidm@hpl.hp.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If not,
+   write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#include <errno.h>
+#include <ldsodefs.h>
+#include <stddef.h>
+#include <atomic.h>
+
+/* This seqlock is analogous to the one by Keith Owens found in the
+   Linux kernel, except that we don't need a spinlock since we know
+   that calls to _dl_increment_counter() are already serialized via
+   dl_load_lock.  */
+
+struct seqlock
+  {
+    unsigned long int sequence;
+  };
+
+
+/* Return TRUE if loads and stores to "unsigned long long int"
+   variables are atomic.  According to Roland McGrath, Libc assumes
+   this to be always true for variables of type "unsigned long"...  */
+
+static inline int
+long_long_is_atomic (void)
+{
+  return sizeof (unsigned long long int) == sizeof (unsigned long int);
+}
+
+static inline void
+write_seqlock (struct seqlock *sl)
+{
+  ++sl->sequence;
+  atomic_write_barrier ();
+}
+
+static inline void
+write_sequnlock (struct seqlock *sl)
+{
+  atomic_write_barrier ();
+  ++sl->sequence;
+}
+
+static inline unsigned long int
+read_seqbegin (struct seqlock *sl)
+{
+  unsigned long int seq = sl->sequence;
+  atomic_read_barrier ();
+  return seq;
+}
+
+static inline int
+read_seqretry (struct seqlock *sl, unsigned long int seq)
+{
+  atomic_read_barrier ();
+  return (seq & 1) | (sl->sequence ^ seq);
+}
+
+static struct seqlock sl;
+
+static inline unsigned long long int
+read_counter (unsigned long long int *cp)
+{
+  unsigned long long int val;
+  unsigned long int seq;
+
+  if (long_long_is_atomic ())
+    val = *cp;
+  else
+    do
+      {
+	seq = read_seqbegin (&sl);
+	val = *cp;
+      }
+    while (read_seqretry (&sl, seq));
+  return val;
+}
+
+void
+internal_function
+_dl_increment_counter (unsigned long long int *cp)
+{
+  if (!long_long_is_atomic ())
+    write_seqlock (&sl);
+
+  ++*cp;
+
+  if (!long_long_is_atomic ())
+    write_sequnlock (&sl);
+}
+
+unsigned long long int
+__dl_phdr_additions_counter (void)
+{
+  return read_counter (&GL(dl_load_adds));
+}
+weak_alias (__dl_phdr_additions_counter, dl_phdr_additions_counter);
+
+unsigned long long int
+__dl_phdr_removals_counter (void)
+{
+  return read_counter (&GL(dl_load_subs));
+}
+weak_alias (__dl_phdr_removals_counter, dl_phdr_removals_counter);
Index: elf/dl-object.c
--- elf/dl-object.c
+++ elf/dl-object.c
@@ -85,7 +85,7 @@
   else
     GL(dl_ns)[nsid]._ns_loaded = new;
   ++GL(dl_ns)[nsid]._ns_nloaded;
-  ++GL(dl_load_adds);
+  _dl_increment_counter (&GL(dl_load_adds));
 
   /* If we have no loader the new object acts as it.  */
   if (loader == NULL)
Index: elf/dl-support.c
--- elf/dl-support.c
+++ elf/dl-support.c
@@ -73,6 +73,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/rtld.c
--- elf/rtld.c
+++ elf/rtld.c
@@ -1085,7 +1085,7 @@
   main_map->l_next = &GL(dl_rtld_map);
   GL(dl_rtld_map).l_prev = main_map;
   ++GL(dl_ns)[LM_ID_BASE]._ns_nloaded;
-  ++GL(dl_load_adds);
+  _dl_increment_counter (&GL(dl_load_adds));
 
   /* If LD_USE_LOAD_BIAS env variable has not been seen, default
      to not using bias for non-prelinked PIEs and libraries
Index: elf/tst-dlmodcount.c
--- elf/tst-dlmodcount.c
+++ elf/tst-dlmodcount.c
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_counter (), dl_phdr_removals_counter ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_counter (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_counter (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_counter ();
+  last_subs = dl_phdr_removals_counter ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
--- include/link.h
+++ include/link.h
@@ -317,4 +317,23 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are added to the list visited by dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_additions_counter (void);
+
+/* The value returned by this function increments by at least 1
+   (modulo N, where N=2^64 on platforms with 64-bit "long long int")
+   when objects are removed from the list visited by
+   dl_iterate_phdr().
+
+   This function may not be serialized with respect to
+   dl_iterate_phdr().  If serialization is needed, this function
+   should be called from within a dl_itereate_phdr() callback.  */
+extern unsigned long long int dl_phdr_removals_counter (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
--- sysdeps/generic/ldsodefs.h
+++ sysdeps/generic/ldsodefs.h
@@ -250,6 +250,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;
@@ -909,6 +912,9 @@
 extern int _dl_check_caller (const void *caller, enum allowmask mask)
      attribute_hidden;
 
+extern void _dl_increment_counter (unsigned long long int *cp);
+     internal_function attribute_hidden;
+
 __END_DECLS
 
 #endif /* ldsodefs.h */

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-11  6:17                                 ` David Mosberger
@ 2004-11-11 22:24                                   ` Roland McGrath
  2004-11-12 19:32                                     ` David Mosberger
  0 siblings, 1 reply; 22+ messages in thread
From: Roland McGrath @ 2004-11-11 22:24 UTC (permalink / raw)
  To: davidm; +Cc: libc-hacker

> That's a bit disappointing.  Earlier you said:
> 
>   Roland> If there is a complete, desireable, and working new
>   Roland> interface ready to go in, there is no "opening date" for it.
> 
> Is the attached patch falling short of being complete, desireable, and
> working?

There is indeed no set "opening date".  There is, however, a daily
judgement on what kind of thing to be putting into the tree this week.  
For this week, it's no new interfaces and just bug fixes.  In fact, I won't
have time even to fully review your patch this week.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
  2004-11-11 22:24                                   ` Roland McGrath
@ 2004-11-12 19:32                                     ` David Mosberger
  0 siblings, 0 replies; 22+ messages in thread
From: David Mosberger @ 2004-11-12 19:32 UTC (permalink / raw)
  To: Roland McGrath; +Cc: davidm, libc-hacker

>>>>> On Thu, 11 Nov 2004 14:24:40 -0800, Roland McGrath <roland@redhat.com> said:

  >> That's a bit disappointing.  Earlier you said:

  Roland> If there is a complete, desireable, and working new
  Roland> interface ready to go in, there is no "opening date" for it.

  >> Is the attached patch falling short of being complete, desireable, and
  >> working?

  Roland> There is indeed no set "opening date".  There is, however, a
  Roland> daily judgement on what kind of thing to be putting into the
  Roland> tree this week.  For this week, it's no new interfaces and
  Roland> just bug fixes.  In fact, I won't have time even to fully
  Roland> review your patch this week.

Ah, ok, that doesn't sound like a problem.  I certainly don't expect
instant reviews.  If it takes a few days or a week or two, that's
certainly fine.  Just let me know if you find any issues with the
patch.

Thanks,

	--david

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: second thoughts on using dl_iterate_phdr() for cache-validation
@ 2004-04-15 18:28 David Mosberger
  0 siblings, 0 replies; 22+ messages in thread
From: David Mosberger @ 2004-04-15 18:28 UTC (permalink / raw)
  To: roland, libc-hacker

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 93 bytes --]

Roland,

Any comments on the proposal?  I'd really like to wrap this up.

Thanks,

	--david


[-- Attachment #2: forwarded message --]
[-- Type: message/rfc822, Size: 10305 bytes --]

From: David Mosberger <davidm@linux.hpl.hp.com>
To: Roland McGrath <roland@redhat.com>
Cc: davidm@hpl.hp.com, libc-hacker@sources.redhat.com
Subject: Re: second thoughts on using dl_iterate_phdr() for cache-validation
Date: Wed, 31 Mar 2004 18:32:57 -0800
Message-ID: <16491.32601.193260.288217@napali.hpl.hp.com>

>>>>> On Wed, 31 Mar 2004 00:04:25 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> The only thing I see is to give you a way to see
  Roland> _dl_load_adds without calling __dl_iterate_phdr and taking
  Roland> locks, which is what you asked for in the first place.  We
  Roland> really can't give you an exported variable symbol, so it
  Roland> would have to be a new function interface.

How about something along the lines of this:

-----------
/* The value returned by this function increments by at least 1 when
   objects are added to the list visited by dl_iterate_phdr().  This
   function is not serialized with respect to dl_iterate_phdr().  If
   serialization is needed, this function should be called from within
   a dl_itereate_phdr() callback.  */
extern unsigned long long dl_phdr_additions_counter (void);

/* The vaue returned by this function increments by at least 1 when
   objects are removed from the list visited by dl_iterate_phdr().
   This function is not serialized with respect to dl_iterate_phdr().
   If serialization is needed, this function should be called from
   within a dl_itereate_phdr() callback.  */
extern unsigned long long dl_phdr_removals_counter (void);
-----------

I considered using a single function with an argument which specifies
the counter to read, but then I thought that many programs may only
read one or the other counter.  Plus if there is two counters, someone
might want to add a third counter in the future and if that needs to
be supported, there would have to be a way for detecting whether a
libc supports the new counter, etc., so I thought having two separate
routines might not be such a bad idea after all.

A preliminary but complete patch is below and it dropped the cost of
unw_init_local() from 223 to 119ns (on the faster machine I profiled
yesterday).  The code of unw_init_local() itself accounts for 113ns
and the overhead of 6ns is about in line with a shared library call.
This performance-level seems very acceptable to me.  Note that the way
things work in libunwind, I don't have to worry about concurrent
dl_close() operations and hence it is safe to read the counter without
locking.

Some questions/comments:

 - I always get confused about the versioning stuff in glibc.
   I added the new functions to elf/Versions, but am not sure
   this is really correct.

 - I wasn't sure what the right place for the new functions is.
   For now, I put them into dl-iteratephdr.c which is probably
   a bad choice.  Furthermore, I'm not sure about name-space
   pollution issues.  Advice on the naming etc. would be welcome.

 - For now, I didn't remove the dl_adds/dl_subs members from the
   dl_iterate_phdr() callback.  With the new interface, they won't be
   needed anymore and I suspect nobody other than libunwind would be
   affected if we removed them (and even old libunwind versions would
   continue work properly).  If you agree, I'll cook up a second patch
   to remove this stuff once the new interface is in.

Thanks,

	--david

Index: elf/Versions
===================================================================
RCS file: /cvs/glibc/libc/elf/Versions,v
retrieving revision 1.51
diff -u -r1.51 Versions
--- elf/Versions	6 Mar 2004 08:04:12 -0000	1.51
+++ elf/Versions	1 Apr 2004 02:18:54 -0000
@@ -11,6 +11,10 @@
   GLIBC_2.2.4 {
     dl_iterate_phdr;
   }
+  GLIBC_2.3.2 {
+    dl_phdr_additions_counter;
+    dl_phdr_removals_counter;
+  }
 %ifdef EXPORT_UNWIND_FIND_FDE
   GCC_3.0 {
     __register_frame_info_bases; __deregister_frame_info_bases;
Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.103
diff -u -r1.103 dl-close.c
--- elf/dl-close.c	7 Mar 2004 08:38:43 -0000	1.103
+++ elf/dl-close.c	1 Apr 2004 02:18:55 -0000
@@ -319,6 +319,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   GLRO(dl_debug_state) ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.11
diff -u -r1.11 dl-iteratephdr.c
--- elf/dl-iteratephdr.c	27 Jan 2004 21:29:22 -0000	1.11
+++ elf/dl-iteratephdr.c	1 Apr 2004 02:18:55 -0000
@@ -23,6 +23,19 @@
 #include <stddef.h>
 #include <bits/libc-lock.h>
 
+
+unsigned long long
+dl_phdr_additions_counter (void)
+{
+  return GL(dl_load_adds);
+}
+
+unsigned long long
+dl_phdr_removals_counter (void)
+{
+  return GL(dl_load_subs);
+}
+
 static void
 cancel_handler (void *arg __attribute__((unused)))
 {
Index: elf/dl-support.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-support.c,v
retrieving revision 1.83
diff -u -r1.83 dl-support.c
--- elf/dl-support.c	6 Mar 2004 09:06:15 -0000	1.83
+++ elf/dl-support.c	1 Apr 2004 02:18:55 -0000
@@ -75,6 +75,9 @@
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
 
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/tst-dlmodcount.c
===================================================================
RCS file: /cvs/glibc/libc/elf/tst-dlmodcount.c,v
retrieving revision 1.3
diff -u -r1.3 tst-dlmodcount.c
--- elf/tst-dlmodcount.c	26 Mar 2004 09:48:53 -0000	1.3
+++ elf/tst-dlmodcount.c	1 Apr 2004 02:18:55 -0000
@@ -71,6 +71,39 @@
   return -1;
 }
 
+static void
+check (int cmd)
+{
+  static int last_adds = 0, last_subs = 0;
+
+  printf ("  additions = %Lu removals = %Lu\n",
+	  dl_phdr_additions_counter (), dl_phdr_removals_counter ());
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (dl_phdr_additions_counter (), last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (dl_phdr_removals_counter (), last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = dl_phdr_additions_counter ();
+  last_subs = dl_phdr_removals_counter ();
+}
+
 static void *
 load (const char *path)
 {
@@ -81,6 +114,7 @@
   if (!handle)
     exit (1);
   dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  check (ADD);
   return handle;
 }
 
@@ -91,6 +125,7 @@
   if (dlclose (handle) < 0)
     exit (2);
   dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+  check (REMOVE);
 }
 
 int
@@ -99,6 +134,7 @@
   void *handle1, *handle2;
 
   dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  check (SET);
   handle1 = load ("firstobj.so");
   handle2 = load ("globalmod1.so");
   unload ("firstobj.so", handle1);
Index: include/link.h
===================================================================
RCS file: /cvs/glibc/libc/include/link.h,v
retrieving revision 1.31
diff -u -r1.31 link.h
--- include/link.h	27 Mar 2004 03:22:29 -0000	1.31
+++ include/link.h	1 Apr 2004 02:18:55 -0000
@@ -307,4 +307,18 @@
 					       size_t size, void *data),
 			      void *data);
 
+/* The value returned by this function increments by at least 1 when
+   objects are added to the list visited by dl_iterate_phdr().  This
+   function is not serialized with respect to dl_iterate_phdr().  If
+   serialization is needed, this function should be called from within
+   a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_additions_counter (void);
+
+/* The vaue returned by this function increments by at least 1 when
+   objects are removed from the list visited by dl_iterate_phdr().
+   This function is not serialized with respect to dl_iterate_phdr().
+   If serialization is needed, this function should be called from
+   within a dl_itereate_phdr() callback.  */
+extern unsigned long long dl_phdr_removals_counter (void);
+
 #endif /* link.h */
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.100
diff -u -r1.100 ldsodefs.h
--- sysdeps/generic/ldsodefs.h	27 Mar 2004 03:23:51 -0000	1.100
+++ sysdeps/generic/ldsodefs.h	1 Apr 2004 02:18:59 -0000
@@ -241,6 +241,9 @@
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
 
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2004-11-12 19:32 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-03-31  8:53 second thoughts on using dl_iterate_phdr() for cache-validation David Mosberger
2004-03-31 16:04 ` Roland McGrath
2004-04-01  2:33   ` David Mosberger
2004-11-04 23:41     ` David Mosberger
2004-11-04 23:53       ` Roland McGrath
2004-11-05  0:40         ` David Mosberger
2004-11-05  1:10           ` Roland McGrath
2004-11-05  1:28             ` David Mosberger
2004-11-05  1:36               ` Roland McGrath
2004-11-05  2:09                 ` David Mosberger
2004-11-05  3:27                   ` Roland McGrath
2004-11-06  0:21                     ` David Mosberger
2004-11-06  2:32                       ` Roland McGrath
2004-11-09  0:15                         ` David Mosberger
2004-11-09  4:40                           ` Roland McGrath
2004-11-10  5:40                             ` David Mosberger
2004-11-10 20:36                               ` David Mosberger
2004-11-10 20:45                               ` Roland McGrath
2004-11-11  6:17                                 ` David Mosberger
2004-11-11 22:24                                   ` Roland McGrath
2004-11-12 19:32                                     ` David Mosberger
2004-04-15 18:28 David Mosberger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).