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) <[email protected]>
---
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);
}
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.
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!
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) <[email protected]>
> ---
> 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 <[email protected]>
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 :)
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.
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 <[email protected]>
cheers
On Wed, 29 Jul 2015 10:49:06 +0200
Peter Zijlstra <[email protected]> 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.
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.
On Mon, 3 Aug 2015 21:18:16 +0200
Peter Zijlstra <[email protected]> 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
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.
On Mon, 3 Aug 2015 22:00:02 +0200
Peter Zijlstra <[email protected]> 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
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.
--
On Mon, Aug 3, 2015 at 8:37 PM, Borislav Petkov <[email protected]> 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
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.
--
> On Jul 28, 2015, at 21:21, Peter Zijlstra <[email protected]> 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) <[email protected]>
> ---
>
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
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.
On Tue, 4 Aug 2015 05:37:33 +0200
Borislav Petkov <[email protected]> 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.
>
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.
--
On Tue, 4 Aug 2015 16:33:44 +0200
Borislav Petkov <[email protected]> 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
On Tue, Aug 4, 2015 at 7:33 AM, Borislav Petkov <[email protected]> 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