Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700718 > unrolled thread

[PATCH v2 0/4] Working around CPU hotplug and static keys locking

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] Working around CPU hotplug and static keys locking Marc Zyngier <marc.zyngier@arm.com> - 2017-08-01 10:10 +0200
    [PATCH v2 2/4] jump_label: Split out code under the hotplug lock Marc Zyngier <marc.zyngier@arm.com> - 2017-08-01 10:10 +0200
      [tip:locking/core] jump_label: Split out code under the hotplug  lock tip-bot for Marc Zyngier <tipbot@zytor.com> - 2017-08-10 14:20 +0200
    [PATCH v2 3/4] jump_label: Provide hotplug context variants Marc Zyngier <marc.zyngier@arm.com> - 2017-08-01 10:10 +0200
      Re: [PATCH v2 3/4] jump_label: Provide hotplug context variants Peter Zijlstra <peterz@infradead.org> - 2017-08-02 16:40 +0200
        Re: [PATCH v2 3/4] jump_label: Provide hotplug context variants Marc Zyngier <marc.zyngier@arm.com> - 2017-08-02 17:10 +0200
      [tip:locking/core] jump_label: Provide hotplug context variants tip-bot for Marc Zyngier <tipbot@zytor.com> - 2017-08-10 14:20 +0200
    [PATCH v2 4/4] clocksource/arm_arch_timer: Use static_branch_enable_cpuslocked() Marc Zyngier <marc.zyngier@arm.com> - 2017-08-01 10:10 +0200
      [tip:locking/core] clocksource/arm_arch_timer: Use  static_branch_enable_cpuslocked() tip-bot for Marc Zyngier <tipbot@zytor.com> - 2017-08-10 14:20 +0200

#1700718 — [PATCH v2 0/4] Working around CPU hotplug and static keys locking

Since f2545b2d4ce1 ("jump_label: Reorder hotplug lock and
jump_label_lock"), it has become impossible to switch a static key
from a CPU hotplug notifier:

- On the primary CPU, cpu_hotplug_lock is taken by
  __cpuhp_setup_state(), and then again by static_key_slow_inc(). The
  lock being taken as a reader, so it is OK so far.

- On a secondary CPU, _cpu_up takes the lock *as a writer* on the boot
  CPU, and the secondary tries to switch the static key, taking the
  lock as well (as a reader). In that case, we're toasted.

I couldn't find an elegant solution to this, so this series works
around the issue in the most disgusting way, adding a _nolock version
of the static key API to be used in CPU hotplug situations.

The last patch uses this API to work around the issue that Leo
reported, where the static key flipped on a secondary CPU brings the
box down in flames.

Marc Zyngier (4):
  jump_label: Move cpu hotplug locking
  jump_label: Split out code under the hotplug lock
  jump_label: Provide hotplug context variants
  clocksource/arm_arch_timer: Use static_branch_enable_cpuslocked()

 Documentation/static-keys.txt        | 15 ++++++++++++
 drivers/clocksource/arm_arch_timer.c |  6 ++++-
 include/linux/jump_label.h           | 11 +++++++--
 kernel/jump_label.c                  | 44 +++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1700719 — [PATCH v2 2/4] jump_label: Split out code under the hotplug lock

In order to later introduce an "already locked" version of some
of the static key funcions, let's split the code into the core stuff
(the *_cpuslocked functions) and the usual helpers, which now
take/release the hotplug lock and call into the _cpuslocked
versions.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 kernel/jump_label.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index f11b10091100..0ab8b35daa54 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -101,11 +101,10 @@ void static_key_disable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-void static_key_slow_inc(struct static_key *key)
+static void static_key_slow_inc_cpuslocked(struct static_key *key)
 {
 	int v, v1;
 
-	cpus_read_lock();
 	STATIC_KEY_CHECK_USE();
 
 	/*
@@ -122,10 +121,8 @@ void static_key_slow_inc(struct static_key *key)
 	 */
 	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
 		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
-		if (likely(v1 == v)) {
-			cpus_read_unlock();
+		if (likely(v1 == v))
 			return;
-		}
 	}
 
 	jump_label_lock();
@@ -137,14 +134,20 @@ void static_key_slow_inc(struct static_key *key)
 		atomic_inc(&key->enabled);
 	}
 	jump_label_unlock();
+}
+
+void static_key_slow_inc(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_slow_inc_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
-static void __static_key_slow_dec(struct static_key *key,
-		unsigned long rate_limit, struct delayed_work *work)
+static void static_key_slow_dec_cpuslocked(struct static_key *key,
+					   unsigned long rate_limit,
+					   struct delayed_work *work)
 {
-	cpus_read_lock();
 	/*
 	 * The negative count check is valid even when a negative
 	 * key->enabled is in use by static_key_slow_inc(); a
@@ -155,7 +158,6 @@ static void __static_key_slow_dec(struct static_key *key,
 	if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
 		WARN(atomic_read(&key->enabled) < 0,
 		     "jump label: negative count!\n");
-		cpus_read_unlock();
 		return;
 	}
 
@@ -166,6 +168,14 @@ static void __static_key_slow_dec(struct static_key *key,
 		jump_label_update(key);
 	}
 	jump_label_unlock();
+}
+
+static void __static_key_slow_dec(struct static_key *key,
+				  unsigned long rate_limit,
+				  struct delayed_work *work)
+{
+	cpus_read_lock();
+	static_key_slow_dec_cpuslocked(key, rate_limit, work);
 	cpus_read_unlock();
 }
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1708513 — [tip:locking/core] jump_label: Split out code under the hotplug lock

Commit-ID:  8b7b412807053ab5f059ffae426a280e769a5bda
Gitweb:     http://git.kernel.org/tip/8b7b412807053ab5f059ffae426a280e769a5bda
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Tue, 1 Aug 2017 09:02:55 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Aug 2017 12:28:58 +0200

jump_label: Split out code under the hotplug lock

In order to later introduce an "already locked" version of some
of the static key funcions, let's split the code into the core stuff
(the *_cpuslocked functions) and the usual helpers, which now
take/release the hotplug lock and call into the _cpuslocked
versions.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20170801080257.5056-3-marc.zyngier@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/jump_label.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 161301f..cc6d815 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -79,11 +79,10 @@ int static_key_count(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_count);
 
-void static_key_slow_inc(struct static_key *key)
+static void static_key_slow_inc_cpuslocked(struct static_key *key)
 {
 	int v, v1;
 
-	cpus_read_lock();
 	STATIC_KEY_CHECK_USE();
 
 	/*
@@ -100,10 +99,8 @@ void static_key_slow_inc(struct static_key *key)
 	 */
 	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
 		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
-		if (likely(v1 == v)) {
-			cpus_read_unlock();
+		if (likely(v1 == v))
 			return;
-		}
 	}
 
 	jump_label_lock();
@@ -119,6 +116,12 @@ void static_key_slow_inc(struct static_key *key)
 		atomic_inc(&key->enabled);
 	}
 	jump_label_unlock();
+}
+
+void static_key_slow_inc(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_slow_inc_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -163,10 +166,10 @@ void static_key_disable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-static void __static_key_slow_dec(struct static_key *key,
-		unsigned long rate_limit, struct delayed_work *work)
+static void static_key_slow_dec_cpuslocked(struct static_key *key,
+					   unsigned long rate_limit,
+					   struct delayed_work *work)
 {
-	cpus_read_lock();
 	/*
 	 * The negative count check is valid even when a negative
 	 * key->enabled is in use by static_key_slow_inc(); a
@@ -177,7 +180,6 @@ static void __static_key_slow_dec(struct static_key *key,
 	if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
 		WARN(atomic_read(&key->enabled) < 0,
 		     "jump label: negative count!\n");
-		cpus_read_unlock();
 		return;
 	}
 
@@ -188,6 +190,14 @@ static void __static_key_slow_dec(struct static_key *key,
 		jump_label_update(key);
 	}
 	jump_label_unlock();
+}
+
+static void __static_key_slow_dec(struct static_key *key,
+				  unsigned long rate_limit,
+				  struct delayed_work *work)
+{
+	cpus_read_lock();
+	static_key_slow_dec_cpuslocked(key, rate_limit, work);
 	cpus_read_unlock();
 }
 

[toc] | [prev] | [next] | [standalone]

#1700721 — [PATCH v2 3/4] jump_label: Provide hotplug context variants

As using the normal static key API under the hotplug lock is
pretty much impossible, let's provide a variant of some of them
that require the hotplug lock to have already been taken.

These function are only meant to be used in CPU hotplug callbacks.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/static-keys.txt | 15 +++++++++++++++
 include/linux/jump_label.h    | 11 +++++++++--
 kernel/jump_label.c           | 20 ++++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt
index b83dfa1c0602..6d0a181e74d2 100644
--- a/Documentation/static-keys.txt
+++ b/Documentation/static-keys.txt
@@ -149,6 +149,21 @@ static_branch_inc(), will change the branch back to true. Likewise, if the
 key is initialized false, a 'static_branch_inc()', will change the branch to
 true. And then a 'static_branch_dec()', will again make the branch false.
 
+Note that switching branches results in some locks being taken,
+particularly the CPU hotplug lock (in order to avoid races against
+CPUs being brought in the kernel whilst the kernel is getting
+patched). Calling the static key API from within a hotplug notifier is
+thus a sure deadlock recipe. In order to still allow use of the
+functionnality, the following functions are provided:
+
+	static_key_enable_cpuslocked()
+	static_key_disable_cpuslocked()
+	static_branch_enable_cpuslocked()
+	static_branch_disable_cpuslocked()
+
+These functions are *not* general purpose, and must only be used when
+you really know that you're in the above context, and no other.
+
 Where an array of keys is required, it can be defined as::
 
 	DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 2afd74b9d844..c5e5d2ffce58 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -163,6 +163,8 @@ extern void jump_label_apply_nops(struct module *mod);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
 extern void static_key_disable(struct static_key *key);
+extern void static_key_enable_cpuslocked(struct static_key *key);
+extern void static_key_disable_cpuslocked(struct static_key *key);
 
 /*
  * We should be using ATOMIC_INIT() for initializing .enabled, but
@@ -252,6 +254,9 @@ static inline void static_key_disable(struct static_key *key)
 		static_key_slow_dec(key);
 }
 
+#define static_key_enable_cpuslocked(k)		static_key_enable((k))
+#define static_key_disable_cpuslocked(k)	static_key_disable((k))
+
 #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
 #define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
 
@@ -413,8 +418,10 @@ extern bool ____wrong_branch_error(void);
  * Normal usage; boolean enable/disable.
  */
 
-#define static_branch_enable(x)		static_key_enable(&(x)->key)
-#define static_branch_disable(x)	static_key_disable(&(x)->key)
+#define static_branch_enable(x)			static_key_enable(&(x)->key)
+#define static_branch_disable(x)		static_key_disable(&(x)->key)
+#define static_branch_enable_cpuslocked(x)	static_key_enable_cpuslocked(&(x)->key)
+#define static_branch_disable_cpuslocked(x)	static_key_disable_cpuslocked(&(x)->key)
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 0ab8b35daa54..4e7fa067ca17 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -170,6 +170,26 @@ static void static_key_slow_dec_cpuslocked(struct static_key *key,
 	jump_label_unlock();
 }
 
+void static_key_enable_cpuslocked(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (!count)
+		static_key_slow_inc_cpuslocked(key);
+}
+
+void static_key_disable_cpuslocked(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (count)
+		static_key_slow_dec_cpuslocked(key, 0, NULL);
+}
+
 static void __static_key_slow_dec(struct static_key *key,
 				  unsigned long rate_limit,
 				  struct delayed_work *work)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1702166 — Re: [PATCH v2 3/4] jump_label: Provide hotplug context variants

On Tue, Aug 01, 2017 at 09:02:56AM +0100, Marc Zyngier wrote:
> As using the normal static key API under the hotplug lock is
> pretty much impossible, let's provide a variant of some of them
> that require the hotplug lock to have already been taken.
> 
> These function are only meant to be used in CPU hotplug callbacks.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/static-keys.txt | 15 +++++++++++++++
>  include/linux/jump_label.h    | 11 +++++++++--
>  kernel/jump_label.c           | 20 ++++++++++++++++++++
>  3 files changed, 44 insertions(+), 2 deletions(-)

I stuffed them in my locking/core branch on top of Paolo's patches:

  https://lkml.kernel.org/r/1501601046-35683-1-git-send-email-pbonzini@redhat.com

This patch now looks like so.

---
Subject: jump_label: Provide hotplug context variants
From:   Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 1 Aug 2017 09:02:56 +0100

As using the normal static key API under the hotplug lock is
pretty much impossible, let's provide a variant of some of them
that require the hotplug lock to have already been taken.

These function are only meant to be used in CPU hotplug callbacks.

Cc: linux-arm-kernel@lists.infradead.org
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20170801080257.5056-4-marc.zyngier@arm.com
---
 Documentation/static-keys.txt |   15 +++++++++++++++
 include/linux/jump_label.h    |   11 +++++++++--
 kernel/jump_label.c           |   22 ++++++++++++++++++----
 3 files changed, 42 insertions(+), 6 deletions(-)

--- a/Documentation/static-keys.txt
+++ b/Documentation/static-keys.txt
@@ -154,6 +154,21 @@ and 'static_key_count()'.  In general, i
 should be protected with the same mutex used around the enable/disable
 or increment/decrement function.
 
+Note that switching branches results in some locks being taken,
+particularly the CPU hotplug lock (in order to avoid races against
+CPUs being brought in the kernel whilst the kernel is getting
+patched). Calling the static key API from within a hotplug notifier is
+thus a sure deadlock recipe. In order to still allow use of the
+functionnality, the following functions are provided:
+
+	static_key_enable_cpuslocked()
+	static_key_disable_cpuslocked()
+	static_branch_enable_cpuslocked()
+	static_branch_disable_cpuslocked()
+
+These functions are *not* general purpose, and must only be used when
+you really know that you're in the above context, and no other.
+
 Where an array of keys is required, it can be defined as::
 
 	DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -163,6 +163,8 @@ extern void jump_label_apply_nops(struct
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
 extern void static_key_disable(struct static_key *key);
+extern void static_key_enable_cpuslocked(struct static_key *key);
+extern void static_key_disable_cpuslocked(struct static_key *key);
 
 /*
  * We should be using ATOMIC_INIT() for initializing .enabled, but
@@ -254,6 +256,9 @@ static inline void static_key_disable(st
 	atomic_set(&key->enabled, 0);
 }
 
+#define static_key_enable_cpuslocked(k)		static_key_enable((k))
+#define static_key_disable_cpuslocked(k)	static_key_disable((k))
+
 #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
 #define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
 
@@ -415,8 +420,10 @@ extern bool ____wrong_branch_error(void)
  * Normal usage; boolean enable/disable.
  */
 
-#define static_branch_enable(x)		static_key_enable(&(x)->key)
-#define static_branch_disable(x)	static_key_disable(&(x)->key)
+#define static_branch_enable(x)			static_key_enable(&(x)->key)
+#define static_branch_disable(x)		static_key_disable(&(x)->key)
+#define static_branch_enable_cpuslocked(x)	static_key_enable_cpuslocked(&(x)->key)
+#define static_branch_disable_cpuslocked(x)	static_key_disable_cpuslocked(&(x)->key)
 
 #endif /* __ASSEMBLY__ */
 
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -126,15 +126,15 @@ void static_key_slow_inc(struct static_k
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
-void static_key_enable(struct static_key *key)
+void static_key_enable_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
+
 	if (atomic_read(&key->enabled) > 0) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
 		return;
 	}
 
-	cpus_read_lock();
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
 		atomic_set(&key->enabled, -1);
@@ -145,23 +145,37 @@ void static_key_enable(struct static_key
 		atomic_set_release(&key->enabled, 1);
 	}
 	jump_label_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
+
+void static_key_enable(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_enable_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_enable);
 
-void static_key_disable(struct static_key *key)
+void static_key_disable_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
+
 	if (atomic_read(&key->enabled) != 1) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
 		return;
 	}
 
-	cpus_read_lock();
 	jump_label_lock();
 	if (atomic_cmpxchg(&key->enabled, 1, 0))
 		jump_label_update(key);
 	jump_label_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);
+
+void static_key_disable(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_disable_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_disable);

[toc] | [prev] | [next] | [standalone]

#1702185 — Re: [PATCH v2 3/4] jump_label: Provide hotplug context variants

On 02/08/17 15:37, Peter Zijlstra wrote:
> On Tue, Aug 01, 2017 at 09:02:56AM +0100, Marc Zyngier wrote:
>> As using the normal static key API under the hotplug lock is
>> pretty much impossible, let's provide a variant of some of them
>> that require the hotplug lock to have already been taken.
>>
>> These function are only meant to be used in CPU hotplug callbacks.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  Documentation/static-keys.txt | 15 +++++++++++++++
>>  include/linux/jump_label.h    | 11 +++++++++--
>>  kernel/jump_label.c           | 20 ++++++++++++++++++++
>>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> I stuffed them in my locking/core branch on top of Paolo's patches:
> 
>   https://lkml.kernel.org/r/1501601046-35683-1-git-send-email-pbonzini@redhat.com
> 
> This patch now looks like so.
> 
> ---
> Subject: jump_label: Provide hotplug context variants
> From:   Marc Zyngier <marc.zyngier@arm.com>
> Date: Tue, 1 Aug 2017 09:02:56 +0100
> 
> As using the normal static key API under the hotplug lock is
> pretty much impossible, let's provide a variant of some of them
> that require the hotplug lock to have already been taken.
> 
> These function are only meant to be used in CPU hotplug callbacks.
> 
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/r/20170801080257.5056-4-marc.zyngier@arm.com
> ---
>  Documentation/static-keys.txt |   15 +++++++++++++++
>  include/linux/jump_label.h    |   11 +++++++++--
>  kernel/jump_label.c           |   22 ++++++++++++++++++----
>  3 files changed, 42 insertions(+), 6 deletions(-)
> 
> --- a/Documentation/static-keys.txt
> +++ b/Documentation/static-keys.txt
> @@ -154,6 +154,21 @@ and 'static_key_count()'.  In general, i
>  should be protected with the same mutex used around the enable/disable
>  or increment/decrement function.
>  
> +Note that switching branches results in some locks being taken,
> +particularly the CPU hotplug lock (in order to avoid races against
> +CPUs being brought in the kernel whilst the kernel is getting
> +patched). Calling the static key API from within a hotplug notifier is
> +thus a sure deadlock recipe. In order to still allow use of the
> +functionnality, the following functions are provided:
> +
> +	static_key_enable_cpuslocked()
> +	static_key_disable_cpuslocked()
> +	static_branch_enable_cpuslocked()
> +	static_branch_disable_cpuslocked()
> +
> +These functions are *not* general purpose, and must only be used when
> +you really know that you're in the above context, and no other.
> +
>  Where an array of keys is required, it can be defined as::
>  
>  	DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -163,6 +163,8 @@ extern void jump_label_apply_nops(struct
>  extern int static_key_count(struct static_key *key);
>  extern void static_key_enable(struct static_key *key);
>  extern void static_key_disable(struct static_key *key);
> +extern void static_key_enable_cpuslocked(struct static_key *key);
> +extern void static_key_disable_cpuslocked(struct static_key *key);
>  
>  /*
>   * We should be using ATOMIC_INIT() for initializing .enabled, but
> @@ -254,6 +256,9 @@ static inline void static_key_disable(st
>  	atomic_set(&key->enabled, 0);
>  }
>  
> +#define static_key_enable_cpuslocked(k)		static_key_enable((k))
> +#define static_key_disable_cpuslocked(k)	static_key_disable((k))
> +
>  #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
>  #define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
>  
> @@ -415,8 +420,10 @@ extern bool ____wrong_branch_error(void)
>   * Normal usage; boolean enable/disable.
>   */
>  
> -#define static_branch_enable(x)		static_key_enable(&(x)->key)
> -#define static_branch_disable(x)	static_key_disable(&(x)->key)
> +#define static_branch_enable(x)			static_key_enable(&(x)->key)
> +#define static_branch_disable(x)		static_key_disable(&(x)->key)
> +#define static_branch_enable_cpuslocked(x)	static_key_enable_cpuslocked(&(x)->key)
> +#define static_branch_disable_cpuslocked(x)	static_key_disable_cpuslocked(&(x)->key)
>  
>  #endif /* __ASSEMBLY__ */
>  
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -126,15 +126,15 @@ void static_key_slow_inc(struct static_k
>  }
>  EXPORT_SYMBOL_GPL(static_key_slow_inc);
>  
> -void static_key_enable(struct static_key *key)
> +void static_key_enable_cpuslocked(struct static_key *key)
>  {
>  	STATIC_KEY_CHECK_USE();
> +
>  	if (atomic_read(&key->enabled) > 0) {
>  		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
>  		return;
>  	}
>  
> -	cpus_read_lock();
>  	jump_label_lock();
>  	if (atomic_read(&key->enabled) == 0) {
>  		atomic_set(&key->enabled, -1);
> @@ -145,23 +145,37 @@ void static_key_enable(struct static_key
>  		atomic_set_release(&key->enabled, 1);
>  	}
>  	jump_label_unlock();
> +}
> +EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
> +
> +void static_key_enable(struct static_key *key)
> +{
> +	cpus_read_lock();
> +	static_key_enable_cpuslocked(key);
>  	cpus_read_unlock();
>  }
>  EXPORT_SYMBOL_GPL(static_key_enable);
>  
> -void static_key_disable(struct static_key *key)
> +void static_key_disable_cpuslocked(struct static_key *key)
>  {
>  	STATIC_KEY_CHECK_USE();
> +
>  	if (atomic_read(&key->enabled) != 1) {
>  		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
>  		return;
>  	}
>  
> -	cpus_read_lock();
>  	jump_label_lock();
>  	if (atomic_cmpxchg(&key->enabled, 1, 0))
>  		jump_label_update(key);
>  	jump_label_unlock();
> +}
> +EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);
> +
> +void static_key_disable(struct static_key *key)
> +{
> +	cpus_read_lock();
> +	static_key_disable_cpuslocked(key);
>  	cpus_read_unlock();
>  }
>  EXPORT_SYMBOL_GPL(static_key_disable);
> 

I've just tried this one of the affected systems, and it booted just
fine. So thumbs up from me.

Thanks again,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1708511 — [tip:locking/core] jump_label: Provide hotplug context variants

Commit-ID:  5a40527f8f0798553764fc8db4111d7d9c33ea51
Gitweb:     http://git.kernel.org/tip/5a40527f8f0798553764fc8db4111d7d9c33ea51
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Tue, 1 Aug 2017 09:02:56 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Aug 2017 12:28:59 +0200

jump_label: Provide hotplug context variants

As using the normal static key API under the hotplug lock is
pretty much impossible, let's provide a variant of some of them
that require the hotplug lock to have already been taken.

These function are only meant to be used in CPU hotplug callbacks.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20170801080257.5056-4-marc.zyngier@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/static-keys.txt | 15 +++++++++++++++
 include/linux/jump_label.h    | 11 +++++++++--
 kernel/jump_label.c           | 22 ++++++++++++++++++----
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt
index 870b4be..ab16efe 100644
--- a/Documentation/static-keys.txt
+++ b/Documentation/static-keys.txt
@@ -154,6 +154,21 @@ and 'static_key_count()'.  In general, if you use these functions, they
 should be protected with the same mutex used around the enable/disable
 or increment/decrement function.
 
+Note that switching branches results in some locks being taken,
+particularly the CPU hotplug lock (in order to avoid races against
+CPUs being brought in the kernel whilst the kernel is getting
+patched). Calling the static key API from within a hotplug notifier is
+thus a sure deadlock recipe. In order to still allow use of the
+functionnality, the following functions are provided:
+
+	static_key_enable_cpuslocked()
+	static_key_disable_cpuslocked()
+	static_branch_enable_cpuslocked()
+	static_branch_disable_cpuslocked()
+
+These functions are *not* general purpose, and must only be used when
+you really know that you're in the above context, and no other.
+
 Where an array of keys is required, it can be defined as::
 
 	DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 740a42e..cd58616 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -163,6 +163,8 @@ extern void jump_label_apply_nops(struct module *mod);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
 extern void static_key_disable(struct static_key *key);
+extern void static_key_enable_cpuslocked(struct static_key *key);
+extern void static_key_disable_cpuslocked(struct static_key *key);
 
 /*
  * We should be using ATOMIC_INIT() for initializing .enabled, but
@@ -254,6 +256,9 @@ static inline void static_key_disable(struct static_key *key)
 	atomic_set(&key->enabled, 0);
 }
 
+#define static_key_enable_cpuslocked(k)		static_key_enable((k))
+#define static_key_disable_cpuslocked(k)	static_key_disable((k))
+
 #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
 #define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
 
@@ -415,8 +420,10 @@ extern bool ____wrong_branch_error(void);
  * Normal usage; boolean enable/disable.
  */
 
-#define static_branch_enable(x)		static_key_enable(&(x)->key)
-#define static_branch_disable(x)	static_key_disable(&(x)->key)
+#define static_branch_enable(x)			static_key_enable(&(x)->key)
+#define static_branch_disable(x)		static_key_disable(&(x)->key)
+#define static_branch_enable_cpuslocked(x)	static_key_enable_cpuslocked(&(x)->key)
+#define static_branch_disable_cpuslocked(x)	static_key_disable_cpuslocked(&(x)->key)
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index cc6d815..0bf2e8f5 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -126,15 +126,15 @@ void static_key_slow_inc(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
-void static_key_enable(struct static_key *key)
+void static_key_enable_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
+
 	if (atomic_read(&key->enabled) > 0) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
 		return;
 	}
 
-	cpus_read_lock();
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
 		atomic_set(&key->enabled, -1);
@@ -145,23 +145,37 @@ void static_key_enable(struct static_key *key)
 		atomic_set_release(&key->enabled, 1);
 	}
 	jump_label_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
+
+void static_key_enable(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_enable_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_enable);
 
-void static_key_disable(struct static_key *key)
+void static_key_disable_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
+
 	if (atomic_read(&key->enabled) != 1) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
 		return;
 	}
 
-	cpus_read_lock();
 	jump_label_lock();
 	if (atomic_cmpxchg(&key->enabled, 1, 0))
 		jump_label_update(key);
 	jump_label_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);
+
+void static_key_disable(struct static_key *key)
+{
+	cpus_read_lock();
+	static_key_disable_cpuslocked(key);
 	cpus_read_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_disable);

[toc] | [prev] | [next] | [standalone]

#1700723 — [PATCH v2 4/4] clocksource/arm_arch_timer: Use static_branch_enable_cpuslocked()

Use the new static_branch_enable_cpuslocked function to switch
the workaround static key on the CPU hotplug path.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index aae87c4c546e..c62e71614c75 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -455,7 +455,11 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
 			per_cpu(timer_unstable_counter_workaround, i) = wa;
 	}
 
-	static_branch_enable(&arch_timer_read_ool_enabled);
+	/*
+	 * Use the locked version, as we're called from the CPU
+	 * hotplug framework. Otherwise, we end-up in deadlock-land.
+	 */
+	static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
 
 	/*
 	 * Don't use the vdso fastpath if errata require using the
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1708522 — [tip:locking/core] clocksource/arm_arch_timer: Use static_branch_enable_cpuslocked()

Commit-ID:  450f9689f294c331c56ec37d68302ccc19c7caa2
Gitweb:     http://git.kernel.org/tip/450f9689f294c331c56ec37d68302ccc19c7caa2
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Tue, 1 Aug 2017 09:02:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Aug 2017 12:29:00 +0200

clocksource/arm_arch_timer: Use static_branch_enable_cpuslocked()

Use the new static_branch_enable_cpuslocked() function to switch
the workaround static key on the CPU hotplug path.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20170801080257.5056-5-marc.zyngier@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/clocksource/arm_arch_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index aae87c4..c62e716 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -455,7 +455,11 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
 			per_cpu(timer_unstable_counter_workaround, i) = wa;
 	}
 
-	static_branch_enable(&arch_timer_read_ool_enabled);
+	/*
+	 * Use the locked version, as we're called from the CPU
+	 * hotplug framework. Otherwise, we end-up in deadlock-land.
+	 */
+	static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
 
 	/*
 	 * Don't use the vdso fastpath if errata require using the

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web