[PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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(-)

-- 
1.7.6.2

[PATCH RFC 1/8] jump_label: use proper atomic_t initializer

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

ATOMIC_INIT() is the proper thing to use.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/linux/jump_label.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 66f23dc..1213e9d 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -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)
-- 
1.7.6.2

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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 <jeremy.fitzhardinge@citrix.com>
---
 include/linux/jump_label.h |    3 ++-
 kernel/jump_label.c        |   17 +++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 1213e9d..c8fb1b3 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -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);
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index a8ce450..54e8e2d 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -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)
-- 
1.7.6.2

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 arch/x86/kernel/jump_label.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index 3fee346..0887b59 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -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
-- 
1.7.6.2

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: David S. Miller <davem@davemloft.net>
---
 arch/sparc/kernel/jump_label.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/sparc/kernel/jump_label.c b/arch/sparc/kernel/jump_label.c
index ea2dafc..d3dad25 100644
--- a/arch/sparc/kernel/jump_label.c
+++ b/arch/sparc/kernel/jump_label.c
@@ -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
-- 
1.7.6.2

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

[David Miller]

From: Jeremy Fitzhardinge <jeremy@goop.org>
Date: Thu, 29 Sep 2011 16:26:34 -0700

> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> This allows jump-label entries to be modified early, in a pre-SMP
> environment.
> 
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Acked-by: David S. Miller <davem@davemloft.net>

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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 <jeremy.fitzhardinge@citrix.com>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/jump_label.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kernel/jump_label.c b/arch/mips/kernel/jump_label.c
index 6001610..ddc9a65 100644
--- a/arch/mips/kernel/jump_label.c
+++ b/arch/mips/kernel/jump_label.c
@@ -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 */
-- 
1.7.6.2

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Michael Ellerman <michael@ellerman.id.au>
---
 arch/powerpc/kernel/jump_label.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/jump_label.c b/arch/powerpc/kernel/jump_label.c
index 368d158..20c82fb 100644
--- a/arch/powerpc/kernel/jump_label.c
+++ b/arch/powerpc/kernel/jump_label.c
@@ -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);
+}
-- 
1.7.6.2

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

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Jan Glauber <jang@linux.vnet.ibm.com>
---
 arch/s390/kernel/jump_label.c |   49 +++++++++++++++++++++++++----------------
 1 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c
index 44cc06b..6001228 100644
--- a/arch/s390/kernel/jump_label.c
+++ b/arch/s390/kernel/jump_label.c
@@ -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
-- 
1.7.6.2

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

[Jan Glauber]

On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> This allows jump-label entries to be modified early, in a pre-SMP
> environment.
> 
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Cc: Jan Glauber <jang@linux.vnet.ibm.com>

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

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

[Jeremy Fitzhardinge]

On 09/30/2011 07:48 AM, Jan Glauber wrote:
> On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> This allows jump-label entries to be modified early, in a pre-SMP
>> environment.
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> Cc: Jan Glauber <jang@linux.vnet.ibm.com>
> 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).

>   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 <jeremy.fitzhardinge@citrix.com>
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 <jeremy.fitzhardinge@citrix.com>
Cc: Jan Glauber <jang@linux.vnet.ibm.com>

diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c
index 44cc06b..4fbe63b 100644
--- a/arch/s390/kernel/jump_label.c
+++ b/arch/s390/kernel/jump_label.c
@@ -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

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

[Jeremy Fitzhardinge]

On 09/30/2011 07:48 AM, Jan Glauber wrote:
> On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> This allows jump-label entries to be modified early, in a pre-SMP
>> environment.
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> Cc: Jan Glauber <jang@linux.vnet.ibm.com>
> 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).

>   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 <jeremy.fitzhardinge@citrix.com>
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 <jeremy.fitzhardinge@citrix.com>
Cc: Jan Glauber <jang@linux.vnet.ibm.com>

diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c
index 44cc06b..4fbe63b 100644
--- a/arch/s390/kernel/jump_label.c
+++ b/arch/s390/kernel/jump_label.c
@@ -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

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

[Jan Glauber]

On Fri, 2011-09-30 at 09:03 -0700, Jeremy Fitzhardinge wrote:
> On 09/30/2011 07:48 AM, Jan Glauber wrote:
> > On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
> >> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >>
> >> This allows jump-label entries to be modified early, in a pre-SMP
> >> environment.
> >>
> >> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >> Cc: Jan Glauber <jang@linux.vnet.ibm.com>
> > 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.

> 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.

> >   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?

Yes, that compiles!

--Jan

> From 9572689d1e5e6f54a1936a1dca09a6920d1bce27 Mon Sep 17 00:00:00 2001
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 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 <jeremy.fitzhardinge@citrix.com>
> Cc: Jan Glauber <jang@linux.vnet.ibm.com>
> 
> diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c
> index 44cc06b..4fbe63b 100644
> --- a/arch/s390/kernel/jump_label.c
> +++ b/arch/s390/kernel/jump_label.c
> @@ -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
> 

[PATCH RFC 8/8] jump_label: drop default arch_jump_label_transform_early

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 kernel/jump_label.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 54e8e2d..3010bcf 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -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;
-- 
1.7.6.2

Re: [PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Steven Rostedt]

On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 

> 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

Re: [PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Jeremy Fitzhardinge]

On 09/29/2011 05:52 PM, Steven Rostedt wrote:
> On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> 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.

> 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.

> 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

Re: [PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Jeremy Fitzhardinge]

On 09/29/2011 05:52 PM, Steven Rostedt wrote:
> On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> 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.

> 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.

> 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

Re: [PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Steven Rostedt]

On Thu, 2011-09-29 at 21:40 -0700, Jeremy Fitzhardinge wrote:
> On 09/29/2011 05:52 PM, Steven Rostedt wrote:
> > On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
> >> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >>
> >> 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.

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.

> 
> > 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.
> 
> > 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

Re: [PATCH RFC 0/8] jump-label: allow early jump_label_enable()

[Jeremy Fitzhardinge]

On 09/30/2011 08:28 AM, Steven Rostedt wrote:
> On Thu, 2011-09-29 at 21:40 -0700, Jeremy Fitzhardinge wrote:
>> On 09/29/2011 05:52 PM, Steven Rostedt wrote:
>>> On Thu, 2011-09-29 at 16:26 -0700, Jeremy Fitzhardinge wrote:
>>>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>>>
>>>> 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.
> 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:

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index ba5070c..b6ad9b3 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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