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

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 10 Jun 2024 14:08:15 +0530
Naveen N Rao <naveen@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@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.


>  
>  #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

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


> +		} else {
> +			return -EINVAL;
> +		}
>  	}
>  
>  	*call_inst = ftrace_create_branch_inst(ip, stub, 1);
> @@ -256,14 +281,6 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>  	if (ret)
>  		return ret;
>  
> -	if (!core_kernel_text(ip)) {
> -		if (!mod) {
> -			pr_err("0x%lx: No module provided for non-kernel address\n", ip);
> -			return -EFAULT;
> -		}
> -		rec->arch.mod = mod;
> -	}
> -
>  	/* Nop-out the ftrace location */
>  	new = ppc_inst(PPC_RAW_NOP());
>  	addr = MCOUNT_ADDR;

-- Steve