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