Re: [RFC PATCH v3 04/11] powerpc/ftrace: Remove pointer to struct module from dyn_arch_ftrace

["Nicholas Piggin" <npiggin@gmail.com>]

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@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?

> @@ -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@gmail.com>