Re: [PATCH -v5 05/17] x86/ftrace: Use text_poke()

[Peter Zijlstra]

On Tue, Nov 12, 2019 at 05:48:16PM -0500, Steven Rostedt wrote:
> On Tue, 12 Nov 2019 23:24:13 +0100
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> > On Tue, Nov 12, 2019 at 01:25:36PM -0500, Steven Rostedt wrote:
> > > On Mon, 11 Nov 2019 14:12:57 +0100
> > > Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:  
> > 
> > > >  int ftrace_arch_code_modify_post_process(void)
> > > >      __releases(&text_mutex)
> > > >  {
> > > > -	set_all_modules_text_ro();
> > > > -	set_kernel_text_ro();
> > > > +	text_poke_finish();  
> > > 
> > > Why is the text_poke_finish() needed here? Can we add a comment about
> > > why?  
> > 
> > I think this is because of the text_poke_queue() in
> > ftrace_modify_code_direct(). I seem to have forgotten the code-flow
> > between the core and arch parts of ftrace again.
> 
> Hmm, I don't think there's a case where ftrace_make_nop() or
> ftrace_make_call() ever use the queued function. I added this:
> 
>  static int
>  ftrace_modify_code_direct(unsigned long ip, const char *old_code,
>  			  const char *new_code)
>  {
>  	int ret = ftrace_verify_code(ip, old_code);
>  	if (ret)
>  		return ret;
>  
>  	/* replace the text with the new text */
> - 	if (ftrace_poke_late)
> + 	if (ftrace_poke_late) {
> +		printk("POKE LATE!\n");
>  		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
> - 	else
> + 	} else
>  		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
>  	return 0;
>  }
> 
> And that printk() never printed, even after running the ftracetests.

Well, then wth did it do that set_all_modules_text_rw() nonsense?
Because all I did was preserve that semantic.

Anyway, all this can be greatly simplified once we get KLP fixed and can
move where we flip modules RO,X.

At that point we can merge ftrace_module_init() and
ftrace_module_enable() (both will run before RO,X) and the core code
will loose the ftrace_arch_code_modify_*() calls (for that callchain)
and then we can remove ftrace_poke_late.

So I'll keep this for now, because it does exactly what the old code
did, and then we can clean it all up once the other stuff lands and
everything gets simpler.