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.
>
> Signed-off-by: Naveen N Rao <naveen@xxxxxxxxxx>
> ---
> 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?
> @@ -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.
Looks okay though
Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>
