[RFC PATCH v2 2/5] powerpc/ftrace: Remove pointer to struct module from dyn_arch_ftrace

[Naveen N Rao]

On Mon, Jun 10, 2024 at 04:03:56PM GMT, Steven Rostedt wrote:
> On Mon, 10 Jun 2024 14:08:15 +0530
> Naveen N Rao <naveen at kernel.org> wrote:
> 
> > Pointer to struct module is only relevant for ftrace records belonging
> > to kernel modules. Having this field in dyn_arch_ftrace wastes memory
> > for all ftrace records belonging to the kernel. Remove the same in
> > favour of looking up the module from the ftrace record address, similar
> > to other architectures.
> > 
> > Signed-off-by: Naveen N Rao <naveen at kernel.org>
> > ---
> >  arch/powerpc/include/asm/ftrace.h        |  1 -
> >  arch/powerpc/kernel/trace/ftrace.c       | 47 ++++++++++-----
> >  arch/powerpc/kernel/trace/ftrace_64_pg.c | 73 +++++++++++-------------
> >  3 files changed, 64 insertions(+), 57 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> > index 107fc5a48456..201f9d15430a 100644
> > --- a/arch/powerpc/include/asm/ftrace.h
> > +++ b/arch/powerpc/include/asm/ftrace.h
> > @@ -26,7 +26,6 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
> >  struct module;
> >  struct dyn_ftrace;
> >  struct dyn_arch_ftrace {
> > -	struct module *mod;
> >  };
> 
> Nice. I always hated that extra field.

It was your complaint a while back that prompted this change :)

Though I introduce a different pointer here in the next patch. /me 
ducks.

> 
> 
> >  
> >  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> > index d8d6b4fd9a14..041be965485e 100644
> > --- a/arch/powerpc/kernel/trace/ftrace.c
> > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > @@ -106,20 +106,36 @@ static unsigned long find_ftrace_tramp(unsigned long ip)
> >  	return 0;
> >  }
> >  
> > +static struct module *ftrace_lookup_module(struct dyn_ftrace *rec)
> > +{
> > +	struct module *mod = NULL;
> > +
> > +#ifdef CONFIG_MODULES
> > +	/*
> > +	 * NOTE: __module_text_address() must be called with preemption
> > +	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
> > +	 * retains its validity throughout the remainder of this code.
> > +	*/
> > +	preempt_disable();
> > +	mod = __module_text_address(rec->ip);
> > +	preempt_enable();
> > +
> > +	if (!mod)
> > +		pr_err("No module loaded at addr=%lx\n", rec->ip);
> > +#endif
> > +
> > +	return mod;
> > +}
> 
> It may look nicer to have:
> 
> #ifdef CONFIG_MODULES
> static struct module *ftrace_lookup_module(struct dyn_ftrace *rec)
> {
> 	struct module *mod = NULL;
> 
> 	/*
> 	 * NOTE: __module_text_address() must be called with preemption
> 	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
> 	 * retains its validity throughout the remainder of this code.
> 	*/
> 	preempt_disable();
> 	mod = __module_text_address(rec->ip);
> 	preempt_enable();
> 
> 	if (!mod)
> 		pr_err("No module loaded at addr=%lx\n", rec->ip);
> 
> 	return mod;
> }
> #else
> static inline struct module *ftrace_lookup_module(struct dyn_ftrace *rec)
> {
> 	return NULL;
> }
> #endif

I wrote this, and then I thought it will be simpler to do the version I 
posted. I will move back to this since it looks to be the preferred way.

> 
> > +
> >  static int ftrace_get_call_inst(struct dyn_ftrace *rec, unsigned long addr, ppc_inst_t *call_inst)
> >  {
> >  	unsigned long ip = rec->ip;
> >  	unsigned long stub;
> > +	struct module *mod;
> >  
> >  	if (is_offset_in_branch_range(addr - ip)) {
> >  		/* Within range */
> >  		stub = addr;
> > -#ifdef CONFIG_MODULES
> > -	} else if (rec->arch.mod) {
> > -		/* Module code would be going to one of the module stubs */
> > -		stub = (addr == (unsigned long)ftrace_caller ? rec->arch.mod->arch.tramp :
> > -							       rec->arch.mod->arch.tramp_regs);
> > -#endif
> >  	} else if (core_kernel_text(ip)) {
> >  		/* We would be branching to one of our ftrace stubs */
> >  		stub = find_ftrace_tramp(ip);
> > @@ -128,7 +144,16 @@ static int ftrace_get_call_inst(struct dyn_ftrace *rec, unsigned long addr, ppc_
> >  			return -EINVAL;
> >  		}
> >  	} else {
> > -		return -EINVAL;
> > +		mod = ftrace_lookup_module(rec);
> > +		if (mod) {
> > +#ifdef CONFIG_MODULES
> > +			/* Module code would be going to one of the module stubs */
> > +			stub = (addr == (unsigned long)ftrace_caller ? mod->arch.tramp :
> > +								       mod->arch.tramp_regs);
> > +#endif
> 
> You have CONFIG_MODULES here and in ftrace_lookup_module() above, which
> would always return NULL. Could you combine the above to be done in
> ftrace_lookup_module() so that there's no #ifdef CONFIG_MODULES here?

Yes, indeed. That will look cleaner.


Thanks,
Naveen