| Message ID | 20210429031602.2606654-3-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Further Strict RWX support | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (1b628dbc3ccb0b51963fcb3c60b57daac21dc016) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 25 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > setup_text_poke_area() is a late init call so it runs before > mark_rodata_ro() and after the init calls. This lets all the init code > patching simply write to their locations. In the future, kprobes is > going to allocate its instruction pages RO which means they will need > setup_text__poke_area() to have been already called for their code > patching. However, init_kprobes() (which allocates and patches some > instruction pages) is an early init call so it happens before > setup_text__poke_area(). > > start_kernel() calls poking_init() before any of the init calls. On > powerpc, poking_init() is currently a nop. setup_text_poke_area() relies > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called > before poking_init(). > > Turn setup_text_poke_area() into poking_init(). I can't remember, maybe I already asked the question: Have you done some performance measurement or at least some performance analysis ? Christophe > > Reviewed-by: Russell Currey <ruscur@russell.cc> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v9: New to series > --- > arch/powerpc/lib/code-patching.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index 870b30d9be2f..15296207e1ba 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -70,14 +70,11 @@ static int text_area_cpu_down(unsigned int cpu) > } > > /* > - * Run as a late init call. This allows all the boot time patching to be done > - * simply by patching the code, and then we're called here prior to > - * mark_rodata_ro(), which happens after all init calls are run. Although > - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge > - * it as being preferable to a kernel that will crash later when someone tries > - * to use patch_instruction(). > + * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and > + * we judge it as being preferable to a kernel that will crash later when > + * someone tries to use patch_instruction(). > */ > -static int __init setup_text_poke_area(void) > +int __init poking_init(void) > { > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > "powerpc/text_poke:online", text_area_cpu_up, > @@ -85,7 +82,6 @@ static int __init setup_text_poke_area(void) > > return 0; > } > -late_initcall(setup_text_poke_area); > > /* > * This can be called for kernel text or a module. >
On Thu, Apr 29, 2021 at 2:53 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > > setup_text_poke_area() is a late init call so it runs before > > mark_rodata_ro() and after the init calls. This lets all the init code > > patching simply write to their locations. In the future, kprobes is > > going to allocate its instruction pages RO which means they will need > > setup_text__poke_area() to have been already called for their code > > patching. However, init_kprobes() (which allocates and patches some > > instruction pages) is an early init call so it happens before > > setup_text__poke_area(). > > > > start_kernel() calls poking_init() before any of the init calls. On > > powerpc, poking_init() is currently a nop. setup_text_poke_area() relies > > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. > > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called > > before poking_init(). > > > > Turn setup_text_poke_area() into poking_init(). > > I can't remember, maybe I already asked the question: > Have you done some performance measurement or at least some performance analysis ? No I don't think you have asked and it is a good question. Here are some results on a Power9 (T2P9D01 REV 1.01) running powernv_defconfig Timestamp at "Run /init as init process" Before: ~1.059326 After: ~1.273105 Turning on more testing the difference is greater: For example, turning on CONFIG_FTRACE_STARTUP_TEST Timestamp at "Run /init as init process" Before: ~7.176759 After: ~15.967576 Running with initcall_debug: Before: initcall init_trace_selftests+0x0/0x1b4 returned 0 after 2880859 usecs After: initcall init_trace_selftests+0x0/0x1b4 returned 0 after 10048828 usecs So it does slow it down. But it also might be a good thing for testing that these tests using code patching now will use the same code path for patching that would be used on a fully booted system. > > Christophe > > > > > Reviewed-by: Russell Currey <ruscur@russell.cc> > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v9: New to series > > --- > > arch/powerpc/lib/code-patching.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > > index 870b30d9be2f..15296207e1ba 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -70,14 +70,11 @@ static int text_area_cpu_down(unsigned int cpu) > > } > > > > /* > > - * Run as a late init call. This allows all the boot time patching to be done > > - * simply by patching the code, and then we're called here prior to > > - * mark_rodata_ro(), which happens after all init calls are run. Although > > - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge > > - * it as being preferable to a kernel that will crash later when someone tries > > - * to use patch_instruction(). > > + * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and > > + * we judge it as being preferable to a kernel that will crash later when > > + * someone tries to use patch_instruction(). > > */ > > -static int __init setup_text_poke_area(void) > > +int __init poking_init(void) > > { > > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > > "powerpc/text_poke:online", text_area_cpu_up, > > @@ -85,7 +82,6 @@ static int __init setup_text_poke_area(void) > > > > return 0; > > } > > -late_initcall(setup_text_poke_area); > > > > /* > > * This can be called for kernel text or a module. > >
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 870b30d9be2f..15296207e1ba 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -70,14 +70,11 @@ static int text_area_cpu_down(unsigned int cpu) } /* - * Run as a late init call. This allows all the boot time patching to be done - * simply by patching the code, and then we're called here prior to - * mark_rodata_ro(), which happens after all init calls are run. Although - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge - * it as being preferable to a kernel that will crash later when someone tries - * to use patch_instruction(). + * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and + * we judge it as being preferable to a kernel that will crash later when + * someone tries to use patch_instruction(). */ -static int __init setup_text_poke_area(void) +int __init poking_init(void) { BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powerpc/text_poke:online", text_area_cpu_up, @@ -85,7 +82,6 @@ static int __init setup_text_poke_area(void) return 0; } -late_initcall(setup_text_poke_area); /* * This can be called for kernel text or a module.