jump-label: allow early jump_label_enable()

From: Jeremy Fitzhardinge <[email protected]>

It is no longer used.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 arch/x86/kernel/jump_label.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

arch/x86/kernel/jump_label.c +0 -0

index 3fee346..2ad0298 100644

From: Jeremy Fitzhardinge <[email protected]>

When updating a newly loaded module, the code is definitely not yet executing on any processor, so it can be updated with no need for any heavyweight synchronization.

This patch adds arch_jump_label_static() which is implemented as arch_jump_label_transform() by default, but architectures can override it if it avoids, say, a call to stop_machine().

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 include/linux/jump_label.h |    2 ++
 kernel/jump_label.c        |   18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

include/linux/jump_label.h +2 -0

index 12e804e..56594e4 100644
@@ -45,6 +45,8 @@ extern void jump_label_lock(void);
 extern void jump_label_unlock(void);
 extern void arch_jump_label_transform(struct jump_entry *entry,
 				      enum jump_label_type type);
+extern void arch_jump_label_transform_static(struct jump_entry *entry,
+					     enum jump_label_type type);
 extern int jump_label_text_reserved(void *start, void *end);
 extern void jump_label_inc(struct jump_label_key *key);
 extern void jump_label_dec(struct jump_label_key *key);

kernel/jump_label.c +14 -2

index 059202d5..ff2028f 100644
@@ -104,6 +104,18 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
 	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
+ * cheaper.
+ */
+void __weak arch_jump_label_transform_static(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	arch_jump_label_transform(entry, type);	
+}
+
 static void __jump_label_update(struct jump_label_key *key,
 				struct jump_entry *entry,
 				struct jump_entry *stop, int enable)

@@ -135,8 +147,8 @@ static __init int jump_label_init(void)
 		struct jump_label_key *iterk;
 
 		iterk = (struct jump_label_key *)(unsigned long)iter->key;
-		arch_jump_label_transform(iter, jump_label_enabled(iterk) ?
-					  JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
+		arch_jump_label_transform_static(iter, jump_label_enabled(iterk) ?
+						 JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
 		if (iterk == key)
 			continue;
 

Quoted 65 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

When updating a newly loaded module, the code is definitely not yet executing on any processor, so it can be updated with no need for any heavyweight synchronization.

This patch adds arch_jump_label_static() which is implemented as arch_jump_label_transform() by default, but architectures can override it if it avoids, say, a call to stop_machine().

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 include/linux/jump_label.h |    2 ++
 kernel/jump_label.c        |   18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

include/linux/jump_label.h +2 -0

index 12e804e..56594e4 100644
@@ -45,6 +45,8 @@ extern void jump_label_lock(void);
 extern void jump_label_unlock(void);
 extern void arch_jump_label_transform(struct jump_entry *entry,
 				      enum jump_label_type type);
+extern void arch_jump_label_transform_static(struct jump_entry *entry,
+					     enum jump_label_type type);
 extern int jump_label_text_reserved(void *start, void *end);
 extern void jump_label_inc(struct jump_label_key *key);
 extern void jump_label_dec(struct jump_label_key *key);

kernel/jump_label.c +14 -2

index 059202d5..ff2028f 100644
@@ -104,6 +104,18 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
 	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
+ * cheaper.
+ */
+void __weak arch_jump_label_transform_static(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	arch_jump_label_transform(entry, type);	
+}
+
 static void __jump_label_update(struct jump_label_key *key,
 				struct jump_entry *entry,
 				struct jump_entry *stop, int enable)

@@ -135,8 +147,8 @@ static __init int jump_label_init(void)
 		struct jump_label_key *iterk;
 
 		iterk = (struct jump_label_key *)(unsigned long)iter->key;
-		arch_jump_label_transform(iter, jump_label_enabled(iterk) ?
-					  JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
+		arch_jump_label_transform_static(iter, jump_label_enabled(iterk) ?
+						 JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
 		if (iterk == key)
 			continue;
 

kernel/jump_label.c void jump_label_apply_nops(struct module *mod) · line 225

Quoted 7 lines

kernel/jump_label.c +1 -1

@@ -208,7 +220,7 @@ void jump_label_apply_nops(struct module *mod)
 		return;
 
 	for (iter = iter_start; iter < iter_stop; iter++)
-		arch_jump_label_transform(iter, JUMP_LABEL_DISABLE);
+		arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
 }
 

So I got myself a little confused wrt the early jump_label_apply_nops() call and the MODULE_COMING notifiers.

It looks to me like jump_label_apply_nops() is called way early and is in fact called before _any_ of the module code has had a chance of running. However it simply NOPs out all jump_labels.

The jump_label_add_module() thing, which is ran on the MODULE_COMING callback will then set up stuff and do the proper patch-up.

Now the only bit of the module text that can be ran between those two calls appears to be the module argument parsing stuff, but since jump_labels are non-functional it can't rely on them, so why do we do the early patch up again?

Quoted 8 lines

So I got myself a little confused wrt the early jump_label_apply_nops() call and the MODULE_COMING notifiers.

It looks to me like jump_label_apply_nops() is called way early and is in fact called before _any_ of the module code has had a chance of running. However it simply NOPs out all jump_labels.

yes.

Quoted 3 lines

The jump_label_add_module() thing, which is ran on the MODULE_COMING callback will then set up stuff and do the proper patch-up.

yes, only for the enabled ones.

Quoted 6 lines

Now the only bit of the module text that can be ran between those two calls appears to be the module argument parsing stuff, but since jump_labels are non-functional it can't rely on them, so why do we do the early patch up again?

The 'early patch' is for putting in the 'ideal' no-ops into the module code. These 'ideal' no-ops are discovered at run-time, not boot-time.

The code is optimized (hopefully) for the most common case. The jump labels are by nature expected to be off,

and by patching them early
like this, at least for x86, we can avoid the stop machine calls. So its the combination of most are expected to be off and no sense to call extra stop machines that lead the code to its present state.

Quoted 9 lines · Jason Baron

Quoted 8 lines

So I got myself a little confused wrt the early jump_label_apply_nops() call and the MODULE_COMING notifiers.

It looks to me like jump_label_apply_nops() is called way early and is in fact called before _any_ of the module code has had a chance of running. However it simply NOPs out all jump_labels.

Quoted 7 lines

yes.

Quoted 3 lines

The jump_label_add_module() thing, which is ran on the MODULE_COMING callback will then set up stuff and do the proper patch-up.

yes, only for the enabled ones.

But we could make it nop out the disabled ones quite trivially.

Quoted 9 lines

Quoted 6 lines

Now the only bit of the module text that can be ran between those two calls appears to be the module argument parsing stuff, but since jump_labels are non-functional it can't rely on them, so why do we do the early patch up again?

The 'early patch' is for putting in the 'ideal' no-ops into the module code. These 'ideal' no-ops are discovered at run-time, not boot-time.

Sure, but since we can't use static_branch() and friends from very early module code (arg parsing) anyway, it doesn't matter what NOP they encounter, so we might as well do something like the below, right?

Quoted 2 lines

The code is optimized (hopefully) for the most common case. The jump labels are by nature expected to be off,

I actually need them to be either way.. no preference between on or off just a means of very _very_ infrequent runtime change in behaviour.

If we can push jump_label init to before sched_init() all I need is a static_branch() without the unlikely() in to avoid GCC out-of-lining the branch.

Quoted 4 lines

and by patching them early
like this, at least for x86, we can avoid the stop machine calls. So its the combination of most are expected to be off and no sense to call extra stop machines that lead the code to its present state.

But we could use arch_jump_label_transform_static because its before we actually execute any module text (sans the arg crap) which is stomp-machine free, removing that obstacle.

Or am I confused more?

---
 arch/mips/kernel/module.c  |    3 ---
 arch/sparc/kernel/module.c |    3 ---
 arch/x86/kernel/module.c   |    3 ---
 include/linux/jump_label.h |    6 ------
 kernel/jump_label.c        |   22 +++++++++++++---------
 5 files changed, 13 insertions(+), 24 deletions(-)

Index: linux-2.6/arch/mips/kernel/module.c
===================================================================

arch/mips/kernel/module.c +0 -3

@@ -368,9 +368,6 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const Elf_Shdr *s;
 	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
-	/* Make jump label nops. */
-	jump_label_apply_nops(me);
-
 	INIT_LIST_HEAD(&me->arch.dbe_list);
 	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
 		if (strcmp("__dbe_table", secstrings + s->sh_name) != 0)

Index: linux-2.6/arch/sparc/kernel/module.c
===================================================================

arch/sparc/kernel/module.c +0 -3

@@ -207,9 +207,6 @@ int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
-	/* make jump label nops */
-	jump_label_apply_nops(me);
-
 	/* Cheetah's I-cache is fully coherent.  */
 	if (tlb_type == spitfire) {
 		unsigned long va;

Index: linux-2.6/arch/x86/kernel/module.c
===================================================================

arch/x86/kernel/module.c +0 -3

@@ -194,9 +194,6 @@ int module_finalize(const Elf_Ehdr *hdr,
 		apply_paravirt(pseg, pseg + para->sh_size);
 	}
 
-	/* make jump label nops */
-	jump_label_apply_nops(me);
-
 	return 0;
 }
 

Index: linux-2.6/include/linux/jump_label.h
===================================================================

include/linux/jump_label.h +0 -6

@@ -52,7 +52,6 @@ extern int jump_label_text_reserved(void
 extern void jump_label_inc(struct jump_label_key *key);
 extern void jump_label_dec(struct jump_label_key *key);
 extern bool jump_label_enabled(struct jump_label_key *key);
-extern void jump_label_apply_nops(struct module *mod);
 
 #else  /* !HAVE_JUMP_LABEL */
 

@@ -97,11 +96,6 @@ static inline bool jump_label_enabled(st
 {
 	return !!atomic_read(&key->enabled);
 }
-
-static inline int jump_label_apply_nops(struct module *mod)
-{
-	return 0;
-}
 #endif	/* HAVE_JUMP_LABEL */
 
 #endif	/* _LINUX_JUMP_LABEL_H */

Index: linux-2.6/kernel/jump_label.c
===================================================================

kernel/jump_label.c +13 -9

@@ -116,9 +116,15 @@ void __weak arch_jump_label_transform_st
 	arch_jump_label_transform(entry, type);	
 }
 
+static inline enum jump_label_type jump_label_dyn_type(struct jump_label_key *key)
+{
+	return jump_label_enabled(key) ? JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE;
+}
+
 static void __jump_label_update(struct jump_label_key *key,
 				struct jump_entry *entry,
-				struct jump_entry *stop, int enable)
+				struct jump_entry *stop, int enable,
+				void (*transform)(struct jump_entry *, enum jump_label_type))
 {
 	for (; (entry < stop) &&
 	      (entry->key == (jump_label_t)(unsigned long)key);

@@ -129,7 +135,7 @@ static void __jump_label_update(struct j
 		 * init code, see jump_label_invalidate_module_init().
 		 */
 		if (entry->code && kernel_text_address(entry->code))
-			arch_jump_label_transform(entry, enable);
+			transform(entry, enable);
 	}
 }
 

@@ -147,8 +153,7 @@ void __init jump_label_init(void)
 		struct jump_label_key *iterk;
 
 		iterk = (struct jump_label_key *)(unsigned long)iter->key;
-		arch_jump_label_transform_static(iter, jump_label_enabled(iterk) ?
-						 JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
+		arch_jump_label_transform_static(iter, jump_label_dyn_type(iterk));
 		if (iterk == key)
 			continue;
 

@@ -193,7 +198,7 @@ static void __jump_label_mod_update(stru
 
 		__jump_label_update(key, mod->entries,
 				    m->jump_entries + m->num_jump_entries,
-				    enable);
+				    enable, arch_jump_label_transform);
 		mod = mod->next;
 	}
 }

@@ -256,9 +261,8 @@ static int jump_label_add_module(struct
 		jlm->next = key->next;
 		key->next = jlm;
 
-		if (jump_label_enabled(key))
-			__jump_label_update(key, iter, iter_stop,
-					    JUMP_LABEL_ENABLE);
+		__jump_label_update(key, iter, iter_stop, jump_label_dyn_type(key),
+				arch_jump_label_transform_static);
 	}
 
 	return 0;

@@ -392,7 +396,7 @@ static void jump_label_update(struct jum
 #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, enable, arch_jump_label_transform);
 }
 
 #endif

Quoted 3 lines

I actually need them to be either way.. no preference between on or off just a means of very _very_ infrequent runtime change in behaviour.

ok, this is a new use case, all the current users are biased with gcc out-of-lining the infrequent case.

Quoted 4 lines

If we can push jump_label init to before sched_init() all I need is a static_branch() without the unlikely() in to avoid GCC out-of-lining the branch.

hmmm....the current code (I believe) is biased b/c gcc sees the branch as always false, see: arch_static_branch() - its not b/c we have an unlikely there. Without open coding the label, like we had before everybody hated, I'll have to play around and see what will create an unbiased branch...perhaps, somebody has an idea?

Quoted 9 lines

Quoted 4 lines

and by patching them early
like this, at least for x86, we can avoid the stop machine calls. So its the combination of most are expected to be off and no sense to call extra stop machines that lead the code to its present state.

But we could use arch_jump_label_transform_static because its before we actually execute any module text (sans the arg crap) which is stomp-machine free, removing that obstacle.

Quoted 2 lines

Or am I confused more?

The MODULE_COMING callback happens *after* the call to flush_module_icache(mod), so I'm not sure that is safe...

+ __jump_label_transform(entry, type, text_poke_early);

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

Quoted 4 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

Quoted 3 lines · H. Peter Anvin

Quoted 2 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

Quoted 3 lines

Quoted 2 lines

GENERIC_NOP5_ATOMIC should make a better nop.

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

So the cleanest way of fixing that is to make the GENERIC_NOP* be defined to P6_NOP* on 64-bit then? Assuming I'm correct in guessing that the intent of GENERIC_NOP* is "a basically good-enough NOP that will work on any x86".

Quoted 4 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

Quoted 6 lines · H. Peter Anvin

Quoted 4 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

Something like this?

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 5 lines · H. Peter Anvin

Quoted 4 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

Quoted 4 lines

Quoted 1 line

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

Something like this?

No, we only care about 5byte nops here. Look at ideal_nop in alterative.c

Quoted 9 lines · Steven Rostedt

Quoted 8 lines · Jeremy Fitzhardinge

Quoted 6 lines · H. Peter Anvin

Quoted 4 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

GENERIC_NOP5_ATOMIC should make a better nop.

On 32 bits, yes. On 64 bits you should use P6_NOP5_ATOMIC.

Quoted 6 lines

Quoted 2 lines

Something like this?

No, we only care about 5byte nops here. Look at ideal_nop in alterative.c

He needs a compile-time alternative.

Something like this?

From: Jeremy Fitzhardinge <[email protected]>
Date: Thu, 13 Oct 2011 09:55:25 -0700
Subject: [PATCH] x86/nop: we can use P6 as generic NOP on 64-bit

Signed-off-by Jeremy Fitzhardinge <[email protected]>

arch/x86/include/asm/nops.h +0 -9

index 405b403..f9fead9 100644
@@ -19,15 +19,15 @@
    6: leal 0x00000000(%esi),%esi
    7: leal 0x00000000(,%esi,1),%esi
 */
-#define GENERIC_NOP1 0x90
-#define GENERIC_NOP2 0x89,0xf6
-#define GENERIC_NOP3 0x8d,0x76,0x00
-#define GENERIC_NOP4 0x8d,0x74,0x26,0x00
-#define GENERIC_NOP5 GENERIC_NOP1,GENERIC_NOP4
-#define GENERIC_NOP6 0x8d,0xb6,0x00,0x00,0x00,0x00
-#define GENERIC_NOP7 0x8d,0xb4,0x26,0x00,0x00,0x00,0x00
-#define GENERIC_NOP8 GENERIC_NOP1,GENERIC_NOP7
-#define GENERIC_NOP5_ATOMIC NOP_DS_PREFIX,GENERIC_NOP4

arch/x86/include/asm/nops.h +12 -0

 /* Opteron 64bit nops
    1: nop

@@ -87,6 +87,28 @@
 #define P6_NOP8	0x0f,0x1f,0x84,0x00,0,0,0,0
 #define P6_NOP5_ATOMIC P6_NOP5
 
+#ifdef CONFIG_X86_32
+#define GENERIC_NOP1		GAS_NOP1
+#define GENERIC_NOP2		GAS_NOP2
+#define GENERIC_NOP3		GAS_NOP3
+#define GENERIC_NOP4		GAS_NOP4
+#define GENERIC_NOP5		GAS_NOP5
+#define GENERIC_NOP6		GAS_NOP6
+#define GENERIC_NOP7		GAS_NOP7
+#define GENERIC_NOP8		GAS_NOP8
+#define GENERIC_NOP5_ATOMIC	GAS_NOP5_ATOMIC
+#else
+#define GENERIC_NOP1		P6_NOP1

From: Jeremy Fitzhardinge <[email protected]>

Initialize jump_labels much earlier, we can use them.

Quoted 3 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

Initialize jump_labels much earlier, we can use them.

We can use them, where? how? what?, that sentence just begs for more.

Quoted 8 lines

init/main.c +0 -0

index 2a9b88a..f4094ed 100644
@@ -637,6 +637,7 @@ asmlinkage void __init start_kernel(void)
 	acpi_early_init(); /* before LAPIC and SMP init */
 	sfi_init_late();
 

init/main.c asmlinkage void __init start_kernel(void) · line 643

Quoted 4 lines

init/main.c +1 -0

+	jump_label_init();
 	ftrace_init();
 
 	/* Do the rest non-__init'ed, we're now alive */

Can we push them much earlier still? If possible I'd like them to be before sched_init() so that I might use them there, if not possible, at the very least before enabling interrupts would be nice.

Quoted 3 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

Initialize jump_labels much earlier, we can use them.

We can use them, where? how? what?, that sentence just begs for more.

init/main.c asmlinkage void __init start_kernel(void) · line 643

Quoted 12 lines

init/main.c +1 -0

index 2a9b88a..f4094ed 100644
@@ -637,6 +637,7 @@ asmlinkage void __init start_kernel(void)
 	acpi_early_init(); /* before LAPIC and SMP init */
 	sfi_init_late();
 
+	jump_label_init();
 	ftrace_init();
 
 	/* Do the rest non-__init'ed, we're now alive */

Can we push them much earlier still? If possible I'd like them to be before sched_init() so that I might use them there, if not possible, at the very least before enabling interrupts would be nice.

Quoted 3 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

Initialize jump_labels much earlier, we can use them.

We can use them, where? how? what?, that sentence just begs for more.

From 62720522d512ffce8f4be9140f73fefbdfd2872e Mon Sep 17 00:00:00 2001

How about this? Proper comment and much earlier init.

Quoted 1 line · Jeremy Fitzhardinge

How about this? Proper comment and much earlier init.

Looks good to me, thanks!

Quoted 3 lines · Peter Zijlstra

Quoted 1 line · Jeremy Fitzhardinge

How about this? Proper comment and much earlier init.

Looks good to me, thanks!

Should I take that as an ACK on the series?

Quoted 5 lines · Jeremy Fitzhardinge

Quoted 3 lines · Peter Zijlstra

Quoted 1 line · Jeremy Fitzhardinge

How about this? Proper comment and much earlier init.

Looks good to me, thanks!

Should I take that as an ACK on the series?

Yes, I don't see any problems with it,

Acked-by Peter Zijlstra <[email protected]>