[PATCH -v2 0/8] jump_label: Another (better) static_key interface

[PATCH -v2 0/8] jump_label: Another (better) static_key interface

[Peter Zijlstra]

Hi all,

After yet another bug because of the weirdness of the static key interface,
here an attempt at providing a better one.

This series is tested on x86_64 (by me) and s390x (heiko).

---
 arch/Kconfig                          |    6 +
 arch/arm/include/asm/jump_label.h     |   24 ++--
 arch/arm/kernel/jump_label.c          |    2 
 arch/arm64/include/asm/jump_label.h   |   18 ++-
 arch/arm64/kernel/jump_label.c        |    2 
 arch/mips/include/asm/jump_label.h    |   19 +++
 arch/mips/kernel/jump_label.c         |    2 
 arch/powerpc/include/asm/jump_label.h |   19 +++
 arch/powerpc/kernel/jump_label.c      |    2 
 arch/s390/include/asm/jump_label.h    |   19 +++
 arch/s390/kernel/jump_label.c         |    2 
 arch/sparc/include/asm/jump_label.h   |   35 ++++--
 arch/sparc/kernel/jump_label.c        |    2 
 arch/x86/include/asm/jump_label.h     |   21 +++
 arch/x86/kernel/jump_label.c          |    2 
 arch/x86/kernel/tsc.c                 |   22 +--
 include/linux/jump_label.h            |  188 +++++++++++++++++++++++++++++-----
 kernel/jump_label.c                   |  158 +++++++++++++++++++---------
 kernel/sched/core.c                   |    6 -
 19 files changed, 424 insertions(+), 125 deletions(-)

[PATCH -v2 1/8] jump_label: Rename JUMP_LABEL_{EN,DIS}ABLE

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-rename-jump_label_type.patch --]
[-- Type: text/plain, Size: 5269 bytes --]

Since we've already stepped away from ENABLE is a JMP and DISABLE is a
NOP with the branch_default bits, and are going to make it even worse,
rename to make it clearer.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm/kernel/jump_label.c     |    2 +-
 arch/arm64/kernel/jump_label.c   |    2 +-
 arch/mips/kernel/jump_label.c    |    2 +-
 arch/powerpc/kernel/jump_label.c |    2 +-
 arch/s390/kernel/jump_label.c    |    2 +-
 arch/sparc/kernel/jump_label.c   |    2 +-
 arch/x86/kernel/jump_label.c     |    2 +-
 include/linux/jump_label.h       |    4 ++--
 kernel/jump_label.c              |   18 +++++++++---------
 9 files changed, 18 insertions(+), 18 deletions(-)

--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -12,7 +12,7 @@ static void __arch_jump_label_transform(
 	void *addr = (void *)entry->code;
 	unsigned int insn;
 
-	if (type == JUMP_LABEL_ENABLE)
+	if (type == JUMP_LABEL_JMP)
 		insn = arm_gen_branch(entry->code, entry->target);
 	else
 		insn = arm_gen_nop();
--- a/arch/arm64/kernel/jump_label.c
+++ b/arch/arm64/kernel/jump_label.c
@@ -28,7 +28,7 @@ void arch_jump_label_transform(struct ju
 	void *addr = (void *)entry->code;
 	u32 insn;
 
-	if (type == JUMP_LABEL_ENABLE) {
+	if (type == JUMP_LABEL_JMP) {
 		insn = aarch64_insn_gen_branch_imm(entry->code,
 						   entry->target,
 						   AARCH64_INSN_BRANCH_NOLINK);
--- a/arch/mips/kernel/jump_label.c
+++ b/arch/mips/kernel/jump_label.c
@@ -51,7 +51,7 @@ void arch_jump_label_transform(struct ju
 	/* Target must have the right alignment and ISA must be preserved. */
 	BUG_ON((e->target & J_ALIGN_MASK) != J_ISA_BIT);
 
-	if (type == JUMP_LABEL_ENABLE) {
+	if (type == JUMP_LABEL_JMP) {
 		insn.j_format.opcode = J_ISA_BIT ? mm_j32_op : j_op;
 		insn.j_format.target = e->target >> J_RANGE_SHIFT;
 	} else {
--- a/arch/powerpc/kernel/jump_label.c
+++ b/arch/powerpc/kernel/jump_label.c
@@ -17,7 +17,7 @@ void arch_jump_label_transform(struct ju
 {
 	u32 *addr = (u32 *)(unsigned long)entry->code;
 
-	if (type == JUMP_LABEL_ENABLE)
+	if (type == JUMP_LABEL_JMP)
 		patch_branch(addr, entry->target, 0);
 	else
 		patch_instruction(addr, PPC_INST_NOP);
--- a/arch/s390/kernel/jump_label.c
+++ b/arch/s390/kernel/jump_label.c
@@ -64,7 +64,7 @@ static void __jump_label_transform(struc
 {
 	struct insn old, new;
 
-	if (type == JUMP_LABEL_ENABLE) {
+	if (type == JUMP_LABEL_JMP) {
 		jump_label_make_nop(entry, &old);
 		jump_label_make_branch(entry, &new);
 	} else {
--- a/arch/sparc/kernel/jump_label.c
+++ b/arch/sparc/kernel/jump_label.c
@@ -16,7 +16,7 @@ void arch_jump_label_transform(struct ju
 	u32 val;
 	u32 *insn = (u32 *) (unsigned long) entry->code;
 
-	if (type == JUMP_LABEL_ENABLE) {
+	if (type == JUMP_LABEL_JMP) {
 		s32 off = (s32)entry->target - (s32)entry->code;
 
 #ifdef CONFIG_SPARC64
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -45,7 +45,7 @@ static void __jump_label_transform(struc
 	const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
 	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
 
-	if (type == JUMP_LABEL_ENABLE) {
+	if (type == JUMP_LABEL_JMP) {
 		if (init) {
 			/*
 			 * Jump label is enabled for the first time.
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -86,8 +86,8 @@ struct static_key {
 #ifndef __ASSEMBLY__
 
 enum jump_label_type {
-	JUMP_LABEL_DISABLE = 0,
-	JUMP_LABEL_ENABLE,
+	JUMP_LABEL_NOP = 0,
+	JUMP_LABEL_JMP,
 };
 
 struct module;
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -65,9 +65,9 @@ void static_key_slow_inc(struct static_k
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
 		if (!jump_label_get_branch_default(key))
-			jump_label_update(key, JUMP_LABEL_ENABLE);
+			jump_label_update(key, JUMP_LABEL_JMP);
 		else
-			jump_label_update(key, JUMP_LABEL_DISABLE);
+			jump_label_update(key, JUMP_LABEL_NOP);
 	}
 	atomic_inc(&key->enabled);
 	jump_label_unlock();
@@ -88,9 +88,9 @@ static void __static_key_slow_dec(struct
 		schedule_delayed_work(work, rate_limit);
 	} else {
 		if (!jump_label_get_branch_default(key))
-			jump_label_update(key, JUMP_LABEL_DISABLE);
+			jump_label_update(key, JUMP_LABEL_NOP);
 		else
-			jump_label_update(key, JUMP_LABEL_ENABLE);
+			jump_label_update(key, JUMP_LABEL_JMP);
 	}
 	jump_label_unlock();
 }
@@ -184,9 +184,9 @@ static enum jump_label_type jump_label_t
 	bool state = static_key_enabled(key);
 
 	if ((!true_branch && state) || (true_branch && !state))
-		return JUMP_LABEL_ENABLE;
+		return JUMP_LABEL_JMP;
 
-	return JUMP_LABEL_DISABLE;
+	return JUMP_LABEL_NOP;
 }
 
 void __init jump_label_init(void)
@@ -276,7 +276,7 @@ void jump_label_apply_nops(struct module
 		return;
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
-		arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
+		arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
 	}
 }
 
@@ -318,8 +318,8 @@ static int jump_label_add_module(struct
 		jlm->next = key->next;
 		key->next = jlm;
 
-		if (jump_label_type(key) == JUMP_LABEL_ENABLE)
-			__jump_label_update(key, iter, iter_stop, JUMP_LABEL_ENABLE);
+		if (jump_label_type(key) == JUMP_LABEL_JMP)
+			__jump_label_update(key, iter, iter_stop, JUMP_LABEL_JMP);
 	}
 
 	return 0;

[PATCH -v2 2/8] jump_label: Rename JUMP_LABEL_TYPE_*

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-rename-jump_type.patch --]
[-- Type: text/plain, Size: 3784 bytes --]

Rename the JUMP_LABEL_TYPE_* macros to be JUMP_TYPE_* and move the
inline helpers into kernel/jump_label.c, since that's the only place
they're ever used.

Also rename the helpers.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/jump_label.h |   25 +++++--------------------
 kernel/jump_label.c        |   25 ++++++++++++++++---------
 2 files changed, 21 insertions(+), 29 deletions(-)

--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -101,24 +101,9 @@ static inline int static_key_count(struc
 
 #ifdef HAVE_JUMP_LABEL
 
-#define JUMP_LABEL_TYPE_FALSE_BRANCH	0UL
-#define JUMP_LABEL_TYPE_TRUE_BRANCH	1UL
-#define JUMP_LABEL_TYPE_MASK		1UL
-
-static
-inline struct jump_entry *jump_label_get_entries(struct static_key *key)
-{
-	return (struct jump_entry *)((unsigned long)key->entries
-						& ~JUMP_LABEL_TYPE_MASK);
-}
-
-static inline bool jump_label_get_branch_default(struct static_key *key)
-{
-	if (((unsigned long)key->entries & JUMP_LABEL_TYPE_MASK) ==
-	    JUMP_LABEL_TYPE_TRUE_BRANCH)
-		return true;
-	return false;
-}
+#define JUMP_TYPE_FALSE	0UL
+#define JUMP_TYPE_TRUE	1UL
+#define JUMP_TYPE_MASK	1UL
 
 static __always_inline bool static_key_false(struct static_key *key)
 {
@@ -147,10 +132,10 @@ extern void jump_label_apply_nops(struct
 
 #define STATIC_KEY_INIT_TRUE ((struct static_key)		\
 	{ .enabled = ATOMIC_INIT(1),				\
-	  .entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
+	  .entries = (void *)JUMP_TYPE_TRUE })
 #define STATIC_KEY_INIT_FALSE ((struct static_key)		\
 	{ .enabled = ATOMIC_INIT(0),				\
-	  .entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
+	  .entries = (void *)JUMP_TYPE_FALSE })
 
 #else  /* !HAVE_JUMP_LABEL */
 
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -56,6 +56,11 @@ jump_label_sort_entries(struct jump_entr
 
 static void jump_label_update(struct static_key *key, int enable);
 
+static inline bool static_key_type(struct static_key *key)
+{
+	return (unsigned long)key->entries & JUMP_TYPE_MASK;
+}
+
 void static_key_slow_inc(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
@@ -64,7 +69,7 @@ void static_key_slow_inc(struct static_k
 
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
-		if (!jump_label_get_branch_default(key))
+		if (!static_key_type(key))
 			jump_label_update(key, JUMP_LABEL_JMP);
 		else
 			jump_label_update(key, JUMP_LABEL_NOP);
@@ -87,7 +92,7 @@ static void __static_key_slow_dec(struct
 		atomic_inc(&key->enabled);
 		schedule_delayed_work(work, rate_limit);
 	} else {
-		if (!jump_label_get_branch_default(key))
+		if (!static_key_type(key))
 			jump_label_update(key, JUMP_LABEL_NOP);
 		else
 			jump_label_update(key, JUMP_LABEL_JMP);
@@ -178,15 +183,17 @@ static void __jump_label_update(struct s
 	}
 }
 
-static enum jump_label_type jump_label_type(struct static_key *key)
+static inline struct jump_entry *static_key_entries(struct static_key *key)
 {
-	bool true_branch = jump_label_get_branch_default(key);
-	bool state = static_key_enabled(key);
+	return (struct jump_entry *)((unsigned long)key->entries & ~JUMP_TYPE_MASK);
+}
 
-	if ((!true_branch && state) || (true_branch && !state))
-		return JUMP_LABEL_JMP;
+static enum jump_label_type jump_label_type(struct static_key *key)
+{
+	bool enabled = static_key_enabled(key);
+	bool type = static_key_type(key);
 
-	return JUMP_LABEL_NOP;
+	return enabled ^ type;
 }
 
 void __init jump_label_init(void)
@@ -442,7 +449,7 @@ int jump_label_text_reserved(void *start
 static void jump_label_update(struct static_key *key, int enable)
 {
 	struct jump_entry *stop = __stop___jump_table;
-	struct jump_entry *entry = jump_label_get_entries(key);
+	struct jump_entry *entry = static_key_entries(key);
 #ifdef CONFIG_MODULES
 	struct module *mod;
 

[PATCH -v2 3/8] jump_label: Add jump_entry_key() helper

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-jump_entry_key.patch --]
[-- Type: text/plain, Size: 1795 bytes --]

Avoid some casting with a helper, also prepares the way for
overloading the LSB of jump_entry::key.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/jump_label.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -188,6 +188,11 @@ static inline struct jump_entry *static_
 	return (struct jump_entry *)((unsigned long)key->entries & ~JUMP_TYPE_MASK);
 }
 
+static inline struct static_key *jump_entry_key(struct jump_entry *entry)
+{
+	return (struct static_key *)((unsigned long)entry->key);
+}
+
 static enum jump_label_type jump_label_type(struct static_key *key)
 {
 	bool enabled = static_key_enabled(key);
@@ -209,8 +214,8 @@ void __init jump_label_init(void)
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
-		iterk = (struct static_key *)(unsigned long)iter->key;
-		arch_jump_label_transform_static(iter, jump_label_type(iterk));
+		iterk = jump_entry_key(iter);
+		arch_jump_label_transform_static(iter, jump_label_type(iterk));
 		if (iterk == key)
 			continue;
 
@@ -304,7 +309,7 @@ static int jump_label_add_module(struct
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
-		iterk = (struct static_key *)(unsigned long)iter->key;
+		iterk = jump_entry_key(iter);
 		if (iterk == key)
 			continue;
 
@@ -341,10 +346,10 @@ static void jump_label_del_module(struct
 	struct static_key_mod *jlm, **prev;
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
-		if (iter->key == (jump_label_t)(unsigned long)key)
+		if (jump_entry_key(iter) == key)
 			continue;
 
-		key = (struct static_key *)(unsigned long)iter->key;
+		key = jump_entry_key(iter);
 
 		if (within_module(iter->key, mod))
 			continue;

[PATCH -v2 4/8] jump_label: Add static_key_{en,dis}able() helpers

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-bool.patch --]
[-- Type: text/plain, Size: 1485 bytes --]

Add two helpers to make it easier to treat the refcount as boolean.

Suggested-by: Jason Baron <jasonbaron0@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/jump_label.h |   20 ++++++++++++++++++++
 kernel/sched/core.c        |    6 ++----
 2 files changed, 22 insertions(+), 4 deletions(-)

--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -198,6 +198,26 @@ static inline bool static_key_enabled(st
 	return static_key_count(key) > 0;
 }
 
+static inline void static_key_enable(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (!count)
+		static_key_slow_inc(key);
+}
+
+static inline void static_key_disable(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (count)
+		static_key_slow_dec(key);
+}
+
 #endif	/* _LINUX_JUMP_LABEL_H */
 
 #endif /* __ASSEMBLY__ */
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -164,14 +164,12 @@ struct static_key sched_feat_keys[__SCHE
 
 static void sched_feat_disable(int i)
 {
-	if (static_key_enabled(&sched_feat_keys[i]))
-		static_key_slow_dec(&sched_feat_keys[i]);
+	static_key_disable(&sched_feat_keys[i]);
 }
 
 static void sched_feat_enable(int i)
 {
-	if (!static_key_enabled(&sched_feat_keys[i]))
-		static_key_slow_inc(&sched_feat_keys[i]);
+	static_key_enable(&sched_feat_keys[i]);
 }
 #else
 static void sched_feat_disable(int i) { };

[PATCH -v2 5/8] jump_label: Rework update logic

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-rework-update.patch --]
[-- Type: text/plain, Size: 6724 bytes --]

Instead of spreading the branch_default logic all over the place,
concentrate it into the one jump_label_type() function.

This does mean we need to actually increment/decrement they enabled
count _before_ calling the update path, otherwise jump_label_type()
will not see the right state.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/jump_label.c |   88 ++++++++++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 50 deletions(-)

--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -54,12 +54,7 @@ jump_label_sort_entries(struct jump_entr
 	sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
 }
 
-static void jump_label_update(struct static_key *key, int enable);
-
-static inline bool static_key_type(struct static_key *key)
-{
-	return (unsigned long)key->entries & JUMP_TYPE_MASK;
-}
+static void jump_label_update(struct static_key *key);
 
 void static_key_slow_inc(struct static_key *key)
 {
@@ -68,13 +63,8 @@ void static_key_slow_inc(struct static_k
 		return;
 
 	jump_label_lock();
-	if (atomic_read(&key->enabled) == 0) {
-		if (!static_key_type(key))
-			jump_label_update(key, JUMP_LABEL_JMP);
-		else
-			jump_label_update(key, JUMP_LABEL_NOP);
-	}
-	atomic_inc(&key->enabled);
+	if (atomic_inc_return(&key->enabled) == 1)
+		jump_label_update(key);
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -92,10 +82,7 @@ static void __static_key_slow_dec(struct
 		atomic_inc(&key->enabled);
 		schedule_delayed_work(work, rate_limit);
 	} else {
-		if (!static_key_type(key))
-			jump_label_update(key, JUMP_LABEL_NOP);
-		else
-			jump_label_update(key, JUMP_LABEL_JMP);
+		jump_label_update(key);
 	}
 	jump_label_unlock();
 }
@@ -154,7 +141,7 @@ static int __jump_label_text_reserved(st
 	return 0;
 }
 
-/* 
+/*
  * Update code which is definitely not currently executing.
  * Architectures which need heavyweight synchronization to modify
  * running code can override this to make the non-live update case
@@ -163,29 +150,17 @@ static int __jump_label_text_reserved(st
 void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry,
 					    enum jump_label_type type)
 {
-	arch_jump_label_transform(entry, type);	
+	arch_jump_label_transform(entry, type);
 }
 
-static void __jump_label_update(struct static_key *key,
-				struct jump_entry *entry,
-				struct jump_entry *stop, int enable)
+static inline struct jump_entry *static_key_entries(struct static_key *key)
 {
-	for (; (entry < stop) &&
-	      (entry->key == (jump_label_t)(unsigned long)key);
-	      entry++) {
-		/*
-		 * entry->code set to 0 invalidates module init text sections
-		 * kernel_text_address() verifies we are not in core kernel
-		 * init code, see jump_label_invalidate_module_init().
-		 */
-		if (entry->code && kernel_text_address(entry->code))
-			arch_jump_label_transform(entry, enable);
-	}
+	return (struct jump_entry *)((unsigned long)key->entries & ~JUMP_TYPE_MASK);
 }
 
-static inline struct jump_entry *static_key_entries(struct static_key *key)
+static inline bool static_key_type(struct static_key *key)
 {
-	return (struct jump_entry *)((unsigned long)key->entries & ~JUMP_TYPE_MASK);
+	return (unsigned long)key->entries & JUMP_TYPE_MASK;
 }
 
 static inline struct static_key *jump_entry_key(struct jump_entry *entry)
@@ -193,14 +168,30 @@ static inline struct static_key *jump_en
 	return (struct static_key *)((unsigned long)entry->key);
 }
 
-static enum jump_label_type jump_label_type(struct static_key *key)
+static enum jump_label_type jump_label_type(struct jump_entry *entry)
 {
+	struct static_key *key = jump_entry_key(entry);
 	bool enabled = static_key_enabled(key);
 	bool type = static_key_type(key);
 
 	return enabled ^ type;
 }
 
+static void __jump_label_update(struct static_key *key,
+				struct jump_entry *entry,
+				struct jump_entry *stop)
+{
+	for (; (entry < stop) && (jump_entry_key(entry) == key); entry++) {
+		/*
+		 * entry->code set to 0 invalidates module init text sections
+		 * kernel_text_address() verifies we are not in core kernel
+		 * init code, see jump_label_invalidate_module_init().
+		 */
+		if (entry->code && kernel_text_address(entry->code))
+			arch_jump_label_transform(entry, jump_label_type(entry));
+	}
+}
+
 void __init jump_label_init(void)
 {
 	struct jump_entry *iter_start = __start___jump_table;
@@ -214,8 +205,8 @@ void __init jump_label_init(void)
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
+		arch_jump_label_transform_static(iter, jump_label_type(iter));
 		iterk = jump_entry_key(iter);
-		arch_jump_label_transform_static(iter, jump_label_type(iterk));
 		if (iterk == key)
 			continue;
 
@@ -255,17 +246,15 @@ static int __jump_label_mod_text_reserve
 				start, end);
 }
 
-static void __jump_label_mod_update(struct static_key *key, int enable)
+static void __jump_label_mod_update(struct static_key *key)
 {
-	struct static_key_mod *mod = key->next;
+	struct static_key_mod *mod;
 
-	while (mod) {
+	for (mod = key->next; mod; mod = mod->next) {
 		struct module *m = mod->mod;
 
 		__jump_label_update(key, mod->entries,
-				    m->jump_entries + m->num_jump_entries,
-				    enable);
-		mod = mod->next;
+				    m->jump_entries + m->num_jump_entries);
 	}
 }
 
@@ -287,9 +276,8 @@ void jump_label_apply_nops(struct module
 	if (iter_start == iter_stop)
 		return;
 
-	for (iter = iter_start; iter < iter_stop; iter++) {
+	for (iter = iter_start; iter < iter_stop; iter++)
 		arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
-	}
 }
 
 static int jump_label_add_module(struct module *mod)
@@ -330,8 +318,8 @@ static int jump_label_add_module(struct
 		jlm->next = key->next;
 		key->next = jlm;
 
-		if (jump_label_type(key) == JUMP_LABEL_JMP)
-			__jump_label_update(key, iter, iter_stop, JUMP_LABEL_JMP);
+		if (jump_label_type(iter) == JUMP_LABEL_JMP)
+			__jump_label_update(key, iter, iter_stop);
 	}
 
 	return 0;
@@ -451,14 +439,14 @@ int jump_label_text_reserved(void *start
 	return ret;
 }
 
-static void jump_label_update(struct static_key *key, int enable)
+static void jump_label_update(struct static_key *key)
 {
 	struct jump_entry *stop = __stop___jump_table;
 	struct jump_entry *entry = static_key_entries(key);
 #ifdef CONFIG_MODULES
 	struct module *mod;
 
-	__jump_label_mod_update(key, enable);
+	__jump_label_mod_update(key);
 
 	preempt_disable();
 	mod = __module_address((unsigned long)key);
@@ -468,7 +456,7 @@ static void jump_label_update(struct sta
 #endif
 	/* if there are no users, entry can be NULL */
 	if (entry)
-		__jump_label_update(key, entry, stop, enable);
+		__jump_label_update(key, entry, stop);
 }
 
 #endif

[PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-new-interface.patch --]
[-- Type: text/plain, Size: 16410 bytes --]

There are various problems and short-comings with the current
static_key interface:

 - static_key_{true,false}() read like a branch depending on the key
   value, instead of the actual likely/unlikely branch depending on
   init value.

 - static_key_{true,false}() are, as stated above, tied to the
   static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.

 - we're limited to the 2 (out of 4) possible options that compile to
   a default NOP because that's what our arch_static_branch() assembly
   emits.

So provide a new static_key interface:

  DEFINE_STATIC_KEY_TRUE(name);
  DEFINE_STATIC_KEY_FALSE(name);

Which define a key of different types with an initial true/false
value.

Then allow:

   static_branch_likely()
   static_branch_unlikely()

to take a key of either type and emit the right instruction for the
case.

This means adding a second arch_static_branch_jump() assembly helper
which emits a JMP per default.

In order to determine the right instruction for the right state,
encode the branch type in the LSB of jump_entry::key.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm/include/asm/jump_label.h     |   24 +++--
 arch/arm64/include/asm/jump_label.h   |   18 +++-
 arch/mips/include/asm/jump_label.h    |   19 ++++
 arch/powerpc/include/asm/jump_label.h |   19 ++++
 arch/s390/include/asm/jump_label.h    |   19 ++++
 arch/sparc/include/asm/jump_label.h   |   35 ++++++--
 arch/x86/include/asm/jump_label.h     |   21 ++++
 include/linux/jump_label.h            |  143 ++++++++++++++++++++++++++++++++--
 kernel/jump_label.c                   |   35 ++++++--
 9 files changed, 294 insertions(+), 39 deletions(-)

--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -7,20 +7,28 @@
 
 #define JUMP_LABEL_NOP_SIZE 4
 
-#ifdef CONFIG_THUMB2_KERNEL
-#define JUMP_LABEL_NOP	"nop.w"
-#else
-#define JUMP_LABEL_NOP	"nop"
-#endif
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:\n\t"
+		 WASM(nop) "\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection\n\t"
+		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("1:\n\t"
-		 JUMP_LABEL_NOP "\n\t"
+		 WASM(b) " %l[l_yes]\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
-		 : :  "i" (key) :  : l_yes);
+		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
 
 	return false;
 l_yes:
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -26,14 +26,28 @@
 
 #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm goto("1: nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".align 3\n\t"
 		 ".quad 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
-		 :  :  "i"(key) :  : l_yes);
+		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm goto("1: b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 3\n\t"
+		 ".quad 1b, %l[l_yes], %c0\n\t"
+		 ".popsection\n\t"
+		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
 
 	return false;
 l_yes:
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -26,14 +26,29 @@
 #define NOP_INSN "nop"
 #endif
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
 		"nop\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
-		: :  "i" (key) : : l_yes);
+		: :  "i" (&((char *)key)[branch]) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:\tj %l[l_yes]\n\t"
+		"nop\n\t"
+		".pushsection __jump_table,  \"aw\"\n\t"
+		WORD_INSN " 1b, %l[l_yes], %0\n\t"
+		".popsection\n\t"
+		: :  "i" (&((char *)key)[branch]) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -18,14 +18,29 @@
 #define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
 #define JUMP_LABEL_NOP_SIZE	4
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("1:\n\t"
 		 "nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
 		 ".popsection \n\t"
-		 : :  "i" (key) : : l_yes);
+		 : :  "i" (&((char *)key)[branch]) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:\n\t"
+		 "b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (&((char *)key)[branch]) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
--- a/arch/s390/include/asm/jump_label.h
+++ b/arch/s390/include/asm/jump_label.h
@@ -12,14 +12,29 @@
  * We use a brcl 0,2 instruction for jump labels at compile time so it
  * can be easily distinguished from a hotpatch generated instruction.
  */
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("0:	brcl 0,"__stringify(JUMP_LABEL_NOP_OFFSET)"\n"
 		".pushsection __jump_table, \"aw\"\n"
 		".balign 8\n"
 		".quad 0b, %l[label], %0\n"
 		".popsection\n"
-		: : "X" (key) : : label);
+		: : "X" (&((char *)key)[branch]) : : label);
+
+	return false;
+label:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("0:	brcl 15, %l[label]\n"
+		".pushsection __jump_table, \"aw\"\n"
+		".balign 8\n"
+		".quad 0b, %l[label], %0\n"
+		".popsection\n"
+		: : "X" (&((char *)key)[branch]) : : label);
+
 	return false;
 label:
 	return true;
--- a/arch/sparc/include/asm/jump_label.h
+++ b/arch/sparc/include/asm/jump_label.h
@@ -7,16 +7,33 @@
 
 #define JUMP_LABEL_NOP_SIZE 4
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-		asm_volatile_goto("1:\n\t"
-			 "nop\n\t"
-			 "nop\n\t"
-			 ".pushsection __jump_table,  \"aw\"\n\t"
-			 ".align 4\n\t"
-			 ".word 1b, %l[l_yes], %c0\n\t"
-			 ".popsection \n\t"
-			 : :  "i" (key) : : l_yes);
+	asm_volatile_goto("1:\n\t"
+		 "nop\n\t"
+		 "nop\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 4\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (&((char *)key)[branch]) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:\n\t"
+		 "b %l[l_yes]\n\t"
+		 "nop\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 4\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (&((char *)key)[branch]) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -16,7 +16,7 @@
 # define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
 #endif
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("1:"
 		".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
@@ -24,7 +24,24 @@ static __always_inline bool arch_static_
 		_ASM_ALIGN "\n\t"
 		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
 		".popsection \n\t"
-		: :  "i" (key) : : l_yes);
+		: :  "i" (&((char *)key)[branch]) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:"
+		".byte 0xe9\n\t .long %l[l_yes] - 2f\n\t"
+		"2:\n\t"
+		".pushsection __jump_table,  \"aw\" \n\t"
+		_ASM_ALIGN "\n\t"
+		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
+		".popsection \n\t"
+		: :  "i" (&((char *)key)[branch]) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -107,12 +107,12 @@ static inline int static_key_count(struc
 
 static __always_inline bool static_key_false(struct static_key *key)
 {
-	return arch_static_branch(key);
+	return arch_static_branch(key, false);
 }
 
 static __always_inline bool static_key_true(struct static_key *key)
 {
-	return !static_key_false(key);
+	return !arch_static_branch(key, true);
 }
 
 extern struct jump_entry __start___jump_table[];
@@ -130,12 +130,12 @@ extern void static_key_slow_inc(struct s
 extern void static_key_slow_dec(struct static_key *key);
 extern void jump_label_apply_nops(struct module *mod);
 
-#define STATIC_KEY_INIT_TRUE ((struct static_key)		\
+#define STATIC_KEY_INIT_TRUE					\
 	{ .enabled = ATOMIC_INIT(1),				\
-	  .entries = (void *)JUMP_TYPE_TRUE })
-#define STATIC_KEY_INIT_FALSE ((struct static_key)		\
+	  .entries = (void *)JUMP_TYPE_TRUE }
+#define STATIC_KEY_INIT_FALSE					\
 	{ .enabled = ATOMIC_INIT(0),				\
-	  .entries = (void *)JUMP_TYPE_FALSE })
+	  .entries = (void *)JUMP_TYPE_FALSE }
 
 #else  /* !HAVE_JUMP_LABEL */
 
@@ -218,6 +218,137 @@ static inline void static_key_disable(st
 		static_key_slow_dec(key);
 }
 
+/* -------------------------------------------------------------------------- */
+
+/*
+ * Two type wrappers around static_key, such that we can use compile time
+ * type differentiation to emit the right code.
+ *
+ * All the below code is macros in order to play type games.
+ */
+
+struct static_key_true {
+	struct static_key key;
+};
+
+struct static_key_false {
+	struct static_key key;
+};
+
+#define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
+#define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }
+
+#define DEFINE_STATIC_KEY_TRUE(name)	\
+	struct static_key_true name = STATIC_KEY_TRUE_INIT
+
+#define DEFINE_STATIC_KEY_FALSE(name)	\
+	struct static_key_false name = STATIC_KEY_FALSE_INIT
+
+#ifdef HAVE_JUMP_LABEL
+
+/*
+ * Combine the right initial value (type) with the right branch order
+ * to generate the desired result.
+ *
+ *
+ * type\branch|	likely (1)	      |	unlikely (0)
+ * -----------+-----------------------+------------------
+ *            |                       |
+ *  true (1)  |	   ...		      |	   ...
+ *            |    NOP		      |	   JMP L
+ *            |    <br-stmts>	      |	1: ...
+ *            |	L: ...		      |
+ *            |			      |
+ *            |			      |	L: <br-stmts>
+ *            |			      |	   jmp 1b
+ *            |                       |
+ * -----------+-----------------------+------------------
+ *            |                       |
+ *  false (0) |	   ...		      |	   ...
+ *            |    JMP L	      |	   NOP
+ *            |    <br-stmts>	      |	1: ...
+ *            |	L: ...		      |
+ *            |			      |
+ *            |			      |	L: <br-stmts>
+ *            |			      |	   jmp 1b
+ *            |                       |
+ * -----------+-----------------------+------------------
+ *
+ * The initial value is encoded in the LSB of static_key::entries,
+ * type: 0 = false, 1 = true.
+ *
+ * The branch type is encoded in the LSB of jump_entry::key,
+ * branch: 0 = unlikely, 1 = likely.
+ *
+ * This gives the following logic table:
+ *
+ *	enabled	type	branch	  instuction
+ * -----------------------------+-----------
+ *	0	0	0	| NOP
+ *	0	0	1	| JMP
+ *	0	1	0	| NOP
+ *	0	1	1	| JMP
+ *
+ *	1	0	0	| JMP
+ *	1	0	1	| NOP
+ *	1	1	0	| JMP
+ *	1	1	1	| NOP
+ *
+ * Which gives the following functions:
+ *
+ *   dynamic: instruction = enabled ^ branch
+ *   static:  instruction = type ^ branch
+ *
+ * See jump_label_type() / jump_label_init_type().
+ */
+
+extern bool ____wrong_branch_error(void);
+
+#define static_branch_likely(x)							\
+({										\
+	bool branch;								\
+	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
+		branch = !arch_static_branch(&(x)->key, true);			\
+	else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
+		branch = !arch_static_branch_jump(&(x)->key, true);		\
+	else									\
+		branch = ____wrong_branch_error();				\
+	branch;									\
+})
+
+#define static_branch_unlikely(x)						\
+({										\
+	bool branch;								\
+	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
+		branch = arch_static_branch_jump(&(x)->key, false);		\
+	else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
+		branch = arch_static_branch(&(x)->key, false);			\
+	else									\
+		branch = ____wrong_branch_error();				\
+	branch;									\
+})
+
+#else /* !HAVE_JUMP_LABEL */
+
+#define static_branch_likely(x)		likely(static_key_enabled(&(x)->key))
+#define static_branch_unlikely(x)	unlikely(static_key_enabled(&(x)->key))
+
+#endif /* HAVE_JUMP_LABEL */
+
+/*
+ * Advanced usage; refcount, branch is enabled when: count != 0
+ */
+
+#define static_branch_inc(x)		static_key_slow_inc(&(x)->key)
+#define static_branch_dec(x)		static_key_slow_dec(&(x)->key)
+
+/*
+ * 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)
+
 #endif	/* _LINUX_JUMP_LABEL_H */
 
 #endif /* __ASSEMBLY__ */
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -165,16 +165,32 @@ static inline bool static_key_type(struc
 
 static inline struct static_key *jump_entry_key(struct jump_entry *entry)
 {
-	return (struct static_key *)((unsigned long)entry->key);
+	return (struct static_key *)((unsigned long)entry->key & ~1UL);
+}
+
+static bool jump_entry_branch(struct jump_entry *entry)
+{
+	return (unsigned long)entry->key & 1UL;
 }
 
 static enum jump_label_type jump_label_type(struct jump_entry *entry)
 {
 	struct static_key *key = jump_entry_key(entry);
 	bool enabled = static_key_enabled(key);
+	bool branch = jump_entry_branch(entry);
+
+	/* See the comment in linux/jump_label.h */
+	return enabled ^ branch;
+}
+
+static enum jump_label_type jump_label_init_type(struct jump_entry *entry)
+{
+	struct static_key *key = jump_entry_key(entry);
 	bool type = static_key_type(key);
+	bool branch = jump_entry_branch(entry);
 
-	return enabled ^ type;
+	/* See the comment in linux/jump_label.h */
+	return type ^ branch;
 }
 
 static void __jump_label_update(struct static_key *key,
@@ -205,7 +221,10 @@ void __init jump_label_init(void)
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
-		arch_jump_label_transform_static(iter, jump_label_type(iter));
+		/* rewrite NOPs */
+		if (jump_label_type(iter) == JUMP_LABEL_NOP)
+			arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
+
 		iterk = jump_entry_key(iter);
 		if (iterk == key)
 			continue;
@@ -276,8 +295,11 @@ void jump_label_apply_nops(struct module
 	if (iter_start == iter_stop)
 		return;
 
-	for (iter = iter_start; iter < iter_stop; iter++)
-		arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
+	for (iter = iter_start; iter < iter_stop; iter++) {
+		/* Only write NOPs for arch_branch_static(). */
+		if (jump_label_init_type(iter) == JUMP_LABEL_NOP)
+			arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
+	}
 }
 
 static int jump_label_add_module(struct module *mod)
@@ -318,7 +340,8 @@ static int jump_label_add_module(struct
 		jlm->next = key->next;
 		key->next = jlm;
 
-		if (jump_label_type(iter) == JUMP_LABEL_JMP)
+		/* Only update if we've changed from our initial state */
+		if (jump_label_type(iter) != jump_label_init_type(iter))
 			__jump_label_update(key, iter, iter_stop);
 	}
 

[PATCH -v2 7/8] jump_label: Add selftest

[Peter Zijlstra]

[-- Attachment #1: peterz-sk-test.patch --]
[-- Type: text/plain, Size: 1925 bytes --]

Add a little selftest that validates all combinations.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/Kconfig        |    6 ++++++
 kernel/jump_label.c |   39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -71,6 +71,12 @@ config JUMP_LABEL
 	 ( On 32-bit x86, the necessary options added to the compiler
 	   flags may increase the size of the kernel slightly. )
 
+config JUMP_LABEL_SELFTEST
+	bool "Static key selftest"
+	depends on JUMP_LABEL
+	help
+	  Boot time self-test of the branch patching code.
+
 config OPTPROBES
 	def_bool y
 	depends on KPROBES && HAVE_OPTPROBES
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -482,4 +482,41 @@ static void jump_label_update(struct sta
 		__jump_label_update(key, entry, stop);
 }
 
-#endif
+#ifdef CONFIG_JUMP_LABEL_SELFTEST
+static DEFINE_STATIC_KEY_TRUE(sk_true);
+static DEFINE_STATIC_KEY_FALSE(sk_false);
+
+static __init int jump_label_test(void)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		WARN_ON(static_key_enabled(&sk_true.key) != true);
+		WARN_ON(static_key_enabled(&sk_false.key) != false);
+
+		WARN_ON(!static_branch_likely(&sk_true));
+		WARN_ON(!static_branch_unlikely(&sk_true));
+		WARN_ON(static_branch_likely(&sk_false));
+		WARN_ON(static_branch_unlikely(&sk_false));
+
+		static_branch_disable(&sk_true);
+		static_branch_enable(&sk_false);
+
+		WARN_ON(static_key_enabled(&sk_true.key) == true);
+		WARN_ON(static_key_enabled(&sk_false.key) == false);
+
+		WARN_ON(static_branch_likely(&sk_true));
+		WARN_ON(static_branch_unlikely(&sk_true));
+		WARN_ON(!static_branch_likely(&sk_false));
+		WARN_ON(!static_branch_unlikely(&sk_false));
+
+		static_branch_enable(&sk_true);
+		static_branch_disable(&sk_false);
+	}
+
+	return 0;
+}
+late_initcall(jump_label_test);
+#endif /* JUMP_LABEL_SELFTEST */
+
+#endif /* HAVE_JUMP_LABEL */

[PATCH -v2 8/8] x86, tsc: Employ static_branch_likely()

[Peter Zijlstra]

[-- Attachment #1: peterz-static-key-use-new.patch --]
[-- Type: text/plain, Size: 1811 bytes --]

Because of the static_key restrictions we had to take an unconditional
jump for the most likely case, causing $I bloat.

Rewrite to use the new primitives.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/tsc.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -38,7 +38,7 @@ static int __read_mostly tsc_unstable;
    erroneous rdtsc usage on !cpu_has_tsc processors */
 static int __read_mostly tsc_disabled = -1;
 
-static struct static_key __use_tsc = STATIC_KEY_INIT;
+static DEFINE_STATIC_KEY_FALSE(__use_tsc);
 
 int tsc_clocksource_reliable;
 
@@ -274,7 +274,12 @@ static void set_cyc2ns_scale(unsigned lo
  */
 u64 native_sched_clock(void)
 {
-	u64 tsc_now;
+	if (static_branch_likely(&__use_tsc)) {
+		u64 tsc_now = rdtsc();
+
+		/* return the value in ns */
+		return cycles_2_ns(tsc_now);
+	}
 
 	/*
 	 * Fall back to jiffies if there's no TSC available:
@@ -284,16 +289,9 @@ u64 native_sched_clock(void)
 	 *   very important for it to be as fast as the platform
 	 *   can achieve it. )
 	 */
-	if (!static_key_false(&__use_tsc)) {
-		/* No locking but a rare wrong value is not a big deal: */
-		return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
-	}
-
-	/* read the Time Stamp Counter: */
-	tsc_now = rdtsc();
 
-	/* return the value in ns */
-	return cycles_2_ns(tsc_now);
+	/* No locking but a rare wrong value is not a big deal: */
+	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
 }
 
 /*
@@ -1212,7 +1210,7 @@ void __init tsc_init(void)
 	/* now allow native_sched_clock() to use rdtsc */
 
 	tsc_disabled = 0;
-	static_key_slow_inc(&__use_tsc);
+	static_branch_enable(&__use_tsc);
 
 	if (!no_sched_irq_time)
 		enable_sched_clock_irqtime();

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Rabin Vincent]

On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
> --- a/arch/arm/include/asm/jump_label.h
> +++ b/arch/arm/include/asm/jump_label.h
> @@ -7,20 +7,28 @@
>  
>  #define JUMP_LABEL_NOP_SIZE 4
>  
> -#ifdef CONFIG_THUMB2_KERNEL
> -#define JUMP_LABEL_NOP	"nop.w"
> -#else
> -#define JUMP_LABEL_NOP	"nop"
> -#endif
> +static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
> +{
> +	asm_volatile_goto("1:\n\t"
> +		 WASM(nop) "\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 ".word 1b, %l[l_yes], %c0\n\t"
> +		 ".popsection\n\t"
> +		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
>  {
>  	asm_volatile_goto("1:\n\t"
> -		 JUMP_LABEL_NOP "\n\t"
> +		 WASM(b) " %l[l_yes]\n\t"
>  		 ".pushsection __jump_table,  \"aw\"\n\t"
>  		 ".word 1b, %l[l_yes], %c0\n\t"
>  		 ".popsection\n\t"
> -		 : :  "i" (key) :  : l_yes);
> +		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
>  
>  	return false;
>  l_yes:

This is missing an include of asm/unified.h for the WASM():

diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
index f8bc12f..34f7b69 100644
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -4,6 +4,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/types.h>
+#include <asm/unified.h>
 
 #define JUMP_LABEL_NOP_SIZE 4

With that added, it builds and works fine on ARM.

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

On Tue, Jul 28, 2015 at 07:00:55PM +0200, Rabin Vincent wrote:

> 
> This is missing an include of asm/unified.h for the WASM():
> 
> diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
> index f8bc12f..34f7b69 100644
> --- a/arch/arm/include/asm/jump_label.h
> +++ b/arch/arm/include/asm/jump_label.h
> @@ -4,6 +4,7 @@
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/types.h>
> +#include <asm/unified.h>
>  
>  #define JUMP_LABEL_NOP_SIZE 4
> 
> With that added, it builds and works fine on ARM.

Duh, thanks!

Re: [PATCH -v2 7/8] jump_label: Add selftest

[Jason Baron]

On 07/28/2015 09:21 AM, Peter Zijlstra wrote:

Hi,

Funny-so I did something similar but its a modules self test
so I think its complementary. I can re-post with a changelog
and some more comments if you think its worthwhile. I have
two modules in order to test actually updating the key
during module load of the second module. It also covers
the 'legacy' interface.

In order to get the !CONFIG_JUMP_LABEL to work I needed,
the following:

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index c033595..27b335a 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -183,10 +183,10 @@ static inline int jump_label_apply_nops(struct module *mod)
     return 0;
 }
 
-#define STATIC_KEY_INIT_TRUE ((struct static_key) \
-        { .enabled = ATOMIC_INIT(1) })
-#define STATIC_KEY_INIT_FALSE ((struct static_key) \
-        { .enabled = ATOMIC_INIT(0) })
+#define STATIC_KEY_INIT_TRUE    \
+        { .enabled = ATOMIC_INIT(1) }
+#define STATIC_KEY_INIT_FALSE    \
+        { .enabled = ATOMIC_INIT(0) }
 
 #endif    /* HAVE_JUMP_LABEL */
 

Other than that, everything seems to be working fine for me
with -v2.

We probably should also update Documentation/static-keys.txt.
I can take a stab at that, if needed.

Module selftest is below.

Thanks,

-Jason

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index b908048..d87d7b6 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1835,6 +1835,15 @@ config MEMTEST
             memtest=17, mean do 17 test patterns.
       If you are unsure how to answer this question, answer N.
 
+config TEST_JUMP_LABEL
+    tristate "Test jump label"
+    default n
+    depends on m
+    help
+      Test jump labels.
+
+      If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
diff --git a/lib/Makefile b/lib/Makefile
index ff37c8c..ea20293 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -39,6 +39,8 @@ obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_LKM) += test_module.o
 obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
+obj-$(CONFIG_TEST_JUMP_LABEL) += test_jump_label.o
+obj-$(CONFIG_TEST_JUMP_LABEL) += test_jump_label_base.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_jump_label.c b/lib/test_jump_label.c
new file mode 100644
index 0000000..5a5eb86
--- /dev/null
+++ b/lib/test_jump_label.c
@@ -0,0 +1,225 @@
+/*
+ * Kernel module for testing jump labels.
+ *
+ * Copyright 2015 Akamai Technologies Inc. All Rights Reserved
+ *
+ * Authors:
+ *      Jason Baron       <jbaron@akamai.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program 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 General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/jump_label.h>
+
+/* old keys */
+struct static_key old_true_key = STATIC_KEY_INIT_TRUE;
+struct static_key old_false_key = STATIC_KEY_INIT_FALSE;
+
+/* new api */
+DEFINE_STATIC_KEY_TRUE(true_key);
+DEFINE_STATIC_KEY_FALSE(false_key);
+
+/* external */
+extern struct static_key base_old_true_key;
+extern struct static_key base_inv_old_true_key;
+extern struct static_key base_old_false_key;
+extern struct static_key base_inv_old_false_key;
+
+/* new api */
+extern struct static_key_true base_true_key;
+extern struct static_key_true base_inv_true_key;
+extern struct static_key_false base_false_key;
+extern struct static_key_false base_inv_false_key;
+
+
+struct test_branch {
+    bool init_state;
+    struct static_key *key;
+    bool (*test_key)(void);
+};
+
+#define test_key_func(key, branch)    \
+({bool func(void) { return branch(key); } func;    })
+
+static void invert_key(struct static_key *key)
+{
+    if (static_key_enabled(key))
+        static_key_disable(key);
+    else
+        static_key_enable(key);
+}
+
+static void invert_keys(struct test_branch *branches, int size)
+{
+    struct static_key *previous = NULL;
+    int i;
+   
+    for (i = 0; i < size; i++) {
+        if (previous != branches[i].key) {
+            invert_key(branches[i].key);
+            previous = branches[i].key;
+        }
+    }
+}
+
+int verify_branches(struct test_branch *branches, int size, bool invert)
+{
+    int i;
+    bool ret, init;
+
+    for (i = 0; i < size; i++) {
+        ret = static_key_enabled(branches[i].key);
+        init = branches[i].init_state;
+        if (ret != (invert ? !init : init))
+            return -EINVAL;
+        ret = branches[i].test_key();
+        if (static_key_enabled(branches[i].key)) {
+            if (!ret)
+                return -EINVAL;
+        } else {
+            if (ret)
+                return -EINVAL;
+        }
+    }
+    return 0;
+}
+
+static int __init test_jump_label_init(void)
+{
+    int ret;
+    int size;
+
+    struct test_branch jump_label_tests[] = {
+        /* internal keys - old keys */
+        {
+            .init_state = true,
+            .key = &old_true_key,
+            .test_key = test_key_func(&old_true_key, static_key_true),
+        },
+        {
+            .init_state = false,
+            .key = &old_false_key,
+            .test_key = test_key_func(&old_false_key, static_key_false),
+        },
+        /* internal keys - new keys */
+        {
+            .init_state = true,
+            .key = &true_key.key,
+            .test_key = test_key_func(&true_key, static_branch_likely),
+        },
+        {
+            .init_state = true,
+            .key = &true_key.key,
+            .test_key = test_key_func(&true_key, static_branch_unlikely),
+        },
+        {
+            .init_state = false,
+            .key = &false_key.key,
+            .test_key = test_key_func(&false_key, static_branch_likely),
+        },
+        {
+            .init_state = false,
+            .key = &false_key.key,
+            .test_key = test_key_func(&false_key, static_branch_unlikely),
+        },
+        /* external keys - old keys */
+        {
+            .init_state = true,
+            .key = &base_old_true_key,
+            .test_key = test_key_func(&base_old_true_key, static_key_true),
+        },
+        {
+            .init_state = false,
+            .key = &base_inv_old_true_key,
+            .test_key = test_key_func(&base_inv_old_true_key, static_key_true),
+        },
+        {
+            .init_state = false,
+            .key = &base_old_false_key,
+            .test_key = test_key_func(&base_old_false_key, static_key_false),
+        },
+        {
+            .init_state = true,
+            .key = &base_inv_old_false_key,
+            .test_key = test_key_func(&base_inv_old_false_key, static_key_false),
+        },
+        /* external keys - new keys */
+        {
+            .init_state = true,
+            .key = &base_true_key.key,
+            .test_key = test_key_func(&base_true_key, static_branch_likely),
+        },
+        {
+            .init_state = true,
+            .key = &base_true_key.key,
+            .test_key = test_key_func(&base_true_key, static_branch_unlikely),
+        },
+        {
+            .init_state = false,
+            .key = &base_inv_true_key.key,
+            .test_key = test_key_func(&base_inv_true_key, static_branch_likely),
+        },
+        {
+            .init_state = false,
+            .key = &base_inv_true_key.key,
+            .test_key = test_key_func(&base_inv_true_key, static_branch_unlikely),
+        },
+        {
+            .init_state = false,
+            .key = &base_false_key.key,
+            .test_key = test_key_func(&base_false_key, static_branch_likely),
+        },
+        {
+            .init_state = false,
+            .key = &base_false_key.key,
+            .test_key = test_key_func(&base_false_key, static_branch_unlikely),
+        },
+        {
+            .init_state = true,
+            .key = &base_inv_false_key.key,
+            .test_key = test_key_func(&base_inv_false_key, static_branch_likely),
+        },
+        {
+            .init_state = true,
+            .key = &base_inv_false_key.key,
+            .test_key = test_key_func(&base_inv_false_key, static_branch_unlikely),
+        },
+    };
+
+    size = ARRAY_SIZE(jump_label_tests);
+
+    ret = verify_branches(jump_label_tests, size, false);
+    if (ret)
+        goto out;
+
+    invert_keys(jump_label_tests, size);
+    ret = verify_branches(jump_label_tests, size, true);
+    if (ret)
+        goto out;
+
+    invert_keys(jump_label_tests, size);
+    ret = verify_branches(jump_label_tests, size, false);
+    if (ret)
+        goto out;
+    return 0;
+out:
+    return ret;
+}
+
+static void __exit test_jump_label_exit(void)
+{
+}
+
+module_init(test_jump_label_init);
+module_exit(test_jump_label_exit);
+
+MODULE_AUTHOR("Jason Baron <jbaron@akamai.com>");
+MODULE_LICENSE("GPL");
diff --git a/lib/test_jump_label_base.c b/lib/test_jump_label_base.c
new file mode 100644
index 0000000..91aed8e
--- /dev/null
+++ b/lib/test_jump_label_base.c
@@ -0,0 +1,68 @@
+/*
+ * Kernel module for testing jump labels.
+ *
+ * Copyright 2015 Akamai Technologies Inc. All Rights Reserved
+ *
+ * Authors:
+ *      Jason Baron       <jbaron@akamai.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program 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 General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/jump_label.h>
+
+/* old keys */
+struct static_key base_old_true_key = STATIC_KEY_INIT_TRUE;
+EXPORT_SYMBOL_GPL(base_old_true_key);
+struct static_key base_inv_old_true_key = STATIC_KEY_INIT_TRUE;
+EXPORT_SYMBOL_GPL(base_inv_old_true_key);
+struct static_key base_old_false_key = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(base_old_false_key);
+struct static_key base_inv_old_false_key = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(base_inv_old_false_key);
+
+/* new keys */
+DEFINE_STATIC_KEY_TRUE(base_true_key);
+EXPORT_SYMBOL_GPL(base_true_key);
+DEFINE_STATIC_KEY_TRUE(base_inv_true_key);
+EXPORT_SYMBOL_GPL(base_inv_true_key);
+DEFINE_STATIC_KEY_FALSE(base_false_key);
+EXPORT_SYMBOL_GPL(base_false_key);
+DEFINE_STATIC_KEY_FALSE(base_inv_false_key);
+EXPORT_SYMBOL_GPL(base_inv_false_key);
+
+static void invert_key(struct static_key *key)
+{
+    if (static_key_enabled(key))
+        static_key_disable(key);
+    else
+        static_key_enable(key);
+}
+
+static int __init test_jump_label_base_init(void)
+{
+    invert_key(&base_inv_old_true_key);
+    invert_key(&base_inv_old_false_key);
+    invert_key(&base_inv_true_key.key);
+    invert_key(&base_inv_false_key.key);
+
+    return 0;
+}
+
+static void __exit test_jump_label_base_exit(void)
+{
+}
+
+module_init(test_jump_label_base_init);
+module_exit(test_jump_label_base_exit);
+
+MODULE_AUTHOR("Jason Baron <jbaron@akamai.com>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 24ae9e8..b8f12e0 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -20,6 +20,7 @@ ifneq (1, $(quicktest))
 TARGETS += timers
 endif
 TARGETS += user
+TARGETS += jumplabel
 TARGETS += vm
 TARGETS += x86
 #Please keep the TARGETS list alphabetically sorted
diff --git a/tools/testing/selftests/jumplabel/Makefile b/tools/testing/selftests/jumplabel/Makefile
new file mode 100644
index 0000000..7526aa5
--- /dev/null
+++ b/tools/testing/selftests/jumplabel/Makefile
@@ -0,0 +1,8 @@
+# Makefile for jump label selftests
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
+all:
+
+TEST_PROGS := test_jump_label.sh
+
+include ../lib.mk
diff --git a/tools/testing/selftests/jumplabel/test_jump_label.sh b/tools/testing/selftests/jumplabel/test_jump_label.sh
new file mode 100755
index 0000000..3457e8a
--- /dev/null
+++ b/tools/testing/selftests/jumplabel/test_jump_label.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+# Runs jump label kernel module tests
+
+if /sbin/modprobe -q test_jump_label_base; then
+    if /sbin/modprobe -q test_jump_label; then
+        echo "jump_label: ok"
+        /sbin/modprobe -q -r test_jump_label
+        /sbin/modprobe -q -r test_jump_label_base
+    else
+        echo "jump_label: [FAIL]"
+        /sbin/modprobe -q -r test_jump_label_base
+    fi
+else
+    echo "jump_label: [FAIL]"
+    exit 1
+fi
-- 
1.8.2.rc2

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Heiko Carstens]

On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
> There are various problems and short-comings with the current
> static_key interface:
> 
>  - static_key_{true,false}() read like a branch depending on the key
>    value, instead of the actual likely/unlikely branch depending on
>    init value.
> 
>  - static_key_{true,false}() are, as stated above, tied to the
>    static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
> 
>  - we're limited to the 2 (out of 4) possible options that compile to
>    a default NOP because that's what our arch_static_branch() assembly
>    emits.
> 
> So provide a new static_key interface:
> 
>   DEFINE_STATIC_KEY_TRUE(name);
>   DEFINE_STATIC_KEY_FALSE(name);
> 
> Which define a key of different types with an initial true/false
> value.
> 
> Then allow:
> 
>    static_branch_likely()
>    static_branch_unlikely()
> 
> to take a key of either type and emit the right instruction for the
> case.
> 
> This means adding a second arch_static_branch_jump() assembly helper
> which emits a JMP per default.
> 
> In order to determine the right instruction for the right state,
> encode the branch type in the LSB of jump_entry::key.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/arm/include/asm/jump_label.h     |   24 +++--
>  arch/arm64/include/asm/jump_label.h   |   18 +++-
>  arch/mips/include/asm/jump_label.h    |   19 ++++
>  arch/powerpc/include/asm/jump_label.h |   19 ++++
>  arch/s390/include/asm/jump_label.h    |   19 ++++
>  arch/sparc/include/asm/jump_label.h   |   35 ++++++--
>  arch/x86/include/asm/jump_label.h     |   21 ++++
>  include/linux/jump_label.h            |  143 ++++++++++++++++++++++++++++++++--
>  kernel/jump_label.c                   |   35 ++++++--
>  9 files changed, 294 insertions(+), 39 deletions(-)

for the s390 part:

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Re: [PATCH -v2 0/8] jump_label: Another (better) static_key interface

[Heiko Carstens]

On Tue, Jul 28, 2015 at 03:20:55PM +0200, Peter Zijlstra wrote:
> Hi all,
> 
> After yet another bug because of the weirdness of the static key interface,
> here an attempt at providing a better one.
> 
> This series is tested on x86_64 (by me) and s390x (heiko).

Works nice. You may include the s390 patch below, so the conversion to the
new interface happens when your code gets merged:

>From 3c9b5a2b9a90d6bb2b41f381f5f89b3657fe4ea5 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Wed, 29 Jul 2015 08:31:24 +0200
Subject: [PATCH] s390/uaccess: employ static_branch_likely()

Use the new static_branch_likely() primitive to make sure that the
most likely case is executed without taking an unconditional branch.
This wasn't possible with the old jump label primitives.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/lib/uaccess.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index 4614d415bb58..93cb1d09493d 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -15,7 +15,7 @@
 #include <asm/mmu_context.h>
 #include <asm/facility.h>
 
-static struct static_key have_mvcos = STATIC_KEY_INIT_FALSE;
+static DEFINE_STATIC_KEY_FALSE(have_mvcos);
 
 static inline unsigned long copy_from_user_mvcos(void *x, const void __user *ptr,
 						 unsigned long size)
@@ -104,7 +104,7 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
 
 unsigned long __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_from_user_mvcos(to, from, n);
 	return copy_from_user_mvcp(to, from, n);
 }
@@ -177,7 +177,7 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
 
 unsigned long __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_to_user_mvcos(to, from, n);
 	return copy_to_user_mvcs(to, from, n);
 }
@@ -240,7 +240,7 @@ static inline unsigned long copy_in_user_mvc(void __user *to, const void __user
 
 unsigned long __copy_in_user(void __user *to, const void __user *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_in_user_mvcos(to, from, n);
 	return copy_in_user_mvc(to, from, n);
 }
@@ -312,7 +312,7 @@ static inline unsigned long clear_user_xc(void __user *to, unsigned long size)
 
 unsigned long __clear_user(void __user *to, unsigned long size)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 			return clear_user_mvcos(to, size);
 	return clear_user_xc(to, size);
 }
@@ -386,7 +386,7 @@ early_param("uaccess_primary", parse_uaccess_pt);
 static int __init uaccess_init(void)
 {
 	if (!uaccess_primary && test_facility(27))
-		static_key_slow_inc(&have_mvcos);
+		static_branch_enable(&have_mvcos);
 	return 0;
 }
 early_initcall(uaccess_init);
-- 
2.3.8

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Vlastimil Babka]

On 07/28/2015 03:21 PM, Peter Zijlstra wrote:
> +/* -------------------------------------------------------------------------- */
> +
> +/*
> + * Two type wrappers around static_key, such that we can use compile time
> + * type differentiation to emit the right code.
> + *
> + * All the below code is macros in order to play type games.
> + */
> +
> +struct static_key_true {
> +	struct static_key key;
> +};
> +
> +struct static_key_false {
> +	struct static_key key;
> +};
> +
> +#define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
> +#define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }

How would one define a static key that's e.g. expected to be mostly false, but
with initial value of true, e.g. during boot?

Is the following legal? Should there be an API wrapper as well?

(struct static_key_false){ .key = STATIC_KEY_INIT_TRUE, }

Otherwise the new API seems like a big improvement, thanks :)

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:

> How would one define a static key that's e.g. expected to be mostly false, but
> with initial value of true, e.g. during boot?

DEFINE_STATIC_KEY_TRUE(blah);

will get you the true at boot time.

You'll then want to use:

	if (static_branch_unlikely(&blah)) {
		/* code that mostly doesn't happen */
	}

To indicate you expect it to be false most of the time. And you'll flip
it to false at runtime using:

	static_branch_disable(&blah);

If GCC co-operates, the body of the branch will be placed out-of-line,
we'll emit a jump to it by default, but once you disable it, we'll nop
the jump and fall straight through.

Re: [PATCH -v2 7/8] jump_label: Add selftest

[Peter Zijlstra]

On Tue, Jul 28, 2015 at 05:46:40PM -0400, Jason Baron wrote:
> On 07/28/2015 09:21 AM, Peter Zijlstra wrote:

> In order to get the !CONFIG_JUMP_LABEL to work I needed,
> the following:
> 
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index c033595..27b335a 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -183,10 +183,10 @@ static inline int jump_label_apply_nops(struct module *mod)
>      return 0;
>  }
>  
> -#define STATIC_KEY_INIT_TRUE ((struct static_key) \
> -        { .enabled = ATOMIC_INIT(1) })
> -#define STATIC_KEY_INIT_FALSE ((struct static_key) \
> -        { .enabled = ATOMIC_INIT(0) })
> +#define STATIC_KEY_INIT_TRUE    \
> +        { .enabled = ATOMIC_INIT(1) }
> +#define STATIC_KEY_INIT_FALSE    \
> +        { .enabled = ATOMIC_INIT(0) }
>  
>  #endif    /* HAVE_JUMP_LABEL */
>  

Right, I still need to figure out why GCC thinks its not a constant
with that typecast present.

Thanks.

> Other than that, everything seems to be working fine for me
> with -v2.
> 
> We probably should also update Documentation/static-keys.txt.
> I can take a stab at that, if needed.

Ah, yes please. Also the blob at the top of jump_label.h needs some TLC.

> Module selftest is below.
> 

You put a _lot_ more effort in it than me, and it does indeed cover
more, so sure we can do that.

Re: [PATCH -v2 0/8] jump_label: Another (better) static_key interface

[Peter Zijlstra]

On Wed, Jul 29, 2015 at 08:46:00AM +0200, Heiko Carstens wrote:
> On Tue, Jul 28, 2015 at 03:20:55PM +0200, Peter Zijlstra wrote:
> > Hi all,
> > 
> > After yet another bug because of the weirdness of the static key interface,
> > here an attempt at providing a better one.
> > 
> > This series is tested on x86_64 (by me) and s390x (heiko).
> 
> Works nice. You may include the s390 patch below, so the conversion to the
> new interface happens when your code gets merged:
> 
> From 3c9b5a2b9a90d6bb2b41f381f5f89b3657fe4ea5 Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date: Wed, 29 Jul 2015 08:31:24 +0200
> Subject: [PATCH] s390/uaccess: employ static_branch_likely()
> 
> Use the new static_branch_likely() primitive to make sure that the
> most likely case is executed without taking an unconditional branch.
> This wasn't possible with the old jump label primitives.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Thanks!

Re: [PATCH -v2 2/8] jump_label: Rename JUMP_LABEL_TYPE_*

[Borislav Petkov]

On Tue, Jul 28, 2015 at 03:20:57PM +0200, Peter Zijlstra wrote:
> Rename the JUMP_LABEL_TYPE_* macros to be JUMP_TYPE_* and move the
> inline helpers into kernel/jump_label.c, since that's the only place
> they're ever used.
> 
> Also rename the helpers.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/jump_label.h |   25 +++++--------------------
>  kernel/jump_label.c        |   25 ++++++++++++++++---------
>  2 files changed, 21 insertions(+), 29 deletions(-)

/me likes.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH -v2 8/8] x86, tsc: Employ static_branch_likely()

[Borislav Petkov]

On Tue, Jul 28, 2015 at 03:21:03PM +0200, Peter Zijlstra wrote:
> Because of the static_key restrictions we had to take an unconditional
> jump for the most likely case, causing $I bloat.
> 
> Rewrite to use the new primitives.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/tsc.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -38,7 +38,7 @@ static int __read_mostly tsc_unstable;
>     erroneous rdtsc usage on !cpu_has_tsc processors */
>  static int __read_mostly tsc_disabled = -1;
>  
> -static struct static_key __use_tsc = STATIC_KEY_INIT;
> +static DEFINE_STATIC_KEY_FALSE(__use_tsc);
>  
>  int tsc_clocksource_reliable;
>  
> @@ -274,7 +274,12 @@ static void set_cyc2ns_scale(unsigned lo
>   */
>  u64 native_sched_clock(void)
>  {
> -	u64 tsc_now;
> +	if (static_branch_likely(&__use_tsc)) {
> +		u64 tsc_now = rdtsc();
> +
> +		/* return the value in ns */
> +		return cycles_2_ns(tsc_now);
> +	}

Hallelujah, this asm finally looks good:

native_sched_clock:
	pushq	%rbp	#
	movq	%rsp, %rbp	#,
	andq	$-16, %rsp	#,
#APP
# 36 "./arch/x86/include/asm/jump_label.h" 1
	1:.byte 0xe9
	 .long .L121 - 2f	#
	2:
	.pushsection __jump_table,  "aw" 
	 .balign 8 
	 .quad 1b, .L121, __use_tsc+1 	#,
	.popsection 

# 0 "" 2
# 124 "./arch/x86/include/asm/msr.h" 1
	rdtsc
# 0 "" 2
#NO_APP

	...

        leave
        ret
.L121:
        imulq   $1000000, jiffies_64(%rip), %rdx        #, jiffies_64, D.28480
        movabsq $-4294667296000000, %rax        #, tmp135
        leave
        addq    %rdx, %rax      # D.28480, D.28480
        ret

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Michael Ellerman]

On Tue, 2015-07-28 at 15:21 +0200, Peter Zijlstra wrote:
> There are various problems and short-comings with the current
> static_key interface:
...
> ---
>  arch/powerpc/include/asm/jump_label.h |   19 ++++

This looks sane and seems to be working, so powerpc bits:

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


cheers

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Michael Ellerman]

On Tue, 2015-07-28 at 15:21 +0200, Peter Zijlstra wrote:
> There are various problems and short-comings with the current
> static_key interface:
...
> ---
>  arch/powerpc/include/asm/jump_label.h |   19 ++++

This looks sane and seems to be working, so powerpc bits:

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


cheers

[tip:locking/core] s390/uaccess, locking/static_keys: employ static_branch_likely()

[tip-bot for Heiko Carstens]

Commit-ID:  ed79e946732e5311934d7f404b3b4e702e45cb97
Gitweb:     http://git.kernel.org/tip/ed79e946732e5311934d7f404b3b4e702e45cb97
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Wed, 29 Jul 2015 08:31:24 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 3 Aug 2015 11:34:17 +0200

s390/uaccess, locking/static_keys: employ static_branch_likely()

Use the new static_branch_likely() primitive to make sure that the
most likely case is executed without taking an unconditional branch.
This wasn't possible with the old jump label primitives.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
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: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150729064600.GB3953@osiris
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/s390/lib/uaccess.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index 4614d41..93cb1d0 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -15,7 +15,7 @@
 #include <asm/mmu_context.h>
 #include <asm/facility.h>
 
-static struct static_key have_mvcos = STATIC_KEY_INIT_FALSE;
+static DEFINE_STATIC_KEY_FALSE(have_mvcos);
 
 static inline unsigned long copy_from_user_mvcos(void *x, const void __user *ptr,
 						 unsigned long size)
@@ -104,7 +104,7 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
 
 unsigned long __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_from_user_mvcos(to, from, n);
 	return copy_from_user_mvcp(to, from, n);
 }
@@ -177,7 +177,7 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
 
 unsigned long __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_to_user_mvcos(to, from, n);
 	return copy_to_user_mvcs(to, from, n);
 }
@@ -240,7 +240,7 @@ static inline unsigned long copy_in_user_mvc(void __user *to, const void __user
 
 unsigned long __copy_in_user(void __user *to, const void __user *from, unsigned long n)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 		return copy_in_user_mvcos(to, from, n);
 	return copy_in_user_mvc(to, from, n);
 }
@@ -312,7 +312,7 @@ static inline unsigned long clear_user_xc(void __user *to, unsigned long size)
 
 unsigned long __clear_user(void __user *to, unsigned long size)
 {
-	if (static_key_false(&have_mvcos))
+	if (static_branch_likely(&have_mvcos))
 			return clear_user_mvcos(to, size);
 	return clear_user_xc(to, size);
 }
@@ -386,7 +386,7 @@ early_param("uaccess_primary", parse_uaccess_pt);
 static int __init uaccess_init(void)
 {
 	if (!uaccess_primary && test_facility(27))
-		static_key_slow_inc(&have_mvcos);
+		static_branch_enable(&have_mvcos);
 	return 0;
 }
 early_initcall(uaccess_init);

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Steven Rostedt]

On Wed, 29 Jul 2015 10:49:06 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:
> 
> > How would one define a static key that's e.g. expected to be mostly false, but
> > with initial value of true, e.g. during boot?
> 
> DEFINE_STATIC_KEY_TRUE(blah);
> 
> will get you the true at boot time.
> 
> You'll then want to use:
> 
> 	if (static_branch_unlikely(&blah)) {
> 		/* code that mostly doesn't happen */
> 	}
> 
> To indicate you expect it to be false most of the time. And you'll flip
> it to false at runtime using:
> 
> 	static_branch_disable(&blah);

I wonder if static_branch_set_false(&blah) would be a better name to
understand. What does "disable" / "enable" mean?

If we declare it "TRUE" when defining it, it only makes sense to change
it to "false" later on.

-- Steve


> 
> If GCC co-operates, the body of the branch will be placed out-of-line,
> we'll emit a jump to it by default, but once you disable it, we'll nop
> the jump and fall straight through.

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote:

> I wonder if static_branch_set_false(&blah) would be a better name to
> understand. What does "disable" / "enable" mean?

"make false" / "make true" ? Check a local dictionary.

http://lmgtfy.com/?q=enable

"2. computing: make (a device or system) operational; active"

A value can be true/false, an action that makes true/false is
enable/disable.

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Steven Rostedt]

On Mon, 3 Aug 2015 21:18:16 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote:
> 
> > I wonder if static_branch_set_false(&blah) would be a better name to
> > understand. What does "disable" / "enable" mean?
> 
> "make false" / "make true" ? Check a local dictionary.
> 
> http://lmgtfy.com/?q=enable

I know the definition on enable :-p

> 
> "2. computing: make (a device or system) operational; active"
> 
> A value can be true/false, an action that makes true/false is
> enable/disable.

enable is more "activate" and disable is more "deactivate" not "make
true" and "make false". It's subtle, but there is a difference. Try
switching it around in other contexts. One could "disable networking"
but saying "make networking false" doesn't make sense.

Technically, one can think: "activate the branch", but we are
activating not the branch, but the jump label itself. It's not as clear
as setting it to "true" or "false".

What the static_branch does is already confusing enough, we should try
to use the terminology that is as clear as possible.

"set_true" is more understandable than "enable" when one can question,
what exactly are we "enabling"?

-- Steve

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote:
> Technically, one can think: "activate the branch", but we are
> activating not the branch, but the jump label itself.

No you are enabling the branch, you're making the branch body active.

There is no enable/disable/true/false for the jump label, only NOP or
JUMP, and either can result in an active branch body.

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Steven Rostedt]

On Mon, 3 Aug 2015 22:00:02 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote:
> > Technically, one can think: "activate the branch", but we are
> > activating not the branch, but the jump label itself.
> 
> No you are enabling the branch, you're making the branch body active.

By making the statement "true".

Otherwise we could just have:

	static_branch_likely(&blah) {
		[..]
	}

And remove the "if".

Then it would make sense to enable or disable it.

> 
> There is no enable/disable/true/false for the jump label, only NOP or
> JUMP, and either can result in an active branch body.

That's implementation details, not a general concept that users will
need to know about.

-- Steve

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Borislav Petkov]

On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote:
> That's implementation details, not a general concept that users will
> need to know about.

Why?

It is a branch, regardless of which insn is used on which arch - it is
either active and you *branch* to that code or *inactive* and you don't.
So now it is actually what it should've been from the beginning...

I realize simplifying the terminology around those jump labels/static
branches things comes kinda unnatural now.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Andy Lutomirski]

On Mon, Aug 3, 2015 at 8:37 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote:
>> That's implementation details, not a general concept that users will
>> need to know about.
>
> Why?
>
> It is a branch, regardless of which insn is used on which arch - it is
> either active and you *branch* to that code or *inactive* and you don't.
> So now it is actually what it should've been from the beginning...

Except that, with the new interface, static_key_likely is the other
way around, right?  If the key is true (i.e. enabled), then it doesn't
branch.

I think of the key as a boolean thing that happens to work by code
patching under the hood.  The fancy patching affects the performance
but doesn't really make it functionally different from a regular
variable.  How about making it extra explicit:

static_key_set(&key, value);

where value is a bool or maybe even an unsigned int?

--Andy

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Borislav Petkov]

On Mon, Aug 03, 2015 at 09:07:53PM -0700, Andy Lutomirski wrote:
> Except that, with the new interface, static_key_likely is the other
> way around, right?  If the key is true (i.e. enabled), then it doesn't
> branch.
> 
> I think of the key as a boolean thing that happens to work by code
> patching under the hood.  The fancy patching affects the performance
> but doesn't really make it functionally different from a regular
> variable.  How about making it extra explicit:
> 
> static_key_set(&key, value);
> 
> where value is a bool or maybe even an unsigned int?

Let's have an actual example:

+       if (static_branch_likely(&__use_tsc)) {
+               u64 tsc_now = rdtsc();
+
+               /* return the value in ns */
+               return cycles_2_ns(tsc_now);
+       }

Well, I can see how the likely/unlikely things can confuse. They
actually don't have anything to do with where we will branch to but how
the code will be laid out, AFAICT. So I'm reading this as:

	if (use_tsc)) {
		RDTSC;
		return;
	}

and then it is straightforward.

So in this case, the jump will be disabled and we won't branch anywhere.
It actually becomes:

	RDTSC;
	return;

which can't get any more optimal than it is.

Hmm, yeah, I see how that can be confusing... But the asm is finally
fine. Hey, at least one thing...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[yalin wang]

> On Jul 28, 2015, at 21:21, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> There are various problems and short-comings with the current
> static_key interface:
> 
> - static_key_{true,false}() read like a branch depending on the key
>   value, instead of the actual likely/unlikely branch depending on
>   init value.
> 
> - static_key_{true,false}() are, as stated above, tied to the
>   static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
> 
> - we're limited to the 2 (out of 4) possible options that compile to
>   a default NOP because that's what our arch_static_branch() assembly
>   emits.
> 
> So provide a new static_key interface:
> 
>  DEFINE_STATIC_KEY_TRUE(name);
>  DEFINE_STATIC_KEY_FALSE(name);
> 
> Which define a key of different types with an initial true/false
> value.
> 
> Then allow:
> 
>   static_branch_likely()
>   static_branch_unlikely()
> 
> to take a key of either type and emit the right instruction for the
> case.
> 
> This means adding a second arch_static_branch_jump() assembly helper
> which emits a JMP per default.
> 
> In order to determine the right instruction for the right state,
> encode the branch type in the LSB of jump_entry::key.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> 
is this means static_key_{true,false}() are deprecated ?
do you need mark static_key_{true,false}() as deprecated?
like this:
static __always_inline  __deprecated bool static_key_false(struct static_key *key)  ?
Thanks

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Peter Zijlstra]

On Tue, Aug 04, 2015 at 02:50:32PM +0800, yalin wang wrote:
> is this means static_key_{true,false}() are deprecated ?

Yes.

> do you need mark static_key_{true,false}() as deprecated?
> like this:
> static __always_inline  __deprecated bool static_key_false(struct static_key *key)  ?

Not until I (or someone else) has made an effort to convert most (if not
all) current users of that interface.

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Steven Rostedt]

On Tue, 4 Aug 2015 05:37:33 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote:
> > That's implementation details, not a general concept that users will
> > need to know about.
> 
> Why?
> 
> It is a branch, regardless of which insn is used on which arch - it is
> either active and you *branch* to that code or *inactive* and you don't.
> So now it is actually what it should've been from the beginning...

I just don't like the inconsistency of the initialization and the
setting.

Either have:

 DEFINE_STATIC_KEY_TRUE()
 DEFINE_STATIC_KEY_FALSE()

and

 static_branch_set_true()
 static_branch_set_false()


or have:

 DEFINE_STATIC_KEY_ENABLED()
 DEFINE_STATIC_KEY_DISABLED()

and

 static_branch_enable()
 static_branch_disable()


But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is
confusing, as enable does not mean "make true"!

This may seem as bike shedding, but terminology *is* important, and
being inconsistent just makes it more probable to have bugs.

-- Steve

> 
> I realize simplifying the terminology around those jump labels/static
> branches things comes kinda unnatural now.
> 

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Borislav Petkov]

On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote:
> I just don't like the inconsistency of the initialization and the
> setting.
> 
> Either have:
> 
>  DEFINE_STATIC_KEY_TRUE()
>  DEFINE_STATIC_KEY_FALSE()
> 
> and
> 
>  static_branch_set_true()
>  static_branch_set_false()
> 
> 
> or have:
> 
>  DEFINE_STATIC_KEY_ENABLED()
>  DEFINE_STATIC_KEY_DISABLED()
> 
> and
> 
>  static_branch_enable()
>  static_branch_disable()
> 
> 
> But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is
> confusing, as enable does not mean "make true"!
> 
> This may seem as bike shedding, but terminology *is* important, and
> being inconsistent just makes it more probable to have bugs.

I absolutely agree but I read "enable" as enable the branch, so no
confusion there. Now, it's a whole another question where we branch to.
And that can be confusing.

Now, let's get back to our example:

+static DEFINE_STATIC_KEY_FALSE(__use_tsc);

We don't use the TSC by default. And that's correct, we need to
calibrate it first.

After calibration:

+       static_branch_enable(&__use_tsc);

Now here we can get confused: we enable the branch but where we branch
to? The key name helps here but it is still not quite 100% clear. I'd
prefer to have:

	static_enable(&__use_tsc);

which basically says, we can use the TSC from now on. No branch, no key,
no nada. It looks like a boolean variable of sorts which says, use the
TSC from now on.

Which equally speaks for your other version:

	static_set_true(&__use_tsc);

Now this looks pretty understandable to me.


Then, the usage site looks like this:

+       if (static_likely(&__use_tsc)) {
+               u64 tsc_now = rdtsc();
+
+               /* return the value in ns */
+               return cycles_2_ns(tsc_now);
+       }

which basically says two things:

* if the static key is enabled, i.e. the boolean var is set to true.

and

* this is a likely key, i.e., the code in brackets should come first in
the layout and the code we branch to comes later.

Hell, we can drop that "key" or "branch" from the whole API for all I
know. "static_" is enough for me to say what the thing is. But that's
just me - I like short names - no poems in code and sh*t.

Thoughts, comments?

So yeah, I absolutely see the problematic here and also the need for
more bikeshedding. And this time, that bikeshedding is important.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Steven Rostedt]

On Tue, 4 Aug 2015 16:33:44 +0200
Borislav Petkov <bp@alien8.de> wrote:

> Hell, we can drop that "key" or "branch" from the whole API for all I
> know. "static_" is enough for me to say what the thing is. But that's
> just me - I like short names - no poems in code and sh*t.
> 
> Thoughts, comments?

I agree to get rid of the "key" and the "branch", I never liked them in
the first place.

I prefer static_set_true/false() but I'm also fine with
static_enable/disable() as long as the initializers are consistent.


> 
> So yeah, I absolutely see the problematic here and also the need for
> more bikeshedding. And this time, that bikeshedding is important.
> 

Right, because the broken part of this nuclear plant, was the bike
shed, and it's color was so ugly that the people in the nuclear plant
kept making mistakes by being distracted by that damn shed!

-- Steve

Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

[Andy Lutomirski]

On Tue, Aug 4, 2015 at 7:33 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote:
>> I just don't like the inconsistency of the initialization and the
>> setting.
>>
>> Either have:
>>
>>  DEFINE_STATIC_KEY_TRUE()
>>  DEFINE_STATIC_KEY_FALSE()
>>
>> and
>>
>>  static_branch_set_true()
>>  static_branch_set_false()
>>
>>
>> or have:
>>
>>  DEFINE_STATIC_KEY_ENABLED()
>>  DEFINE_STATIC_KEY_DISABLED()
>>
>> and
>>
>>  static_branch_enable()
>>  static_branch_disable()
>>
>>
>> But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is
>> confusing, as enable does not mean "make true"!
>>
>> This may seem as bike shedding, but terminology *is* important, and
>> being inconsistent just makes it more probable to have bugs.
>
> I absolutely agree but I read "enable" as enable the branch, so no
> confusion there. Now, it's a whole another question where we branch to.
> And that can be confusing.
>
> Now, let's get back to our example:
>
> +static DEFINE_STATIC_KEY_FALSE(__use_tsc);
>
> We don't use the TSC by default. And that's correct, we need to
> calibrate it first.
>
> After calibration:
>
> +       static_branch_enable(&__use_tsc);
>
> Now here we can get confused: we enable the branch but where we branch
> to? The key name helps here but it is still not quite 100% clear. I'd
> prefer to have:
>
>         static_enable(&__use_tsc);

If everything's consistent about "static_key", then I still like
"static_key_set_true" or "static_key_set".  "static_key_enable" is
okay but not fantastic IMO, and "static_branch_enable" is just
confusing.

--Andy