[PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Stafford Horne]

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

  Author: Vlastimil Babka <vbabka@suse.cz>
  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 <vbabka@suse.cz>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/init/main.c b/init/main.c
index 8b1adb6e..d1ca7cb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -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.9.3

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Vlastimil Babka]

[+CC PeterZ]

On 01/17/2017 01:56 PM, Stafford Horne wrote:
> A recent change in -next introduced static_keys in init zonelists,
> details are:
> 
>   Author: Vlastimil Babka <vbabka@suse.cz>
>   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.

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

> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>  init/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 8b1adb6e..d1ca7cb 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -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()
> 

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Peter Zijlstra]

On Tue, Jan 17, 2017 at 02:07:36PM +0100, Vlastimil Babka wrote:

> Anyway I'm not sure if this patch is safe. Hopefully Peter can judge
> this better...
> 
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >  init/main.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/init/main.c b/init/main.c
> > index 8b1adb6e..d1ca7cb 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -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();
> > -

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

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Stafford Horne]

On Tue, Jan 17, 2017 at 02:44:54PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 17, 2017 at 02:07:36PM +0100, Vlastimil Babka wrote:
> 
> > Anyway I'm not sure if this patch is safe. Hopefully Peter can judge
> > this better...
> > 
> > > Cc: Vlastimil Babka <vbabka@suse.cz>
> > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > > ---
> > >  init/main.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/init/main.c b/init/main.c
> > > index 8b1adb6e..d1ca7cb 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -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();
> > > -
> 
> 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...

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?

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Vlastimil Babka]

On 01/17/2017 03:30 PM, Stafford Horne wrote:
> On Tue, Jan 17, 2017 at 02:44:54PM +0100, Peter Zijlstra wrote:
>> On Tue, Jan 17, 2017 at 02:07:36PM +0100, Vlastimil Babka wrote:
>>
>>> Anyway I'm not sure if this patch is safe. Hopefully Peter can judge
>>> this better...
>>>
>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>> Signed-off-by: Stafford Horne <shorne@gmail.com>
>>>> ---
>>>>  init/main.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/init/main.c b/init/main.c
>>>> index 8b1adb6e..d1ca7cb 100644
>>>> --- a/init/main.c
>>>> +++ b/init/main.c
>>>> @@ -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();
>>>> -
>>
>> 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...
> 
> 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.

Thanks, I'll try.

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

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Andrew Morton]

> > 
> > 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.
> 
> Thanks, I'll try.
> 
> > 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")?

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Vlastimil Babka]

On 17.1.2017 21:34, Andrew Morton wrote:
>>>
>>> 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.
>>
>> Thanks, I'll try.
>>
>>> 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.

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Vlastimil Babka]

On 01/17/2017 09:49 PM, Vlastimil Babka wrote:
> On 17.1.2017 21:34, Andrew Morton wrote:
>>>>
>>>> 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.
>>>
>>> Thanks, I'll try.
>>>
>>>> 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.

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

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Mel Gorman]

On Thu, Jan 19, 2017 at 09:28:28AM +0100, Vlastimil Babka wrote:
> On 01/17/2017 09:49 PM, Vlastimil Babka wrote:
> > On 17.1.2017 21:34, Andrew Morton wrote:
> >>>>
> >>>> 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.
> >>>
> >>> Thanks, I'll try.
> >>>
> >>>> 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.
> 
> 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.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH -next] init/main: Init jump_labels before they are used to build zonelists

[Michael Ellerman]

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Jan 17, 2017 at 02:07:36PM +0100, Vlastimil Babka wrote:
>
>> Anyway I'm not sure if this patch is safe. Hopefully Peter can judge
>> this better...
>> 
>> > Cc: Vlastimil Babka <vbabka@suse.cz>
>> > Signed-off-by: Stafford Horne <shorne@gmail.com>
>> > ---
>> >  init/main.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> > 
>> > diff --git a/init/main.c b/init/main.c
>> > index 8b1adb6e..d1ca7cb 100644
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -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();
>> > -
>
> 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