Skip to contentSkip to conversation

init/main: Init jump_labels before they are used to build zonelists

PATCH In review Stafford Horne 6 participants 10 messages Jan 17, 2017 – Jan 23, 2017 Memory management

lore
Share & tools
lore

Stafford Horne

author
#0 lore raw

PATCH 1/1 init/main: Init jump_labels before they are used to build zonelists

A recent change in -next introduced static_keys in init zonelists, details are:

Author: Vlastimil Babka <[email protected]>
Date: Thu Jan 12 12:19:03 2017 +1100
commit f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")

This causes the following warning in openrisc as reported by Guenter, and repoduced by me.

 WARNING: CPU: 0 PID: 0 at ./include/linux/jump_label.h:198 0xc02d758c
 static_key_slow_inc used before call to jump_label_init

This fixes this by initialized jump_labels even earlier, I am suprized the issue is not showing up in other platforms yet.

Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -2
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
 

@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
 
-	jump_label_init();
-
 	/*
 	 * These use large bootmem allocations and must precede
 	 * kmem_cache_init()
Signature · 1 line
2.9.3
#1 lore raw

[+CC PeterZ]

Quoted 8 lines · Stafford Horne

A recent change in -next introduced static_keys in init zonelists, details are:

Author: Vlastimil Babka <[email protected]>
Date: Thu Jan 12 12:19:03 2017 +1100
commit f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")

Quoted 8 lines

This causes the following warning in openrisc as reported by Guenter, and repoduced by me.

 WARNING: CPU: 0 PID: 0 at ./include/linux/jump_label.h:198 0xc02d758c
 static_key_slow_inc used before call to jump_label_init

This fixes this by initialized jump_labels even earlier, I am suprized the issue is not showing up in other platforms yet.

AFAICS it depends on how much memory is available at that point. But maybe I just missed the warning somehow.

Anyway I'm not sure if this patch is safe. Hopefully Peter can judge this better...

Quoted 28 lines
Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -2
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
 

@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
 
-	jump_label_init();
-
 	/*
 	 * These use large bootmem allocations and must precede
 	 * kmem_cache_init()
#2 lore raw
Quoted 21 lines · Vlastimil Babka

Anyway I'm not sure if this patch is safe. Hopefully Peter can judge this better...

Quoted 18 lines
Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -0
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
Quoted 6 lines

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

Quoted 6 lines
init/main.c +0 -2
@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
 
-	jump_label_init();
-

Urgh, that means auditing all archs that implement this. The thing you're looking for is if the self-modifying code cruft can be done that early.

x86 looks to be fine, because this is after setup_arch() which is required for ideal_nops[] to be initialied and we use text_poke_early() which doesn't really need anything else.

I've not gone through the other arches...

author
#3 lore raw
Quoted 34 lines · Peter Zijlstra
Quoted 21 lines · Vlastimil Babka

Anyway I'm not sure if this patch is safe. Hopefully Peter can judge this better...

Quoted 18 lines
Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -0
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
Quoted 6 lines

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

Quoted 6 lines
init/main.c +0 -2
@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
 
-	jump_label_init();
-

Urgh, that means auditing all archs that implement this. The thing you're looking for is if the self-modifying code cruft can be done that early.

Quoted 5 lines

x86 looks to be fine, because this is after setup_arch() which is required for ideal_nops[] to be initialied and we use text_poke_early() which doesn't really need anything else.

I've not gone through the other arches...

Vlastimil,

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

#4 lore raw
Quoted 57 lines · Stafford Horne
Quoted 34 lines · Peter Zijlstra
Quoted 21 lines · Vlastimil Babka

Anyway I'm not sure if this patch is safe. Hopefully Peter can judge this better...

Quoted 18 lines
Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -0
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
Quoted 6 lines

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

Quoted 6 lines
init/main.c +0 -2
@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
 
-	jump_label_init();
-

Urgh, that means auditing all archs that implement this. The thing you're looking for is if the self-modifying code cruft can be done that early.

Quoted 5 lines

x86 looks to be fine, because this is after setup_arch() which is required for ideal_nops[] to be initialied and we use text_poke_early() which doesn't really need anything else.

I've not gone through the other arches...

Vlastimil,

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL
Quoted 3 lines

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

Thanks, I'll try.

Quoted 2 lines

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

Yes that would be uglier but possible if I find issues or I'm not confident enough with the auditing...

maintainer
#5 lore raw
Quoted 19 lines
Quoted 15 lines

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL
Quoted 3 lines

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

Quoted 7 lines

Thanks, I'll try.

Quoted 2 lines

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

Yes that would be uglier but possible if I find issues or I'm not confident enough with the auditing...

We could just revert f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")?

#6 lore raw
Quoted 21 lines · Andrew Morton
Quoted 21 lines
Quoted 15 lines

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL
Quoted 3 lines

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

Thanks, I'll try.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

Yes that would be uglier but possible if I find issues or I'm not confident enough with the auditing...

We could just revert f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")?

That's a -next commit id, as the patch is in mmotm. I'll ask for removal if I don't have a fix soon, but if you or somebody else prefers to do that ASAP, it can be re-added later with a fix.

#7 lore raw
Quoted 28 lines · Vlastimil Babka
Quoted 27 lines · Andrew Morton
Quoted 26 lines
Quoted 15 lines

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL
Quoted 3 lines

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

Thanks, I'll try.

Quoted 2 lines

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

Yes that would be uglier but possible if I find issues or I'm not confident enough with the auditing...

Quoted 6 lines
Quoted 2 lines

We could just revert f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")?

That's a -next commit id, as the patch is in mmotm. I'll ask for removal if I don't have a fix soon, but if you or somebody else prefers to do that ASAP, it can be re-added later with a fix.

OK I think that we just drop the patch [1] from mmotm. Mel told me the benefit was marginal, and also the last move of jump_label_init() caused problems for several releases.

Thanks,
Vlastimil

[1] mm-page_alloc-convert-page_group_by_mobility_disable-to-static-key.patch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#8 lore raw
Quoted 32 lines · Vlastimil Babka
Quoted 31 lines · Vlastimil Babka
Quoted 29 lines · Andrew Morton
Quoted 19 lines
Quoted 15 lines

Will you be able to look into that? Openrisc doesnt have jump_label support, so its no issue at the moment.

Archs that do have it:

arch/arm64/Kconfig:     select HAVE_ARCH_JUMP_LABEL
arch/mips/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/s390/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/sparc/Kconfig:     select HAVE_ARCH_JUMP_LABEL if SPARC64
arch/tile/Kconfig:      select HAVE_ARCH_JUMP_LABEL
arch/x86/Kconfig:       select HAVE_ARCH_JUMP_LABEL
arch/arm/Kconfig:       select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
arch/powerpc/Kconfig:   select HAVE_ARCH_JUMP_LABEL
Quoted 3 lines

I looked at a few (arm, tile) and I dont see their arch_jump_label_transform* implementations depending on global state like ideal_nops from x86. They should be ok.

Quoted 7 lines

Thanks, I'll try.

Quoted 2 lines

If no time, Should you change your patch to not use static keys for build_all_zonelists at least?

Yes that would be uglier but possible if I find issues or I'm not confident enough with the auditing...

We could just revert f5adbdff6a1c40e19 ("mm, page_alloc: convert page_group_by_mobility_disable to static key")?

Quoted 8 lines
Quoted 3 lines

That's a -next commit id, as the patch is in mmotm. I'll ask for removal if I don't have a fix soon, but if you or somebody else prefers to do that ASAP, it can be re-added later with a fix.

OK I think that we just drop the patch [1] from mmotm. Mel told me the benefit was marginal, and also the last move of jump_label_init() caused problems for several releases.

Note that it's not guaranteed to cause any problems this time. If jump_label_init can go ahead without the page allocator being fully up and running then it may be ok.

Signature · 8 lines
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#9 lore raw
In reply to Peter Zijlstra
Quoted 27 lines · Peter Zijlstra
Quoted 25 lines · Vlastimil Babka

Anyway I'm not sure if this patch is safe. Hopefully Peter can judge this better...

Quoted 22 lines
Cc Vlastimil Babka <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
init/main.c +1 -0
index 8b1adb6e..d1ca7cb 100644
@@ -513,6 +513,7 @@ asmlinkage __visible void __init start_kernel(void)
 	boot_cpu_state_init();
 	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
 
+	jump_label_init();
 	build_all_zonelists(NULL, NULL);
 	page_alloc_init();
 

@@ -526,8 +527,6 @@ asmlinkage __visible void __init start_kernel(void)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
 			   NULL, set_init_arg);
Quoted 6 lines
Quoted 2 lines

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

Quoted 2 lines
init/main.c +0 -2
-	jump_label_init();
-

Urgh, that means auditing all archs that implement this. The thing you're looking for is if the self-modifying code cruft can be done that early.

You could do what we do on powerpc, which is to call jump_label_init() early in arch code.

The second call from generic code will just return without doing anything, see the start of jump_label_init():

void __init jump_label_init(void)

{
	struct jump_entry *iter_start = __start___jump_table;
	struct jump_entry *iter_stop = __stop___jump_table;
	struct static_key *key = NULL;
	struct jump_entry *iter;

	/*
	 * Since we are initializing the static_key.enabled field with
	 * with the 'raw' int values (to avoid pulling in atomic.h) in
	 * jump_label.h, let's make sure that is safe. There are only two
	 * cases to check since we initialize to 0 or 1.
	 */
	BUILD_BUG_ON((int)ATOMIC_INIT(0) != 0);
	BUILD_BUG_ON((int)ATOMIC_INIT(1) != 1);

	if (static_key_initialized)
		return;

cheers