Skip to contentSkip to conversation

jump-label: allow early jump_label_enable()

RFC PATCH Superseded Jeremy Fitzhardinge 4 participants 17 messages Sep 29, 2011 – Oct 1, 2011

Subsystems (6)
x86 PowerPC MIPS s390 +2 more subsystems

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Jeremy Fitzhardinge

author
#0 lore raw

cover jump-label: allow early jump_label_enable()

From: Jeremy Fitzhardinge <[email protected]>

Hi all,

While trying to use the jump-label stuff for my PV ticketlock changes, I had some problems using jump labels early in the kernel's lifetime (pre-SMP).

The basic problem is that even if I enable a jump_label_key, the jump_label_init() initializer ends up nopping out all the code sites.

This series enables early use of jump labels by making jump_label_init() respect already-enabled keys.

To do this, I've dropped arch_jump_label_poke_text_early() and replaced it with arch_jump_label_transform_early(), which is the same as the non-_early variant except that it expects to be operating in a pre-SMP environment.

I've tested this on x86, but all the other architecture changes are completely untested (not even breathed on by a compiler).

One big question which arises is whether the _early() function is necessary at all. All the stop_machine/mutex/etc stuff that arch_jump_label_transform() ends up doing is redundant pre-SMP, but it shouldn't hurt. Maybe we can just drop the _early function? It works on x86, at least, because jump_label_enable() works, which uses the full form. And dropping it would reduce this to a very much smaller series.

Thanks,
J

Jeremy Fitzhardinge (8):
jump_label: use proper atomic_t initializer
jump_label: if a key has already been initialized, don't nop it out x86/jump_label: add arch_jump_label_transform_early() sparc/jump_label: add arch_jump_label_transform_early() mips/jump_label: add arch_jump_label_transform_early() powerpc/jump_label: add arch_jump_label_transform_early() s390/jump-label: add arch_jump_label_transform_early()
jump_label: drop default arch_jump_label_transform_early

 arch/mips/kernel/jump_label.c    |   21 +++++++++++++---
 arch/powerpc/kernel/jump_label.c |    6 ++++
 arch/s390/kernel/jump_label.c    |   49 +++++++++++++++++++++++--------------
 arch/sparc/kernel/jump_label.c   |   24 +++++++++++-------
 arch/x86/kernel/jump_label.c     |   20 +++++++++++----
 include/linux/jump_label.h       |    7 +++--
 kernel/jump_label.c              |   20 ++++++---------
 7 files changed, 94 insertions(+), 53 deletions(-)
Signature · 1 line
1.7.6.2
author
#1 lore raw

RFC PATCH 1/8 jump_label: use proper atomic_t initializer

From: Jeremy Fitzhardinge <[email protected]>

ATOMIC_INIT() is the proper thing to use.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
---
 include/linux/jump_label.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
include/linux/jump_label.h +2 -2
index 66f23dc..1213e9d 100644
@@ -28,9 +28,9 @@ struct module;
 #ifdef HAVE_JUMP_LABEL
 
 #ifdef CONFIG_MODULES
-#define JUMP_LABEL_INIT {{ 0 }, NULL, NULL}
+#define JUMP_LABEL_INIT {ATOMIC_INIT(0), NULL, NULL}
 #else
-#define JUMP_LABEL_INIT {{ 0 }, NULL}
+#define JUMP_LABEL_INIT {ATOMIC_INIT(0), NULL}
 #endif
 
 static __always_inline bool static_branch(struct jump_label_key *key)
Signature · 1 line
1.7.6.2
author
#2 lore raw

RFC PATCH 2/8 jump_label: if a key has already been initialized, don't nop it out

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

If a key has been enabled before jump_label_init() is called, don't nop it out.

This replaces arch_jump_label_text_poke_early() (which can only nop out a site) with arch_jump_label_transform_early(), which is functionally equivalent to arch_jump_label_transform().

Signed-off-by Jeremy Fitzhardinge <[email protected]>
---
 include/linux/jump_label.h |    3 ++-
 kernel/jump_label.c        |   17 +++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)
include/linux/jump_label.h +2 -1
index 1213e9d..c8fb1b3 100644
@@ -45,7 +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_text_poke_early(jump_label_t addr);
+extern void arch_jump_label_transform_early(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 +11 -6
index a8ce450..54e8e2d 100644
@@ -124,8 +124,10 @@ static void __jump_label_update(struct jump_label_key *key,
 /*
  * Not all archs need this.
  */
-void __weak arch_jump_label_text_poke_early(jump_label_t addr)
+void __weak arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
 {
+	arch_jump_label_transform(entry, type);	
 }
 
 static __init int jump_label_init(void)

@@ -139,12 +141,15 @@ static __init int jump_label_init(void)
 	jump_label_sort_entries(iter_start, iter_stop);
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
-		arch_jump_label_text_poke_early(iter->code);
-		if (iter->key == (jump_label_t)(unsigned long)key)
+		struct jump_label_key *iterk;
+
+		iterk = (struct jump_label_key *)(unsigned long)iter->key;
+		arch_jump_label_transform_early(iter, jump_label_enabled(iterk) ?
+						JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
+		if (iterk == key)
 			continue;
 
-		key = (struct jump_label_key *)(unsigned long)iter->key;
-		atomic_set(&key->enabled, 0);
+		key = iterk;
 		key->entries = iter;
 #ifdef CONFIG_MODULES
 		key->next = NULL;

@@ -212,7 +217,7 @@ void jump_label_apply_nops(struct module *mod)
 		return;
 
 	for (iter = iter_start; iter < iter_stop; iter++)
-		arch_jump_label_text_poke_early(iter->code);
+		arch_jump_label_transform(iter, JUMP_LABEL_DISABLE);
 }
 
 static int jump_label_add_module(struct module *mod)
Signature · 1 line
1.7.6.2
author
#3 lore raw

RFC PATCH 3/8 x86/jump_label: add arch_jump_label_transform_early()

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jason Baron <[email protected]>
---
 arch/x86/kernel/jump_label.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)
arch/x86/kernel/jump_label.c +14 -6
index 3fee346..0887b59 100644
@@ -24,8 +24,9 @@ union jump_code_union {
 	} __attribute__((packed));
 };
 
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
+static void __jump_label_transform(struct jump_entry *entry,
+				   enum jump_label_type type,
+				   void *(*poker)(void *, const void *, size_t))
 {
 	union jump_code_union code;
 

@@ -35,17 +36,24 @@ void arch_jump_label_transform(struct jump_entry *entry,
 				(entry->code + JUMP_LABEL_NOP_SIZE);
 	} else
 		memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
+
+	(*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
+}
+
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
 	get_online_cpus();
 	mutex_lock(&text_mutex);
-	text_poke_smp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
+	__jump_label_transform(entry, type, text_poke_smp);
 	mutex_unlock(&text_mutex);
 	put_online_cpus();
 }
 
-void arch_jump_label_text_poke_early(jump_label_t addr)
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
 {
-	text_poke_early((void *)addr, ideal_nops[NOP_ATOMIC5],
-			JUMP_LABEL_NOP_SIZE);
+	__jump_label_transform(entry, type, text_poke_early);
 }
 
 #endif
Signature · 1 line
1.7.6.2
author
#4 lore raw

RFC PATCH 4/8 sparc/jump_label: add arch_jump_label_transform_early()

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc David S. Miller <[email protected]>
---
 arch/sparc/kernel/jump_label.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)
arch/sparc/kernel/jump_label.c +15 -9
index ea2dafc..d3dad25 100644
@@ -8,8 +8,8 @@
 
 #ifdef HAVE_JUMP_LABEL
 
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
+static void __jump_label_transform(struct jump_entry *entry,
+				   enum jump_label_type type)
 {
 	u32 val;
 	u32 *insn = (u32 *) (unsigned long) entry->code;

@@ -28,20 +28,26 @@ void arch_jump_label_transform(struct jump_entry *entry,
 		val = 0x01000000;
 	}
 
-	get_online_cpus();
-	mutex_lock(&text_mutex);
 	*insn = val;
 	flushi(insn);
+}
+
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
+	get_online_cpus();
+	mutex_lock(&text_mutex);
+
+	__jump_label_transform(entry, type);
+
 	mutex_unlock(&text_mutex);
 	put_online_cpus();
 }
 
-void arch_jump_label_text_poke_early(jump_label_t addr)
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
 {
-	u32 *insn_p = (u32 *) (unsigned long) addr;
-
-	*insn_p = 0x01000000;
-	flushi(insn_p);
+	__jump_label_transform(entry, type);
 }
 
 #endif
Signature · 1 line
1.7.6.2
author
#6 lore raw

RFC PATCH 5/8 mips/jump_label: add arch_jump_label_transform_early()

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

XXX TODO: make sure idling CPUs flush their icaches before continuing in case this modifies something that's adjacent to their init-idle loops.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc David Daney <[email protected]>
---
 arch/mips/kernel/jump_label.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)
arch/mips/kernel/jump_label.c +17 -4
index 6001610..ddc9a65 100644
@@ -20,8 +20,8 @@
 
 #define J_RANGE_MASK ((1ul << 28) - 1)
 
-void arch_jump_label_transform(struct jump_entry *e,
-			       enum jump_label_type type)
+static void __jump_label_transform(struct jump_entry *e,
+				   enum jump_label_type type)
 {
 	union mips_instruction insn;
 	union mips_instruction *insn_p =

@@ -40,15 +40,28 @@ void arch_jump_label_transform(struct jump_entry *e,
 		insn.word = 0; /* nop */
 	}
 
-	get_online_cpus();
-	mutex_lock(&text_mutex);
 	*insn_p = insn;
 
 	flush_icache_range((unsigned long)insn_p,
 			   (unsigned long)insn_p + sizeof(*insn_p));
+}
+
+void arch_jump_label_transform(struct jump_entry *e,
+			       enum jump_label_type type)
+{
+	get_online_cpus();
+	mutex_lock(&text_mutex);
+
+	__jump_label_tranform(e, type);
 
 	mutex_unlock(&text_mutex);
 	put_online_cpus();
 }
 
+void __init arch_jump_label_transform_early(struct jump_entry *e,
+					    enum jump_label_type type)
+{
+	__jump_label_tranform(e, type);
+}
+
 #endif /* HAVE_JUMP_LABEL */
Signature · 1 line
1.7.6.2
author
#7 lore raw

RFC PATCH 6/8 powerpc/jump_label: add arch_jump_label_transform_early()

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Michael Ellerman <[email protected]>
---
 arch/powerpc/kernel/jump_label.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
arch/powerpc/kernel/jump_label.c +6 -0
index 368d158..20c82fb 100644
@@ -21,3 +21,9 @@ void arch_jump_label_transform(struct jump_entry *entry,
 	else
 		patch_instruction(addr, PPC_INST_NOP);
 }
+
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	arch_jump_label_transform(entry, type);
+}
Signature · 1 line
1.7.6.2
author
#8 lore raw

RFC PATCH 7/8 s390/jump-label: add arch_jump_label_transform_early()

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jan Glauber <[email protected]>
---
 arch/s390/kernel/jump_label.c |   49 +++++++++++++++++++++++++----------------
 1 files changed, 30 insertions(+), 19 deletions(-)
arch/s390/kernel/jump_label.c +30 -19
index 44cc06b..6001228 100644
@@ -18,23 +18,12 @@ struct insn {
 } __packed;
 
 struct insn_args {
-	unsigned long *target;
-	struct insn *insn;
-	ssize_t size;
+	struct jump_entry *entry;
+	enum jump_label_type type;
 };
 
-static int __arch_jump_label_transform(void *data)
-{
-	struct insn_args *args = data;
-	int rc;
-
-	rc = probe_kernel_write(args->target, args->insn, args->size);
-	WARN_ON_ONCE(rc < 0);
-	return 0;
-}
-
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
+static void __jump_label_transform(struct jump_entry *entry,
+				   enum jump_label_type type)
 {
 	struct insn_args args;
 	struct insn insn;

@@ -49,11 +38,33 @@ void arch_jump_label_transform(struct jump_entry *entry,
 		insn.offset = 0;
 	}
 
-	args.target = (void *) entry->code;
-	args.insn = &insn;
-	args.size = JUMP_LABEL_NOP_SIZE;
+	rc = probe_kernel_write(entry->code, &insn, JUMP_LABEL_NOP_SIZE);
+	WARN_ON_ONCE(rc < 0);
+}
+
+static int __sm_arch_jump_label_transform(void *data)
+{
+	struct insn_args *args = data;
+
+	__jump_label_transform(args->entry, args->type);
+	return 0;
+}
 
-	stop_machine(__arch_jump_label_transform, &args, NULL);
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
+	struct insn_args args;
+
+	args.entry = entry;
+	args.type = type;
+
+	stop_machine(__sm_arch_jump_label_transform, &args, NULL);
+}
+
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	__jump_label_transform(entry, type);
 }
 
 #endif
Signature · 1 line
1.7.6.2
#9 lore raw

Re: [PATCH RFC 7/8] s390/jump-label: add arch_jump_label_transform_early()

Quoted 7 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

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

Hi Jeremy,

Your patch looks fine, if you can fix the minor compiler warnings below. Excluding stop_machine() on pre-SMP also looks safer too me.

--Jan

CC arch/s390/kernel/jump_label.o
arch/s390/kernel/jump_label.c: In function ‘__jump_label_transform’:
arch/s390/kernel/jump_label.c:41:2: error: ‘rc’ undeclared (first use in this function)
arch/s390/kernel/jump_label.c:41:2: note: each undeclared identifier is reported only once for each function it appears in arch/s390/kernel/jump_label.c:41:2: warning: passing argument 1 of ‘probe_kernel_write’ makes pointer from integer without a cast [enabled by default] include/linux/uaccess.h:108:21: note: expected ‘void *’ but argument is of type ‘jump_label_t’ arch/s390/kernel/jump_label.c:28:19: warning: unused variable ‘args’ [-Wunused-variable]
make[2]: *** [arch/s390/kernel/jump_label.o] Error 1

author
#10 lore raw

Re: [PATCH RFC 7/8] s390/jump-label: add arch_jump_label_transform_early()

Quoted 6 lines · Jan Glauber
Quoted 5 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jan Glauber <[email protected]>

Hi Jeremy,

Your patch looks fine, if you can fix the minor compiler warnings below. Excluding stop_machine() on pre-SMP also looks safer too me.

Do you think there would be an actual problem, or are you just being cautious?

It seems to me - in general - stop_machine could just be defined to be a no-op (ie, just directly calls the callback) until enough SMP is initialized for it to make sense, rather than having to make every user work around it (if there's a chance they might call it early).

Quoted 10 lines

CC arch/s390/kernel/jump_label.o
arch/s390/kernel/jump_label.c: In function ‘__jump_label_transform’:
arch/s390/kernel/jump_label.c:41:2: error: ‘rc’ undeclared (first use in this function)
arch/s390/kernel/jump_label.c:41:2: note: each undeclared identifier is reported only once for each function it appears in arch/s390/kernel/jump_label.c:41:2: warning: passing argument 1 of ‘probe_kernel_write’ makes pointer from integer without a cast [enabled by default] include/linux/uaccess.h:108:21: note: expected ‘void *’ but argument is of type ‘jump_label_t’ arch/s390/kernel/jump_label.c:28:19: warning: unused variable ‘args’ [-Wunused-variable]
make[2]: *** [arch/s390/kernel/jump_label.o] Error 1

Like so?

From 9572689d1e5e6f54a1936a1dca09a6920d1bce27 Mon Sep 17 00:00:00 2001
From: Jeremy Fitzhardinge <[email protected]>
Date: Thu, 29 Sep 2011 10:58:46 -0700
Subject: [PATCH] s390/jump-label: add arch_jump_label_transform_early()

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jan Glauber <[email protected]>
arch/s390/kernel/jump_label.c +31 -20
index 44cc06b..4fbe63b 100644
@@ -18,26 +18,15 @@ struct insn {
 } __packed;
 
 struct insn_args {
-	unsigned long *target;
-	struct insn *insn;
-	ssize_t size;
+	struct jump_entry *entry;
+	enum jump_label_type type;
 };
 
-static int __arch_jump_label_transform(void *data)
+static void __jump_label_transform(struct jump_entry *entry,
+				   enum jump_label_type type)
 {
-	struct insn_args *args = data;
-	int rc;
-
-	rc = probe_kernel_write(args->target, args->insn, args->size);
-	WARN_ON_ONCE(rc < 0);
-	return 0;
-}
-
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
-{
-	struct insn_args args;
 	struct insn insn;
+	int rc;
 
 	if (type == JUMP_LABEL_ENABLE) {
 		/* brcl 15,offset */

@@ -49,11 +38,33 @@ void arch_jump_label_transform(struct jump_entry *entry,
 		insn.offset = 0;
 	}
 
-	args.target = (void *) entry->code;
-	args.insn = &insn;
-	args.size = JUMP_LABEL_NOP_SIZE;
+	rc = probe_kernel_write((void *)entry->code, &insn, JUMP_LABEL_NOP_SIZE);
+	WARN_ON_ONCE(rc < 0);
+}
 
-	stop_machine(__arch_jump_label_transform, &args, NULL);
+static int __sm_arch_jump_label_transform(void *data)
+{
+	struct insn_args *args = data;
+
+	__jump_label_transform(args->entry, args->type);
+	return 0;
+}
+
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
+	struct insn_args args;
+
+	args.entry = entry;
+	args.type = type;
+
+	stop_machine(__sm_arch_jump_label_transform, &args, NULL);
+}
+
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	__jump_label_transform(entry, type);
 }
 
 #endif

J

#11 lore raw

Re: [PATCH RFC 7/8] s390/jump-label: add arch_jump_label_transform_early()

Quoted 9 lines · Jeremy Fitzhardinge
Quoted 8 lines · Jan Glauber
Quoted 7 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jan Glauber <[email protected]>
Quoted 7 lines
Quoted 4 lines

Hi Jeremy,

Your patch looks fine, if you can fix the minor compiler warnings below. Excluding stop_machine() on pre-SMP also looks safer too me.

Do you think there would be an actual problem, or are you just being cautious?

Just cautious since stop_machine() is quite a big hammer. Who knows how many jump labels we might get? So without an early exit in stop_machine() for pre-SMP we might waste performance there.

Quoted 4 lines

It seems to me - in general - stop_machine could just be defined to be a no-op (ie, just directly calls the callback) until enough SMP is initialized for it to make sense, rather than having to make every user work around it (if there's a chance they might call it early).

Agreed.

Quoted 11 lines
Quoted 10 lines

CC arch/s390/kernel/jump_label.o
arch/s390/kernel/jump_label.c: In function ‘__jump_label_transform’:
arch/s390/kernel/jump_label.c:41:2: error: ‘rc’ undeclared (first use in this function)
arch/s390/kernel/jump_label.c:41:2: note: each undeclared identifier is reported only once for each function it appears in arch/s390/kernel/jump_label.c:41:2: warning: passing argument 1 of ‘probe_kernel_write’ makes pointer from integer without a cast [enabled by default] include/linux/uaccess.h:108:21: note: expected ‘void *’ but argument is of type ‘jump_label_t’ arch/s390/kernel/jump_label.c:28:19: warning: unused variable ‘args’ [-Wunused-variable]
make[2]: *** [arch/s390/kernel/jump_label.o] Error 1

Quoted 1 line

Like so?

Yes, that compiles!

--Jan

Quoted 88 lines

From 9572689d1e5e6f54a1936a1dca09a6920d1bce27 Mon Sep 17 00:00:00 2001
From: Jeremy Fitzhardinge <[email protected]>
Date: Thu, 29 Sep 2011 10:58:46 -0700
Subject: [PATCH] s390/jump-label: add arch_jump_label_transform_early()

This allows jump-label entries to be modified early, in a pre-SMP environment.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Jan Glauber <[email protected]>
arch/s390/kernel/jump_label.c +31 -20
index 44cc06b..4fbe63b 100644
@@ -18,26 +18,15 @@ struct insn {
 } __packed;
 
 struct insn_args {
-	unsigned long *target;
-	struct insn *insn;
-	ssize_t size;
+	struct jump_entry *entry;
+	enum jump_label_type type;
 };
 
-static int __arch_jump_label_transform(void *data)
+static void __jump_label_transform(struct jump_entry *entry,
+				   enum jump_label_type type)
 {
-	struct insn_args *args = data;
-	int rc;
-
-	rc = probe_kernel_write(args->target, args->insn, args->size);
-	WARN_ON_ONCE(rc < 0);
-	return 0;
-}
-
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
-{
-	struct insn_args args;
 	struct insn insn;
+	int rc;
 
 	if (type == JUMP_LABEL_ENABLE) {
 		/* brcl 15,offset */

@@ -49,11 +38,33 @@ void arch_jump_label_transform(struct jump_entry *entry,
 		insn.offset = 0;
 	}
 
-	args.target = (void *) entry->code;
-	args.insn = &insn;
-	args.size = JUMP_LABEL_NOP_SIZE;
+	rc = probe_kernel_write((void *)entry->code, &insn, JUMP_LABEL_NOP_SIZE);
+	WARN_ON_ONCE(rc < 0);
+}
 
-	stop_machine(__arch_jump_label_transform, &args, NULL);
+static int __sm_arch_jump_label_transform(void *data)
+{
+	struct insn_args *args = data;
+
+	__jump_label_transform(args->entry, args->type);
+	return 0;
+}
+
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
+	struct insn_args args;
+
+	args.entry = entry;
+	args.type = type;
+
+	stop_machine(__sm_arch_jump_label_transform, &args, NULL);
+}
+
+void __init arch_jump_label_transform_early(struct jump_entry *entry,
+					    enum jump_label_type type)
+{
+	__jump_label_transform(entry, type);
 }
 
 #endif

J

author
#12 lore raw

RFC PATCH 8/8 jump_label: drop default arch_jump_label_transform_early

In reply to Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

Signed-off-by Jeremy Fitzhardinge <[email protected]>
---
 kernel/jump_label.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)
kernel/jump_label.c +0 -9
index 54e8e2d..3010bcf 100644
@@ -121,15 +121,6 @@ static void __jump_label_update(struct jump_label_key *key,
 	}
 }
 
-/*
- * Not all archs need this.
- */
-void __weak arch_jump_label_transform_early(struct jump_entry *entry,
-					    enum jump_label_type type)
-{
-	arch_jump_label_transform(entry, type);	
-}
-
 static __init int jump_label_init(void)
 {
 	struct jump_entry *iter_start = __start___jump_table;
Signature · 1 line
1.7.6.2
reviewer
#13 lore raw
In reply to Jeremy Fitzhardinge
Quoted 9 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

One big question which arises is whether the _early() function is necessary at all. All the stop_machine/mutex/etc stuff that arch_jump_label_transform() ends up doing is redundant pre-SMP, but it shouldn't hurt. Maybe we can just drop the _early function? It works on x86, at least, because jump_label_enable() works, which uses the full form. And dropping it would reduce this to a very much smaller series.

It does slow down the boot process, which is not a good thing when everyone is pushing for the fastest restarts.

What we should probably do is have a global read_mostly variable called, smp_activated or something, then things that can be called before and after can read this variable to determine if it can skip certain protections.

While we're at it, perhaps we could add a memory_initialized for things like tracers that want to trace early but need to wait till it can allocate buffers. If we had this flag, it could instead do an early memory init to create the buffers.

-- Steve

author
#14 lore raw
Quoted 11 lines · Steven Rostedt
Quoted 8 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

One big question which arises is whether the _early() function is necessary at all. All the stop_machine/mutex/etc stuff that arch_jump_label_transform() ends up doing is redundant pre-SMP, but it shouldn't hurt. Maybe we can just drop the _early function? It works on x86, at least, because jump_label_enable() works, which uses the full form. And dropping it would reduce this to a very much smaller series.

It does slow down the boot process, which is not a good thing when everyone is pushing for the fastest restarts.

Would it really though? stop_machine() doesn't do very much when there are no other cpus.

Not that I measured or anything, but there was no obvious big lag at boot.

Quoted 4 lines

What we should probably do is have a global read_mostly variable called, smp_activated or something, then things that can be called before and after can read this variable to determine if it can skip certain protections.

Could do that if it turns out to be a problem.

Quoted 4 lines

While we're at it, perhaps we could add a memory_initialized for things like tracers that want to trace early but need to wait till it can allocate buffers. If we had this flag, it could instead do an early memory init to create the buffers.

That seems orthogonal to the jump_label changes.

J

reviewer
#15 lore raw
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 9 lines · Steven Rostedt
Quoted 8 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

One big question which arises is whether the _early() function is necessary at all. All the stop_machine/mutex/etc stuff that arch_jump_label_transform() ends up doing is redundant pre-SMP, but it shouldn't hurt. Maybe we can just drop the _early function? It works on x86, at least, because jump_label_enable() works, which uses the full form. And dropping it would reduce this to a very much smaller series.

Quoted 7 lines
Quoted 2 lines

It does slow down the boot process, which is not a good thing when everyone is pushing for the fastest restarts.

Would it really though? stop_machine() doesn't do very much when there are no other cpus.

Not that I measured or anything, but there was no obvious big lag at boot.

Just bringing up the point, but without measurements, its all hand waving. It may not be a big deal, and simpler code is always better if it doesn't harm anything else.

Quoted 6 lines
Quoted 5 lines

What we should probably do is have a global read_mostly variable called, smp_activated or something, then things that can be called before and after can read this variable to determine if it can skip certain protections.

Quoted 8 lines

Could do that if it turns out to be a problem.

Quoted 4 lines

While we're at it, perhaps we could add a memory_initialized for things like tracers that want to trace early but need to wait till it can allocate buffers. If we had this flag, it could instead do an early memory init to create the buffers.

That seems orthogonal to the jump_label changes.

It is. But it's something that bugs me and this just reminded me of it ;)

-- Steve

author
#16 lore raw
Quoted 13 lines · Steven Rostedt
Quoted 12 lines · Jeremy Fitzhardinge
Quoted 11 lines · Steven Rostedt
Quoted 8 lines · Jeremy Fitzhardinge

From: Jeremy Fitzhardinge <[email protected]>

One big question which arises is whether the _early() function is necessary at all. All the stop_machine/mutex/etc stuff that arch_jump_label_transform() ends up doing is redundant pre-SMP, but it shouldn't hurt. Maybe we can just drop the _early function? It works on x86, at least, because jump_label_enable() works, which uses the full form. And dropping it would reduce this to a very much smaller series.

It does slow down the boot process, which is not a good thing when everyone is pushing for the fastest restarts.

Quoted 7 lines
Quoted 4 lines

Would it really though? stop_machine() doesn't do very much when there are no other cpus.

Not that I measured or anything, but there was no obvious big lag at boot.

Just bringing up the point, but without measurements, its all hand waving. It may not be a big deal, and simpler code is always better if it doesn't harm anything else.

I think the simplest thing is to make stop_machine() well-defined in a pre-smp environment, where it just directly calls the callback:

kernel/stop_machine.c +5 -0
index ba5070c..b6ad9b3 100644
@@ -485,6 +485,11 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
+	if (smdata.num_threads == 1) {
+		(*fn)(data);
+		return 0;
+	}
+
 	/* Set the initial state and stop all online cpus. */
 	set_state(&smdata, STOPMACHINE_PREPARE);
 	return stop_cpus(cpu_online_mask, stop_machine_cpu_stop, &smdata);

so that its guaranteed safe to use at any point.

J