diff mbox

RFC Patch finalize binding of symbols when auditing is no longer required

Message ID 3201A41D-80B7-42E8-A0D2-39EC6DAFF983@redhat.com
State New
Headers show

Commit Message

Ben Woodard July 8, 2015, 4:59 p.m. UTC 
At LLNL they have an application called Spindle  https://computation.llnl.gov/project/spindle/  which is used to accelerate the loading of HPC MPI codes on very large clusters. Spindle works as an audit library and one of the challenges that it faces is that although it needs to intercept one invocation of each symbol called, performance of the application suffers because each subsequent invocation of the symbol goes through the dynamic linker just in case an auditing library may want to track its PLT entry or exit. We believe that we can reclaim this performance loss by allowing the dynamic linker to finalize the binding of the symbol in the case when all the auditing libraries signal their lack of interest in subsequent auditing of that symbol by setting both LA_SYMB_NOPLTENTER and LA_SYMB_NOPLTEXIT in the flags parameter of either la_symbind() or la_pltenter().

I have manually tested that this patch works and the customer confirms that this reclaims enough performance to be acceptable. 

As I was developing the previous version of the patch, which introduced a new LA_FLG_ flag in la_objopen() to signal non-interest, I pointed out to Carlos that it was odd that there was no LA_FLG_NOBIND and he said to add it. In this current version of the patch, it looks less related and if that matters, I’m happy to split that off into its own patch.

From a443be2675a9411218d17a6e611df3b6662a31bf Mon Sep 17 00:00:00 2001
From: Ben Woodard <woodard@redhat.com>
Date: Tue, 23 Jun 2015 21:45:01 -0700
Subject: [PATCH] Finalize binding of uninteresting symbols 

 In the case where no audit library is interested in a
 particular symbol, go ahead and finalize its binding. This improves
 performance for cases where an audit library only needs to track one trip
 through the PLT and returns peformance to that of a normally lazily bound
 symbol. Audit libraries can signal their lack of interest in a symbol by
 setting both LA_SYMB_NOPLTENTER and LA_SYMB_NOPLTEXIT in the flags parameter
 of la_symbind() or la_pltenter(). If multiple audit libraries are loaded and
 they don't all agree that the symbol is no further interest, then the symbol
 binding will not be finalized.

Signed-off-by: Ben Woodard <woodard@redhat.com>
---
 ChangeLog        |  6 ++++++
 elf/dl-runtime.c | 25 +++++++++++++++++++++----
 elf/link.h       |  1 +
 3 files changed, 28 insertions(+), 4 deletions(-)

Comments

Mike Frysinger July 10, 2015, 8:24 a.m. UTC | #1 
On 08 Jul 2015 09:59, Ben Woodard wrote:
> +      memset(&reloc_result_value, '\0', sizeof(reloc_result_value));

needs spaces before the (

> +  /* If we have auditing libraries and PLT auditing for this symbol has
> +     been disabled for all libraries, then go ahead and finalize binding
> +     the symbol rather than forcing each call to go through the PLT. */

two spaces after the .

> +  if ( (reloc_result->enterexit & LA_SYMB_NOPLTENTER) &&
> +       (reloc_result->enterexit & LA_SYMB_NOPLTEXIT) )

no spaces after the first (, the && should be on the 2nd line, and no 
space before the ).

>  enum
>    {
> +    LA_FLG_NOBIND = 0x00,       /* Don't audit symbols from this object */
>      LA_FLG_BINDTO = 0x01,	/* Audit symbols bound to this object.  */
>      LA_FLG_BINDFROM = 0x02	/* Audit symbols bound from this object.  */
>    };

should indent the comment with tabs and have trailing period (and two spaces)

code wise seems reasonable, but i'm not that familiar with the audit interfaces
-mike
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index f909f7a..7f6afdd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -31,6 +31,12 @@ 
 	* nscd/nscd-client.h (__nscd_acquire_maplock): Use atomic_spin_nop
 	instead of atomic_delay.
 
+2015-06-29  Ben Woodard <woodard@redhat.com>
+
+	* elf/link.h add LA_FLG_NOBIND
+	* elf/dl-runtime.c If no auditing libraries are interested in the
+	PLT entry/exit of a particular symbol, go ahead and finalize it.
+
 2015-06-29  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #18613]
diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
index bd9a1b1..0dd8942 100644
--- a/elf/dl-runtime.c
+++ b/elf/dl-runtime.c
@@ -182,10 +182,21 @@  _dl_profile_fixup (
 
   /* This is the address in the array where we store the result of previous
      relocations.  */
-  struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index];
+  struct reloc_result reloc_result_value;
+  struct reloc_result *reloc_result;
+  if (l->l_reloc_result)
+    reloc_result = &l->l_reloc_result[reloc_index];
+  else
+    {
+      memset(&reloc_result_value, '\0', sizeof(reloc_result_value));
+      reloc_result = &reloc_result_value;
+    }
   DL_FIXUP_VALUE_TYPE *resultp = &reloc_result->addr;
 
   DL_FIXUP_VALUE_TYPE value = *resultp;
+  const PLTREL *const reloc
+    = (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset);
+  lookup_t result;
   if (DL_FIXUP_VALUE_CODE_ADDR (value) == 0)
     {
       /* This is the first time we have to relocate this object.  */
@@ -193,11 +204,8 @@  _dl_profile_fixup (
 	= (const void *) D_PTR (l, l_info[DT_SYMTAB]);
       const char *strtab = (const char *) D_PTR (l, l_info[DT_STRTAB]);
 
-      const PLTREL *const reloc
-	= (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset);
       const ElfW(Sym) *refsym = &symtab[ELFW(R_SYM) (reloc->r_info)];
       const ElfW(Sym) *defsym = refsym;
-      lookup_t result;
 
       /* Sanity check that we're really looking at a PLT relocation.  */
       assert (ELFW(R_TYPE)(reloc->r_info) == ELF_MACHINE_JMP_SLOT);
@@ -428,6 +436,15 @@  _dl_profile_fixup (
   /* Store the frame size information.  */
   *framesizep = framesize;
 
+  /* If we have auditing libraries and PLT auditing for this symbol has
+     been disabled for all libraries, then go ahead and finalize binding
+     the symbol rather than forcing each call to go through the PLT. */
+  if ( (reloc_result->enterexit & LA_SYMB_NOPLTENTER) &&
+       (reloc_result->enterexit & LA_SYMB_NOPLTEXIT) )
+    {
+      void *const rel_addr = (void *)(l->l_addr + reloc->r_offset);
+      return elf_machine_fixup_plt (l, result, reloc, rel_addr, value);
+    }
   (*mcount_fct) (retaddr, DL_FIXUP_VALUE_CODE_ADDR (value));
 
   return value;
diff --git a/elf/link.h b/elf/link.h
index eaca802..f0a4156 100644
--- a/elf/link.h
+++ b/elf/link.h
@@ -120,6 +120,7 @@  enum
 /* Values for la_objopen return value.  */
 enum
   {
+    LA_FLG_NOBIND = 0x00,       /* Don't audit symbols from this object */
     LA_FLG_BINDTO = 0x01,	/* Audit symbols bound to this object.  */
     LA_FLG_BINDFROM = 0x02	/* Audit symbols bound from this object.  */
   };