[PATCH 0/3] locking/static_key: improve rate limited labels

[PATCH 0/3] locking/static_key: improve rate limited labels

[Jakub Kicinski]

Hi!

This will be used to fix the static branch disabling in the TLS
code.  The net/tls/ code should be using the deferred static
branch type, because unprivileged users can flip the branch
on and off quite easily with CONFIG_TLS_DEVICE=y.

Second of all we shouldn't take the jump label locks from
the RX path, when the socket is destroyed.  This can be avoided
with some slight code refactoring in deferred static_key as
it already runs from a workqueue.

This the series (and a simple tls patch which makes use of it)
applied opening 0.5M TLS connections to localhost (just calling
setsockopt, no data exchange) goes down from 37.9s to 12.4s.

Jakub Kicinski (3):
  locking/static_key: add support for deferred static branches
  locking/static_key: factor out the fast path of static_key_slow_dec()
  locking/static_key: don't take sleeping locks in
    __static_key_slow_dec_deferred()

 include/linux/jump_label_ratelimit.h | 64 ++++++++++++++++++++++++++--
 kernel/jump_label.c                  | 64 +++++++++++++++-------------
 2 files changed, 95 insertions(+), 33 deletions(-)

-- 
2.21.0

[PATCH 1/3] locking/static_key: add support for deferred static branches

[Jakub Kicinski]

Add deferred static branches.  We can't unfortunately use the
nice trick of encapsulating the entire structure in true/false
variants, because the inside has to be either struct static_key_true
or struct static_key_false.  Use defines to pass the appropriate
members to the helpers separately.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/linux/jump_label_ratelimit.h | 64 ++++++++++++++++++++++++++--
 kernel/jump_label.c                  | 17 +++++---
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h
index a49f2b45b3f0..42710d5949ba 100644
--- a/include/linux/jump_label_ratelimit.h
+++ b/include/linux/jump_label_ratelimit.h
@@ -12,21 +12,79 @@ struct static_key_deferred {
 	struct delayed_work work;
 };
 
-extern void static_key_slow_dec_deferred(struct static_key_deferred *key);
-extern void static_key_deferred_flush(struct static_key_deferred *key);
+struct static_key_true_deferred {
+	struct static_key_true key;
+	unsigned long timeout;
+	struct delayed_work work;
+};
+
+struct static_key_false_deferred {
+	struct static_key_false key;
+	unsigned long timeout;
+	struct delayed_work work;
+};
+
+#define static_key_slow_dec_deferred(x)					\
+	__static_key_slow_dec_deferred(&(x)->key, &(x)->work, (x)->timeout)
+#define static_branch_slow_dec_deferred(x)				\
+	__static_key_slow_dec_deferred(&(x)->key.key, &(x)->work, (x)->timeout)
+
+#define static_key_deferred_flush(x)					\
+	__static_key_deferred_flush((x), &(x)->work)
+
+extern void
+__static_key_slow_dec_deferred(struct static_key *key,
+			       struct delayed_work *work,
+			       unsigned long timeout);
+extern void __static_key_deferred_flush(void *key, struct delayed_work *work);
 extern void
 jump_label_rate_limit(struct static_key_deferred *key, unsigned long rl);
 
+extern void jump_label_update_timeout(struct work_struct *work);
+
+#define DEFINE_STATIC_KEY_DEFERRED_TRUE(name, rl)			\
+	struct static_key_true_deferred name = {			\
+		.key =		{ STATIC_KEY_INIT_TRUE },		\
+		.timeout =	(rl),					\
+		.work =	__DELAYED_WORK_INITIALIZER((name).work,		\
+						   jump_label_update_timeout, \
+						   0),			\
+	}
+
+#define DEFINE_STATIC_KEY_DEFERRED_FALSE(name, rl)			\
+	struct static_key_false_deferred name = {			\
+		.key =		{ STATIC_KEY_INIT_FALSE },		\
+		.timeout =	(rl),					\
+		.work =	__DELAYED_WORK_INITIALIZER((name).work,		\
+						   jump_label_update_timeout, \
+						   0),			\
+	}
+
+#define static_branch_deferred_inc(x)	static_branch_inc(&(x)->key)
+
 #else	/* !CONFIG_JUMP_LABEL */
 struct static_key_deferred {
 	struct static_key  key;
 };
+struct static_key_true_deferred {
+	struct static_key_true key;
+};
+struct static_key_false_deferred {
+	struct static_key_false key;
+};
+#define DEFINE_STATIC_KEY_DEFERRED_TRUE(name, rl)	\
+	struct static_key_true_deferred name = { STATIC_KEY_TRUE_INIT }
+#define DEFINE_STATIC_KEY_DEFERRED_FALSE(name, rl)	\
+	struct static_key_false_deferred name = { STATIC_KEY_FALSE_INIT }
+
+#define static_branch_slow_dec_deferred(x)	static_branch_dec(&(x)->key)
+
 static inline void static_key_slow_dec_deferred(struct static_key_deferred *key)
 {
 	STATIC_KEY_CHECK_USE(key);
 	static_key_slow_dec(&key->key);
 }
-static inline void static_key_deferred_flush(struct static_key_deferred *key)
+static inline void static_key_deferred_flush(void *key)
 {
 	STATIC_KEY_CHECK_USE(key);
 }
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index a799b1ac6b2f..73bbbaddbd9c 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -244,12 +244,13 @@ static void __static_key_slow_dec(struct static_key *key,
 	cpus_read_unlock();
 }
 
-static void jump_label_update_timeout(struct work_struct *work)
+void jump_label_update_timeout(struct work_struct *work)
 {
 	struct static_key_deferred *key =
 		container_of(work, struct static_key_deferred, work.work);
 	__static_key_slow_dec(&key->key, 0, NULL);
 }
+EXPORT_SYMBOL_GPL(jump_label_update_timeout);
 
 void static_key_slow_dec(struct static_key *key)
 {
@@ -264,19 +265,21 @@ void static_key_slow_dec_cpuslocked(struct static_key *key)
 	__static_key_slow_dec_cpuslocked(key, 0, NULL);
 }
 
-void static_key_slow_dec_deferred(struct static_key_deferred *key)
+void __static_key_slow_dec_deferred(struct static_key *key,
+				    struct delayed_work *work,
+				    unsigned long timeout)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(&key->key, key->timeout, &key->work);
+	__static_key_slow_dec(key, timeout, work);
 }
-EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred);
+EXPORT_SYMBOL_GPL(__static_key_slow_dec_deferred);
 
-void static_key_deferred_flush(struct static_key_deferred *key)
+void __static_key_deferred_flush(void *key, struct delayed_work *work)
 {
 	STATIC_KEY_CHECK_USE(key);
-	flush_delayed_work(&key->work);
+	flush_delayed_work(work);
 }
-EXPORT_SYMBOL_GPL(static_key_deferred_flush);
+EXPORT_SYMBOL_GPL(__static_key_deferred_flush);
 
 void jump_label_rate_limit(struct static_key_deferred *key,
 		unsigned long rl)
-- 
2.21.0

[tip:locking/core] locking/static_key: Add support for deferred static branches

[tip-bot for Jakub Kicinski]

Commit-ID:  ad282a8117d5048398f506f20b092c14b3b3c43f
Gitweb:     https://git.kernel.org/tip/ad282a8117d5048398f506f20b092c14b3b3c43f
Author:     Jakub Kicinski <jakub.kicinski@netronome.com>
AuthorDate: Fri, 29 Mar 2019 17:08:52 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 29 Apr 2019 08:29:20 +0200

locking/static_key: Add support for deferred static branches

Add deferred static branches.  We can't unfortunately use the
nice trick of encapsulating the entire structure in true/false
variants, because the inside has to be either struct static_key_true
or struct static_key_false.  Use defines to pass the appropriate
members to the helpers separately.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: alexei.starovoitov@gmail.com
Cc: ard.biesheuvel@linaro.org
Cc: oss-drivers@netronome.com
Cc: yamada.masahiro@socionext.com
Link: https://lkml.kernel.org/r/20190330000854.30142-2-jakub.kicinski@netronome.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/jump_label_ratelimit.h | 64 ++++++++++++++++++++++++++++++++++--
 kernel/jump_label.c                  | 17 ++++++----
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h
index a49f2b45b3f0..42710d5949ba 100644
--- a/include/linux/jump_label_ratelimit.h
+++ b/include/linux/jump_label_ratelimit.h
@@ -12,21 +12,79 @@ struct static_key_deferred {
 	struct delayed_work work;
 };
 
-extern void static_key_slow_dec_deferred(struct static_key_deferred *key);
-extern void static_key_deferred_flush(struct static_key_deferred *key);
+struct static_key_true_deferred {
+	struct static_key_true key;
+	unsigned long timeout;
+	struct delayed_work work;
+};
+
+struct static_key_false_deferred {
+	struct static_key_false key;
+	unsigned long timeout;
+	struct delayed_work work;
+};
+
+#define static_key_slow_dec_deferred(x)					\
+	__static_key_slow_dec_deferred(&(x)->key, &(x)->work, (x)->timeout)
+#define static_branch_slow_dec_deferred(x)				\
+	__static_key_slow_dec_deferred(&(x)->key.key, &(x)->work, (x)->timeout)
+
+#define static_key_deferred_flush(x)					\
+	__static_key_deferred_flush((x), &(x)->work)
+
+extern void
+__static_key_slow_dec_deferred(struct static_key *key,
+			       struct delayed_work *work,
+			       unsigned long timeout);
+extern void __static_key_deferred_flush(void *key, struct delayed_work *work);
 extern void
 jump_label_rate_limit(struct static_key_deferred *key, unsigned long rl);
 
+extern void jump_label_update_timeout(struct work_struct *work);
+
+#define DEFINE_STATIC_KEY_DEFERRED_TRUE(name, rl)			\
+	struct static_key_true_deferred name = {			\
+		.key =		{ STATIC_KEY_INIT_TRUE },		\
+		.timeout =	(rl),					\
+		.work =	__DELAYED_WORK_INITIALIZER((name).work,		\
+						   jump_label_update_timeout, \
+						   0),			\
+	}
+
+#define DEFINE_STATIC_KEY_DEFERRED_FALSE(name, rl)			\
+	struct static_key_false_deferred name = {			\
+		.key =		{ STATIC_KEY_INIT_FALSE },		\
+		.timeout =	(rl),					\
+		.work =	__DELAYED_WORK_INITIALIZER((name).work,		\
+						   jump_label_update_timeout, \
+						   0),			\
+	}
+
+#define static_branch_deferred_inc(x)	static_branch_inc(&(x)->key)
+
 #else	/* !CONFIG_JUMP_LABEL */
 struct static_key_deferred {
 	struct static_key  key;
 };
+struct static_key_true_deferred {
+	struct static_key_true key;
+};
+struct static_key_false_deferred {
+	struct static_key_false key;
+};
+#define DEFINE_STATIC_KEY_DEFERRED_TRUE(name, rl)	\
+	struct static_key_true_deferred name = { STATIC_KEY_TRUE_INIT }
+#define DEFINE_STATIC_KEY_DEFERRED_FALSE(name, rl)	\
+	struct static_key_false_deferred name = { STATIC_KEY_FALSE_INIT }
+
+#define static_branch_slow_dec_deferred(x)	static_branch_dec(&(x)->key)
+
 static inline void static_key_slow_dec_deferred(struct static_key_deferred *key)
 {
 	STATIC_KEY_CHECK_USE(key);
 	static_key_slow_dec(&key->key);
 }
-static inline void static_key_deferred_flush(struct static_key_deferred *key)
+static inline void static_key_deferred_flush(void *key)
 {
 	STATIC_KEY_CHECK_USE(key);
 }
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index a799b1ac6b2f..73bbbaddbd9c 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -244,12 +244,13 @@ static void __static_key_slow_dec(struct static_key *key,
 	cpus_read_unlock();
 }
 
-static void jump_label_update_timeout(struct work_struct *work)
+void jump_label_update_timeout(struct work_struct *work)
 {
 	struct static_key_deferred *key =
 		container_of(work, struct static_key_deferred, work.work);
 	__static_key_slow_dec(&key->key, 0, NULL);
 }
+EXPORT_SYMBOL_GPL(jump_label_update_timeout);
 
 void static_key_slow_dec(struct static_key *key)
 {
@@ -264,19 +265,21 @@ void static_key_slow_dec_cpuslocked(struct static_key *key)
 	__static_key_slow_dec_cpuslocked(key, 0, NULL);
 }
 
-void static_key_slow_dec_deferred(struct static_key_deferred *key)
+void __static_key_slow_dec_deferred(struct static_key *key,
+				    struct delayed_work *work,
+				    unsigned long timeout)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(&key->key, key->timeout, &key->work);
+	__static_key_slow_dec(key, timeout, work);
 }
-EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred);
+EXPORT_SYMBOL_GPL(__static_key_slow_dec_deferred);
 
-void static_key_deferred_flush(struct static_key_deferred *key)
+void __static_key_deferred_flush(void *key, struct delayed_work *work)
 {
 	STATIC_KEY_CHECK_USE(key);
-	flush_delayed_work(&key->work);
+	flush_delayed_work(work);
 }
-EXPORT_SYMBOL_GPL(static_key_deferred_flush);
+EXPORT_SYMBOL_GPL(__static_key_deferred_flush);
 
 void jump_label_rate_limit(struct static_key_deferred *key,
 		unsigned long rl)

[PATCH 2/3] locking/static_key: factor out the fast path of static_key_slow_dec()

[Jakub Kicinski]

static_key_slow_dec() checks if the atomic enable count is larger
than 1, and if so there decrements it before taking the jump_label_lock.
Move this logic into a helper for reuse in rate limitted keys.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 kernel/jump_label.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 73bbbaddbd9c..02c3d11264dd 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -202,13 +202,13 @@ void static_key_disable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-static void __static_key_slow_dec_cpuslocked(struct static_key *key,
-					   unsigned long rate_limit,
-					   struct delayed_work *work)
+static bool static_key_slow_try_dec(struct static_key *key)
 {
 	int val;
 
-	lockdep_assert_cpus_held();
+	val = atomic_fetch_add_unless(&key->enabled, -1, 1);
+	if (val == 1)
+		return false;
 
 	/*
 	 * The negative count check is valid even when a negative
@@ -217,11 +217,18 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key,
 	 * returns is unbalanced, because all other static_key_slow_inc()
 	 * instances block while the update is in progress.
 	 */
-	val = atomic_fetch_add_unless(&key->enabled, -1, 1);
-	if (val != 1) {
-		WARN(val < 0, "jump label: negative count!\n");
+	WARN(val < 0, "jump label: negative count!\n");
+	return true;
+}
+
+static void __static_key_slow_dec_cpuslocked(struct static_key *key,
+					   unsigned long rate_limit,
+					   struct delayed_work *work)
+{
+	lockdep_assert_cpus_held();
+
+	if (static_key_slow_try_dec(key))
 		return;
-	}
 
 	jump_label_lock();
 	if (atomic_dec_and_test(&key->enabled)) {
-- 
2.21.0

[tip:locking/core] locking/static_key: Factor out the fast path of static_key_slow_dec()

[tip-bot for Jakub Kicinski]

Commit-ID:  b92e793bbe4a1c49dbf78d8d526561e7a7dd568a
Gitweb:     https://git.kernel.org/tip/b92e793bbe4a1c49dbf78d8d526561e7a7dd568a
Author:     Jakub Kicinski <jakub.kicinski@netronome.com>
AuthorDate: Fri, 29 Mar 2019 17:08:53 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 29 Apr 2019 08:29:21 +0200

locking/static_key: Factor out the fast path of static_key_slow_dec()

static_key_slow_dec() checks if the atomic enable count is larger
than 1, and if so there decrements it before taking the jump_label_lock.
Move this logic into a helper for reuse in rate limitted keys.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: alexei.starovoitov@gmail.com
Cc: ard.biesheuvel@linaro.org
Cc: oss-drivers@netronome.com
Cc: yamada.masahiro@socionext.com
Link: https://lkml.kernel.org/r/20190330000854.30142-3-jakub.kicinski@netronome.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/jump_label.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 73bbbaddbd9c..02c3d11264dd 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -202,13 +202,13 @@ void static_key_disable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-static void __static_key_slow_dec_cpuslocked(struct static_key *key,
-					   unsigned long rate_limit,
-					   struct delayed_work *work)
+static bool static_key_slow_try_dec(struct static_key *key)
 {
 	int val;
 
-	lockdep_assert_cpus_held();
+	val = atomic_fetch_add_unless(&key->enabled, -1, 1);
+	if (val == 1)
+		return false;
 
 	/*
 	 * The negative count check is valid even when a negative
@@ -217,11 +217,18 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key,
 	 * returns is unbalanced, because all other static_key_slow_inc()
 	 * instances block while the update is in progress.
 	 */
-	val = atomic_fetch_add_unless(&key->enabled, -1, 1);
-	if (val != 1) {
-		WARN(val < 0, "jump label: negative count!\n");
+	WARN(val < 0, "jump label: negative count!\n");
+	return true;
+}
+
+static void __static_key_slow_dec_cpuslocked(struct static_key *key,
+					   unsigned long rate_limit,
+					   struct delayed_work *work)
+{
+	lockdep_assert_cpus_held();
+
+	if (static_key_slow_try_dec(key))
 		return;
-	}
 
 	jump_label_lock();
 	if (atomic_dec_and_test(&key->enabled)) {

[PATCH 3/3] locking/static_key: don't take sleeping locks in __static_key_slow_dec_deferred()

[Jakub Kicinski]

Changing jump_label state is protected by jump_label_lock().
Rate limited static_key_slow_dec(), however, will never
directly call jump_label_update(), it will schedule a delayed
work instead.  Therefore it's unnecessary to take both the
cpus_read_lock() and jump_label_lock().

This allows static_key_slow_dec_deferred() to be called
from atomic contexts, like socket destructing in net/tls,
without the need for another indirection.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 kernel/jump_label.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 02c3d11264dd..de6efdecc70d 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -221,9 +221,7 @@ static bool static_key_slow_try_dec(struct static_key *key)
 	return true;
 }
 
-static void __static_key_slow_dec_cpuslocked(struct static_key *key,
-					   unsigned long rate_limit,
-					   struct delayed_work *work)
+static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	lockdep_assert_cpus_held();
 
@@ -231,23 +229,15 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key,
 		return;
 
 	jump_label_lock();
-	if (atomic_dec_and_test(&key->enabled)) {
-		if (rate_limit) {
-			atomic_inc(&key->enabled);
-			schedule_delayed_work(work, rate_limit);
-		} else {
-			jump_label_update(key);
-		}
-	}
+	if (atomic_dec_and_test(&key->enabled))
+		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)
+static void __static_key_slow_dec(struct static_key *key)
 {
 	cpus_read_lock();
-	__static_key_slow_dec_cpuslocked(key, rate_limit, work);
+	__static_key_slow_dec_cpuslocked(key);
 	cpus_read_unlock();
 }
 
@@ -255,21 +245,21 @@ void jump_label_update_timeout(struct work_struct *work)
 {
 	struct static_key_deferred *key =
 		container_of(work, struct static_key_deferred, work.work);
-	__static_key_slow_dec(&key->key, 0, NULL);
+	__static_key_slow_dec(&key->key);
 }
 EXPORT_SYMBOL_GPL(jump_label_update_timeout);
 
 void static_key_slow_dec(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(key, 0, NULL);
+	__static_key_slow_dec(key);
 }
 EXPORT_SYMBOL_GPL(static_key_slow_dec);
 
 void static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec_cpuslocked(key, 0, NULL);
+	__static_key_slow_dec_cpuslocked(key);
 }
 
 void __static_key_slow_dec_deferred(struct static_key *key,
@@ -277,7 +267,11 @@ void __static_key_slow_dec_deferred(struct static_key *key,
 				    unsigned long timeout)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(key, timeout, work);
+
+	if (static_key_slow_try_dec(key))
+		return;
+
+	schedule_delayed_work(work, timeout);
 }
 EXPORT_SYMBOL_GPL(__static_key_slow_dec_deferred);
 
-- 
2.21.0

[tip:locking/core] locking/static_key: Don't take sleeping locks in __static_key_slow_dec_deferred()

[tip-bot for Jakub Kicinski]

Commit-ID:  94b5f312cfb4a66055d9b688dc9ab6b297eb9dcc
Gitweb:     https://git.kernel.org/tip/94b5f312cfb4a66055d9b688dc9ab6b297eb9dcc
Author:     Jakub Kicinski <jakub.kicinski@netronome.com>
AuthorDate: Fri, 29 Mar 2019 17:08:54 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 29 Apr 2019 08:29:21 +0200

locking/static_key: Don't take sleeping locks in __static_key_slow_dec_deferred()

Changing jump_label state is protected by jump_label_lock().
Rate limited static_key_slow_dec(), however, will never
directly call jump_label_update(), it will schedule a delayed
work instead.  Therefore it's unnecessary to take both the
cpus_read_lock() and jump_label_lock().

This allows static_key_slow_dec_deferred() to be called
from atomic contexts, like socket destructing in net/tls,
without the need for another indirection.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: alexei.starovoitov@gmail.com
Cc: ard.biesheuvel@linaro.org
Cc: oss-drivers@netronome.com
Cc: yamada.masahiro@socionext.com
Link: https://lkml.kernel.org/r/20190330000854.30142-4-jakub.kicinski@netronome.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/jump_label.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 02c3d11264dd..de6efdecc70d 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -221,9 +221,7 @@ static bool static_key_slow_try_dec(struct static_key *key)
 	return true;
 }
 
-static void __static_key_slow_dec_cpuslocked(struct static_key *key,
-					   unsigned long rate_limit,
-					   struct delayed_work *work)
+static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	lockdep_assert_cpus_held();
 
@@ -231,23 +229,15 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key,
 		return;
 
 	jump_label_lock();
-	if (atomic_dec_and_test(&key->enabled)) {
-		if (rate_limit) {
-			atomic_inc(&key->enabled);
-			schedule_delayed_work(work, rate_limit);
-		} else {
-			jump_label_update(key);
-		}
-	}
+	if (atomic_dec_and_test(&key->enabled))
+		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)
+static void __static_key_slow_dec(struct static_key *key)
 {
 	cpus_read_lock();
-	__static_key_slow_dec_cpuslocked(key, rate_limit, work);
+	__static_key_slow_dec_cpuslocked(key);
 	cpus_read_unlock();
 }
 
@@ -255,21 +245,21 @@ void jump_label_update_timeout(struct work_struct *work)
 {
 	struct static_key_deferred *key =
 		container_of(work, struct static_key_deferred, work.work);
-	__static_key_slow_dec(&key->key, 0, NULL);
+	__static_key_slow_dec(&key->key);
 }
 EXPORT_SYMBOL_GPL(jump_label_update_timeout);
 
 void static_key_slow_dec(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(key, 0, NULL);
+	__static_key_slow_dec(key);
 }
 EXPORT_SYMBOL_GPL(static_key_slow_dec);
 
 void static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec_cpuslocked(key, 0, NULL);
+	__static_key_slow_dec_cpuslocked(key);
 }
 
 void __static_key_slow_dec_deferred(struct static_key *key,
@@ -277,7 +267,11 @@ void __static_key_slow_dec_deferred(struct static_key *key,
 				    unsigned long timeout)
 {
 	STATIC_KEY_CHECK_USE(key);
-	__static_key_slow_dec(key, timeout, work);
+
+	if (static_key_slow_try_dec(key))
+		return;
+
+	schedule_delayed_work(work, timeout);
 }
 EXPORT_SYMBOL_GPL(__static_key_slow_dec_deferred);
 

Re: [PATCH 0/3] locking/static_key: improve rate limited labels

[Jakub Kicinski]

On Fri, 29 Mar 2019 17:08:51 -0700, Jakub Kicinski wrote:
> Hi!
> 
> This will be used to fix the static branch disabling in the TLS
> code.  The net/tls/ code should be using the deferred static
> branch type, because unprivileged users can flip the branch
> on and off quite easily with CONFIG_TLS_DEVICE=y.
> 
> Second of all we shouldn't take the jump label locks from
> the RX path, when the socket is destroyed.  This can be avoided
> with some slight code refactoring in deferred static_key as
> it already runs from a workqueue.
> 
> This the series (and a simple tls patch which makes use of it)
> applied opening 0.5M TLS connections to localhost (just calling
> setsockopt, no data exchange) goes down from 37.9s to 12.4s.

Once/if we get positive feedback from locking folks, would it be
possible to merge these via net-next tree alongside the patch
converting TLS to rate limited branches?

Re: [PATCH 0/3] locking/static_key: improve rate limited labels

[Peter Zijlstra]

On Mon, Apr 01, 2019 at 11:21:53AM -0700, Jakub Kicinski wrote:
> On Fri, 29 Mar 2019 17:08:51 -0700, Jakub Kicinski wrote:
> > Hi!
> > 
> > This will be used to fix the static branch disabling in the TLS
> > code.  The net/tls/ code should be using the deferred static
> > branch type, because unprivileged users can flip the branch
> > on and off quite easily with CONFIG_TLS_DEVICE=y.
> > 
> > Second of all we shouldn't take the jump label locks from
> > the RX path, when the socket is destroyed.  This can be avoided
> > with some slight code refactoring in deferred static_key as
> > it already runs from a workqueue.
> > 
> > This the series (and a simple tls patch which makes use of it)
> > applied opening 0.5M TLS connections to localhost (just calling
> > setsockopt, no data exchange) goes down from 37.9s to 12.4s.
> 
> Once/if we get positive feedback from locking folks, would it be
> possible to merge these via net-next tree alongside the patch
> converting TLS to rate limited branches?

Sorry, I've been ill, I'll try and have a look at these patches soon.

Re: [PATCH 0/3] locking/static_key: improve rate limited labels

[Peter Zijlstra]

On Mon, Apr 01, 2019 at 11:21:53AM -0700, Jakub Kicinski wrote:
> On Fri, 29 Mar 2019 17:08:51 -0700, Jakub Kicinski wrote:
> > Hi!
> > 
> > This will be used to fix the static branch disabling in the TLS
> > code.  The net/tls/ code should be using the deferred static
> > branch type, because unprivileged users can flip the branch
> > on and off quite easily with CONFIG_TLS_DEVICE=y.
> > 
> > Second of all we shouldn't take the jump label locks from
> > the RX path, when the socket is destroyed.  This can be avoided
> > with some slight code refactoring in deferred static_key as
> > it already runs from a workqueue.
> > 
> > This the series (and a simple tls patch which makes use of it)
> > applied opening 0.5M TLS connections to localhost (just calling
> > setsockopt, no data exchange) goes down from 37.9s to 12.4s.
> 
> Once/if we get positive feedback from locking folks, would it be
> possible to merge these via net-next tree alongside the patch
> converting TLS to rate limited branches?

Looks good. If routed through the network tree because usage there:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Otherwise let me know and I'll carry them.

Re: [PATCH 0/3] locking/static_key: improve rate limited labels

[Jakub Kicinski]

On Thu, 4 Apr 2019 12:29:02 +0200, Peter Zijlstra wrote:
> On Mon, Apr 01, 2019 at 11:21:53AM -0700, Jakub Kicinski wrote:
> > On Fri, 29 Mar 2019 17:08:51 -0700, Jakub Kicinski wrote:  
> > > Hi!
> > > 
> > > This will be used to fix the static branch disabling in the TLS
> > > code.  The net/tls/ code should be using the deferred static
> > > branch type, because unprivileged users can flip the branch
> > > on and off quite easily with CONFIG_TLS_DEVICE=y.
> > > 
> > > Second of all we shouldn't take the jump label locks from
> > > the RX path, when the socket is destroyed.  This can be avoided
> > > with some slight code refactoring in deferred static_key as
> > > it already runs from a workqueue.
> > > 
> > > This the series (and a simple tls patch which makes use of it)
> > > applied opening 0.5M TLS connections to localhost (just calling
> > > setsockopt, no data exchange) goes down from 37.9s to 12.4s.  
> > 
> > Once/if we get positive feedback from locking folks, would it be
> > possible to merge these via net-next tree alongside the patch
> > converting TLS to rate limited branches?  
> 
> Looks good. If routed through the network tree because usage there:
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Otherwise let me know and I'll carry them.

Hi Peter!  I was initially hoping that a1247d06d010
("locking/static_key: Fix false positive warnings on concurrent dec/inc")
may go into 5.1, but it's not really a regression.  It will conflict, so
the net-next route won't work.  Would you be able to carry this set
after all?

Re: [PATCH 0/3] locking/static_key: improve rate limited labels

[Peter Zijlstra]

On Tue, Apr 16, 2019 at 09:33:15AM -0700, Jakub Kicinski wrote:
> On Thu, 4 Apr 2019 12:29:02 +0200, Peter Zijlstra wrote:
> > On Mon, Apr 01, 2019 at 11:21:53AM -0700, Jakub Kicinski wrote:
> > > On Fri, 29 Mar 2019 17:08:51 -0700, Jakub Kicinski wrote:  
> > > > Hi!
> > > > 
> > > > This will be used to fix the static branch disabling in the TLS
> > > > code.  The net/tls/ code should be using the deferred static
> > > > branch type, because unprivileged users can flip the branch
> > > > on and off quite easily with CONFIG_TLS_DEVICE=y.
> > > > 
> > > > Second of all we shouldn't take the jump label locks from
> > > > the RX path, when the socket is destroyed.  This can be avoided
> > > > with some slight code refactoring in deferred static_key as
> > > > it already runs from a workqueue.
> > > > 
> > > > This the series (and a simple tls patch which makes use of it)
> > > > applied opening 0.5M TLS connections to localhost (just calling
> > > > setsockopt, no data exchange) goes down from 37.9s to 12.4s.  
> > > 
> > > Once/if we get positive feedback from locking folks, would it be
> > > possible to merge these via net-next tree alongside the patch
> > > converting TLS to rate limited branches?  
> > 
> > Looks good. If routed through the network tree because usage there:
> > 
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > Otherwise let me know and I'll carry them.
> 
> Hi Peter!  I was initially hoping that a1247d06d010
> ("locking/static_key: Fix false positive warnings on concurrent dec/inc")
> may go into 5.1, but it's not really a regression.  It will conflict, so
> the net-next route won't work.  Would you be able to carry this set
> after all?

n/p, done!