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