[RFC] Tunable elision patch for siddhesh/tunables

Message ID 5612A237.7040409@linux.vnet.ibm.com
State New, archived
Headers

Commit Message

Paul E. Murphy Oct. 5, 2015, 4:15 p.m. UTC
  Siddhesh et al,

In continuing the glibc tunables discussion, I've made some
additional patches which I think would help our work on the
siddhesh/tunables branch.

I've split this into two patches:

* The first should enable tunables to initialize without
calling malloc(), and prior to __environ being set.

* The second adds a common tunables initialization function
for elision, and enables it for supported archs.

I have tested this on PPC64. I need help testing for
supported s390 and x86 platforms.

Thanks,
Paul
  

Comments

Siddhesh Poyarekar Oct. 6, 2015, 9:22 a.m. UTC | #1
On Monday 05 October 2015 09:45 PM, Paul E. Murphy wrote:
> In continuing the glibc tunables discussion, I've made some
> additional patches which I think would help our work on the
> siddhesh/tunables branch.
> 
> I've split this into two patches:
> 
> * The first should enable tunables to initialize without
> calling malloc(), and prior to __environ being set.
> 
> * The second adds a common tunables initialization function
> for elision, and enables it for supported archs.
> 
> I have tested this on PPC64. I need help testing for
> supported s390 and x86 platforms.

I haven't done a thorough review, but the overall approach seems sane.
The only objection I have there is the new macro
TUNABLE_REGISTER_WITH_ENV.  You should be using the
COMPAT_TUNABLE_REGISTER instead and optionally pass it the envp.  I
don't currently have an opinion on whether a new environment variable
for elision is worth having, but that is a separate point that others
will likely have a strong opinion about.  I would only like it to be
separate from the core tunable infrastructure so that it does not
dictate which way the framework goes.

Thanks,
Siddhesh
  
Paul E. Murphy Oct. 6, 2015, 2:09 p.m. UTC | #2
On 10/06/2015 04:22 AM, Siddhesh Poyarekar wrote:
> On Monday 05 October 2015 09:45 PM, Paul E. Murphy wrote:
>> In continuing the glibc tunables discussion, I've made some
>> additional patches which I think would help our work on the
>> siddhesh/tunables branch.
>>
>> I've split this into two patches:
>>
>> * The first should enable tunables to initialize without
>> calling malloc(), and prior to __environ being set.
>>
>> * The second adds a common tunables initialization function
>> for elision, and enables it for supported archs.
>>
>> I have tested this on PPC64. I need help testing for
>> supported s390 and x86 platforms.
> 
> I haven't done a thorough review, but the overall approach seems sane.
> The only objection I have there is the new macro
> TUNABLE_REGISTER_WITH_ENV.  You should be using the
> COMPAT_TUNABLE_REGISTER instead and optionally pass it the envp.  I

My interpretation is that COMPAT_TUNABLE_* macros should only be used
for porting existing tunables with an existing env var.

> don't currently have an opinion on whether a new environment variable
> for elision is worth having, but that is a separate point that others
> will likely have a strong opinion about.  I would only like it to be

I have intentionally avoided explicitly creating new env vars. It has
been made clear by Roland this is unacceptable. The consensus is that
any such change must use the tunables abstraction. The debate is over
on this topic AFAIK.

> separate from the core tunable infrastructure so that it does not
> dictate which way the framework goes.
> 

I agree. The intent of these patches is to validate your existing
work with tunables by using something clearly demanded by the glibc
users.

What work remains on this branch before you open it for debate?


BR,
Paul
  
Siddhesh Poyarekar Oct. 6, 2015, 2:17 p.m. UTC | #3
On Tuesday 06 October 2015 07:39 PM, Paul E. Murphy wrote:
> My interpretation is that COMPAT_TUNABLE_* macros should only be used
> for porting existing tunables with an existing env var.

Ugh, sorry, I misread the code and thought you were introducing an
environment variable.  I see now that you're only introducing a new envp
parameter to tunable_register.  The WITH_ENV is fine for now.  If we go
the environment variable route, we can use that by default since we
always want envp to be available by some means or the other.

> I agree. The intent of these patches is to validate your existing
> work with tunables by using something clearly demanded by the glibc
> users.
> 
> What work remains on this branch before you open it for debate?

I need to split the patches into two sets, one that simply adds an
abstract framework without doing any work and another that opens up the
discussion about what the tunable interface should look like, i.e. one
envvar per variable, one envvar for all, some file, auxval passed by the
kernel, etc.

Siddhesh
  
Andi Kleen Oct. 13, 2015, 12:27 a.m. UTC | #4
On Mon, Oct 05, 2015 at 11:15:51AM -0500, Paul E. Murphy wrote:
> Siddhesh et al,
> 
> In continuing the glibc tunables discussion, I've made some
> additional patches which I think would help our work on the
> siddhesh/tunables branch.
> 
> I've split this into two patches:
> 
> * The first should enable tunables to initialize without
> calling malloc(), and prior to __environ being set.
> 
> * The second adds a common tunables initialization function
> for elision, and enables it for supported archs.
> 
> I have tested this on PPC64. I need help testing for
> supported s390 and x86 platforms.

Sorry for the delay. I built the patch on x86_64. So far
it seems to completely disable lock elision, even when 
explicitly enabling it with GNU_LIBC_TUNABLES=pthread.elision_enable=1

I'll debug later to see why that is.

-Andi
  
Paul E. Murphy Oct. 13, 2015, 2:02 p.m. UTC | #5
On 10/12/2015 07:27 PM, Andi Kleen wrote:
> On Mon, Oct 05, 2015 at 11:15:51AM -0500, Paul E. Murphy wrote:
>> Siddhesh et al,
>>
>> In continuing the glibc tunables discussion, I've made some
>> additional patches which I think would help our work on the
>> siddhesh/tunables branch.
>>
>> I've split this into two patches:
>>
>> * The first should enable tunables to initialize without
>> calling malloc(), and prior to __environ being set.
>>
>> * The second adds a common tunables initialization function
>> for elision, and enables it for supported archs.
>>
>> I have tested this on PPC64. I need help testing for
>> supported s390 and x86 platforms.

On the S390 side of things, I made a refactoring mistake.
"ELISION_SHOULD_ENABLE" should be "ELISION_CAN_ENABLE" if
anyone attempts to test.

> 
> Sorry for the delay. I built the patch on x86_64. So far
> it seems to completely disable lock elision, even when 
> explicitly enabling it with GNU_LIBC_TUNABLES=pthread.elision_enable=1

No worries. Did you try that exact string? It should be the
yet more verbose "glibc.pthread.elision_enable=1" or 0.

Siddhesh, is it necessary to prefix each tunable with "glibc."?
It is implied by the name of the envvar.

> 
> I'll debug later to see why that is.

Much appreciated.

> 
> -Andi
>
  
Siddhesh Poyarekar Oct. 13, 2015, 2:08 p.m. UTC | #6
On Tuesday 13 October 2015 07:32 PM, Paul E. Murphy wrote:
> No worries. Did you try that exact string? It should be the
> yet more verbose "glibc.pthread.elision_enable=1" or 0.
> 
> Siddhesh, is it necessary to prefix each tunable with "glibc."?
> It is implied by the name of the envvar.

Yes, it is necessary - debian could for example put in a debian.foo.bar
tunable that is useful for their distro but is not acceptable upstream.
 That is what the top namespace is for.

Siddhesh
  

Patch

From 97cc7879757c3269613a5026046470f4eda8e9ba Mon Sep 17 00:00:00 2001
From: Paul E. Murphy <murphyp@linux.vnet.ibm.com>
Date: Wed, 23 Sep 2015 15:16:41 -0500
Subject: [PATCH 2/2] Add elision tunables

This patch adds several new tunables to control the behavior of
elision on supported platforms.  This also disables elision
by default on powerpc.

The tunable names are slightly munged to remove architecture
specific terms.  Otherwise, the elision implementation is
roughly identical on all supported platforms.

2015-09-23  Paul E. Murphy  <murphyp@linux.vnet.ibm.com>

	* nptl/elision-tunables.c: New file.
	* sysdeps/unix/sysv/linux/powerpc/elision-conf.c:
	(ELISION_ENABLE): New macro.
	(ELISION_SKIP_LOCK_BUSY): Likewise.
	(ELISION_SKIP_LOCK_INTERNAL_ABORT): Likewise.
	(ELISION_SKIP_LOCK_AFTER_RETRIES): Likewise.
	(ELISION_TRIES): Likewise.
	(ELISION_TRYLOCK_INTERNAL_ABORT): Likewise.
	(ELISION_CAN_ENABLE): Likewise.
	(elision_init): Add hook to tunable function.
	* sysdeps/unix/sysv/linux/s390/elision-conf.c:
	Likewise.
	* sysdeps/unix/sysv/linux/x86/elision-conf.c:
	(ELISION_ENABLE): New macro.
	(ELISION_SKIP_LOCK_BUSY): Likewise.
	(ELISION_SKIP_LOCK_INTERNAL_ABORT): Likewise.
	(ELISION_TRIES): Likewise.
	(ELISION_TRYLOCK_INTERNAL_ABORT): Likewise.
	(ELISION_CAN_ENABLE): Likewise.
	(elision_init): Add hook to tunable function.
	* tunables/tunable-list.h (tunable_register): Regenerate.
	* tunables/tunable.list: Add elision parameters.
---
 nptl/elision-tunables.c                        |   72 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/powerpc/elision-conf.c |   18 +++++-
 sysdeps/unix/sysv/linux/s390/elision-conf.c    |   19 ++++++-
 sysdeps/unix/sysv/linux/x86/elision-conf.c     |   12 ++++
 tunables/tunable-list.h                        |   12 ++++
 tunables/tunables.list                         |    8 +++
 6 files changed, 138 insertions(+), 3 deletions(-)
 create mode 100644 nptl/elision-tunables.c

diff --git a/nptl/elision-tunables.c b/nptl/elision-tunables.c
new file mode 100644
index 0000000..734414c
--- /dev/null
+++ b/nptl/elision-tunables.c
@@ -0,0 +1,72 @@ 
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define TUNABLE_NAMESPACE pthread
+#include <tunables/tunables.h>
+
+/* Define a function to parse an integral tunable value and
+   set the pointer this value.  */
+#define _ELISION_TUNABLE_INT_FUNC(tunable, ptr) \
+  void \
+  _elision_set_ ## tunable (const char * value) \
+  { \
+    *ptr = atoi (value); \
+  }
+
+/* Define some helper macros to register a tunable, and
+   define the setter function.  Note, this defines a
+   nested function, and the tunable macro likely resolves
+   to a function call.  */
+#define _ELISION_TUNABLE_REGISTER(name, mname) \
+  _ELISION_TUNABLE_INT_FUNC (name, mname); \
+  TUNABLE_REGISTER_WITH_ENV (pthread, elision_ ## name, \
+  			     &_elision_set_ ## name, envp)
+
+/* Initialize the tunables based on what the platform has defined as
+   being available.  */
+static void __attribute__((unused))
+elision_init_tunables (char **envp)
+{
+  /* Don't attempt to do anything if elision isn't supported */
+  if (!ELISION_CAN_ENABLE || __libc_enable_secure)
+    return;
+
+  _ELISION_TUNABLE_REGISTER (enable, ELISION_ENABLE);
+
+#ifdef ELISION_SKIP_LOCK_BUSY
+  _ELISION_TUNABLE_REGISTER (skip_lock_busy, ELISION_SKIP_LOCK_BUSY);
+#endif
+
+#ifdef ELISION_SKIP_LOCK_INTERNAL_ABORT
+  _ELISION_TUNABLE_REGISTER (skip_lock_internal_abort,
+  			     ELISION_SKIP_LOCK_INTERNAL_ABORT);
+#endif
+
+#ifdef ELISION_SKIP_LOCK_AFTER_RETRIES
+  _ELISION_TUNABLE_REGISTER (skip_lock_after_retries,
+  			     ELISION_SKIP_LOCK_AFTER_RETRIES);
+#endif
+
+#ifdef ELISION_TRIES
+  _ELISION_TUNABLE_REGISTER (tries, ELISION_TRIES);
+#endif
+
+#ifdef ELISION_SKIP_TRYLOCK_INTERNAL_ABORT
+  _ELISION_TUNABLE_REGISTER (skip_trylock_internal_abort,
+  			     ELISION_SKIP_TRYLOCK_INTERNAL_ABORT);
+#endif
+}
diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
index 5341222..7980ffc 100644
--- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
@@ -22,6 +22,19 @@ 
 #include <unistd.h>
 #include <dl-procinfo.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_SKIP_LOCK_AFTER_RETRIES \
+  (&__elision_aconf.skip_lock_out_of_tbegin_retries)
+#define ELISION_TRIES (&__elision_aconf.try_tbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE ((GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM))
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -60,8 +73,9 @@  elision_init (int argc __attribute__ ((unused)),
 	      char **environ)
 {
 #ifdef ENABLE_LOCK_ELISION
-  int elision_available = (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM) ? 1 : 0;
-  __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
+  /* Note, elision must be explicitly turned on by setting the
+     appropriate tunable on a supported platform.  */
+  elision_init_tunables (environ);
 #endif
   if (!__pthread_force_elision)
     /* Disable elision on rwlocks.  */
diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c b/sysdeps/unix/sysv/linux/s390/elision-conf.c
index e1ff599..4279bcb 100644
--- a/sysdeps/unix/sysv/linux/s390/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c
@@ -22,6 +22,19 @@ 
 #include <unistd.h>
 #include <dl-procinfo.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_SKIP_LOCK_AFTER_RETRIES \
+  (&__elision_aconf.skip_lock_out_of_tbegin_retries)
+#define ELISION_TRIES (&__elision_aconf.try_tbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE (GLRO (dl_hwcap) & HWCAP_S390_TE)
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -62,9 +75,13 @@  elision_init (int argc __attribute__ ((unused)),
 {
   /* Set when the CPU and the kernel supports transactional execution.
      When false elision is never attempted.  */
-  int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
+  int elision_available = ELISION_SHOULD_ENABLE ? 1 : 0;
 
+  /* Enable elision, if available, by default.  */
   __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
+
+  /* Update any tunables as desired.  */
+  elision_init_tunables (environ);
 }
 
 #ifdef SHARED
diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c
index 4a73382..73a0152 100644
--- a/sysdeps/unix/sysv/linux/x86/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c
@@ -22,6 +22,17 @@ 
 #include <elision-conf.h>
 #include <unistd.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_TRIES (&__elision_aconf.retry_try_xbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE (HAS_CPU_FEATURE (RTM))
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -65,6 +76,7 @@  elision_init (int argc __attribute__ ((unused)),
   __elision_available = HAS_CPU_FEATURE (RTM);
 #ifdef ENABLE_LOCK_ELISION
   __pthread_force_elision = __libc_enable_secure ? 0 : __elision_available;
+  elision_init_tunables (environ);
 #endif
   if (!HAS_CPU_FEATURE (RTM))
     __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
diff --git a/tunables/tunable-list.h b/tunables/tunable-list.h
index b4ef72c..5106346 100644
--- a/tunables/tunable-list.h
+++ b/tunables/tunable-list.h
@@ -7,6 +7,12 @@ 
 
 typedef enum
 {
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_tries),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_trylock_internal_abort),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_enable),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_busy),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_internal_abort),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_after_retries),
   TUNABLE_ENUM_NAME(glibc, malloc, trim_threshold),
   TUNABLE_ENUM_NAME(glibc, malloc, mmap_max),
   TUNABLE_ENUM_NAME(glibc, malloc, arena_max),
@@ -19,6 +25,12 @@  typedef enum
 
 #ifdef TUNABLES_INTERNAL
 static tunable_t tunable_list[] = {
+  {TUNABLE_NAME_S(glibc, pthread, elision_tries), NULL, 27, false, {0}},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_trylock_internal_abort), NULL, 49, false, {0}},
+  {TUNABLE_NAME_S(glibc, pthread, elision_enable), NULL, 28, false, {0}},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_busy), NULL, 36, false, {0}},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_internal_abort), NULL, 46, false, {0}},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_after_retries), NULL, 45, false, {0}},
   {TUNABLE_NAME_S(glibc, malloc, trim_threshold), NULL, 27, false, {0}},
   {TUNABLE_NAME_S(glibc, malloc, mmap_max), NULL, 21, false, {0}},
   {TUNABLE_NAME_S(glibc, malloc, arena_max), NULL, 22, false, {0}},
diff --git a/tunables/tunables.list b/tunables/tunables.list
index f1335cc..d7a677b 100644
--- a/tunables/tunables.list
+++ b/tunables/tunables.list
@@ -9,4 +9,12 @@  glibc {
     arena_max
     arena_test
   }
+  pthread {
+    elision_enable
+    elision_skip_lock_busy
+    elision_skip_lock_internal_abort
+    elision_skip_lock_after_retries
+    elision_tries
+    elision_skip_trylock_internal_abort
+  }
 }
-- 
1.7.1