On Mon, Jul 01, 2024 at 07:27:55PM GMT, Nicholas Piggin wrote:
> On Fri Jun 21, 2024 at 4:54 AM AEST, Naveen N Rao 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.
>
> arm is the only one left that requires dyn_arch_ftrace after this.
Yes, but as you noticed, we add a different field in a subsequenct patch
in this series.
>
> >
> > Signed-off-by: Naveen N Rao <nav...@kernel.org>
> > ---
> > arch/powerpc/include/asm/ftrace.h | 1 -
> > arch/powerpc/kernel/trace/ftrace.c | 54 +++++++++++-------
> > arch/powerpc/kernel/trace/ftrace_64_pg.c | 73 +++++++++++-------------
> > 3 files changed, 65 insertions(+), 63 deletions(-)
> >
>
> [snip]
>
> > @@ -106,28 +106,48 @@ static unsigned long find_ftrace_tramp(unsigned long
> > ip)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_MODULES
> > +static unsigned long ftrace_lookup_module_stub(unsigned long ip, unsigned
> > long addr)
> > +{
> > + 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(ip);
> > + preempt_enable();
>
> If 'mod' was guaranteed to exist before your patch, then it
> should do afterward too. But is it always ftrace_lock that
> protects it, or do dyn_ftrace entries pin a module in some
> cases?
We don't pin a module. It is the ftrace_lock acquired during
delete_module() in ftrace_release_mod() that protects it.
You're right though. That comment is probably not necessary since there
are no new users of this new function.
>
> > @@ -555,7 +551,10 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned
> > long old_addr,
> > ppc_inst_t op;
> > unsigned long ip = rec->ip;
> > unsigned long entry, ptr, tramp;
> > - struct module *mod = rec->arch.mod;
> > + struct module *mod = ftrace_lookup_module(rec);
> > +
> > + if (!mod)
> > + return -EINVAL;
> >
> > /* If we never set up ftrace trampolines, then bail */
> > if (!mod->arch.tramp || !mod->arch.tramp_regs) {
> > @@ -668,14 +667,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec,
> > unsigned long old_addr,
> > return -EINVAL;
> > }
> >
> > - /*
> > - * Out of range jumps are called from modules.
> > - */
> > - if (!rec->arch.mod) {
> > - pr_err("No module loaded\n");
> > - return -EINVAL;
> > - }
> > -
>
> A couple of these conversions are not _exactly_ the same (lost
> the pr_err here), maybe that's deliberate because the messages
> don't look too useful.
Indeed. Most of the earlier ones being eliminated are in
ftrace_init_nop(). The other ones get covered by the pr_err in
ftrace_lookup_module()/ftrace_lookup_module_stub().
>
> Looks okay though
>
> Reviewed-by: Nicholas Piggin <npig...@gmail.com>
Thanks,
Naveen
