Re: [PATCH perf-tools-next v12 06/10] perf ftrace: Support display of inlined functions in function graph tracer

[Aaron Tomlin]

> > +/**
> > + * ftrace_resolve_inlines - Resolve inlined functions for a return address
> > + * @machine: Perf machine context
> > + * @sym_name: Name of the caller function
> > + * @offset: Instruction offset within the caller function
> > + * @inlined_names: Array to store the resolved inline function names
> > + * @max_inlines: Maximum number of inline frames to resolve
> > + *
> > + * Use DWARF debug information from the kernel image to resolve a caller's
> > + * return address to its inlined callchain.
> > + *
> > + * Return: The number of inline functions successfully resolved.
> > + */
> > +int ftrace_resolve_inlines(struct machine *machine,
> > +			   const char *sym_name, u64 offset,
> > +			   const char **inlined_names, int max_inlines)
> 
> Why not remove 'const' from the inlined_names?
> 
> > +{
> > +	struct map *map = NULL;
> > +	struct symbol *sym;
> > +	struct dso *dso;
> > +	struct inline_node *node;
> > +	struct inline_list *ilist;
> > +	u64 ip, addr;
> > +	int count = 0;
> > +	int j;
> > +
> > +	if (!machine || !offset)
> > +		return 0;
> > +
> > +	sym = machine__find_kernel_symbol_by_name(machine, sym_name, &map);
> > +	if (!sym || !map)
> > +		return 0;
> > +
> > +	dso = map__dso(map);
> > +	if (!dso) {
> > +		map__put(map);
> > +		return 0;
> > +	}
> > +
> > +	ip = sym->start + offset - 1;
> > +	addr = map__rip_2objdump(map, ip);
> > +
> > +	node = inlines__tree_find(dso__inlined_nodes(dso), addr);
> > +	if (!node) {
> > +		node = dso__parse_addr_inlines(dso, addr, sym);
> > +		if (node)
> > +			inlines__tree_insert(dso__inlined_nodes(dso), node);
> > +	}
> > +
> > +	if (!node) {
> > +		map__put(map);
> > +		return 0;
> > +	}
> > +
> > +	list_for_each_entry_reverse(ilist, &node->val, list) {
> > +		if (ilist->symbol && symbol__inlined(ilist->symbol)) {
> > +			if (count < max_inlines) {
> 
> You can reduce the indentation by inverting the condition.
> 
> 
> > +				char *name = strdup(ilist->symbol->name);
> 
> Why not just save the pointer?  You may not use some of them, also it
> copies the string again when it's added to the stack.  It'd make the
> error handling a bit easier.

Since the inline_node is cached in the DSO's inlined_nodes tree for the
lifetime of ftrace->machine, we can store ilist->symbol->name directly in
inlined_names without strdup() and without freeing each entry in the
caller. I will also invert the condition in the loop to flatten the
indentation.

> > +		cs->stack[cs->depth].inlined = true;
> > +		cs->stack[cs->depth].indent = entry_indent;
> > +		cs->stack[cs->depth].raw_indent = -1;
> 
> I'm curious if it's possible to get rid of the indent and raw_indent.
> I think you can get the indent from the depth easily.  The raw indent
> would be from any non-inlined parent if needed.  Also having inlined
> depth in the cs/ts seems unnecessary.  What do you think?

We still need to record the trace line's indentation rather than deriving
it purely from ts->depth * 2 because tracing can start mid-callchain (or
after ring-buffer drops), where ts->depth == 0 has a non-zero kernel
indentation (e.g. 6 or 8 spaces), and bare "}" exits must be matched
against that kernel indentation.

However, we do not need both indent and raw_indent on struct
ftrace_stack_frame, nor do we need inlined_depth in struct
ftrace_task_state:

    1.  In struct ftrace_stack_frame, indent was only used for inlined
        frames and raw_indent was only used for non-inlined frames, so they
        can be consolidated into a single indent field.

    2.  In struct ftrace_task_state, inlined_depth can be dropped
        altogether and computed from the active inlined frames on ts->stack
        when needed.

I will simplify both structures accordingly.

> > +#define FTRACE_MAX_INLINES 32
> 
> Isn't 32 too big?  I guess 8 would be enough..

No, 8 is not enough in practice. I will prefer to keep 32.
For instance, after a -O2 kernel build:

❯ addr2line -f -i -e vmlinux 0xffffffff816916e9
      __do_trace_rss_stat
      include/trace/events/kmem.h:427 (discriminator 20)
      trace_rss_stat
      include/trace/events/kmem.h:427 (discriminator 20)
      mm_trace_rss_stat
      mm/memory.c:169 (discriminator 20)
      add_mm_counter
      include/linux/mm.h:3469 (discriminator 20)
      add_mm_rss_vec
      mm/memory.c:492 (discriminator 20)
      zap_pte_range
      mm/memory.c:2087 (discriminator 20)
      zap_pmd_range
      mm/memory.c:2151 (discriminator 20)
      zap_pud_range
      mm/memory.c:2179 (discriminator 20)
      zap_p4d_range
      mm/memory.c:2200 (discriminator 20)
      __zap_vma_range
      mm/memory.c:2240 (discriminator 20)

Another example:

❯ addr2line -f -i -e vmlinux 0xffffffff821c34af
      constant_test_bit
      arch/x86/include/asm/bitops.h:202
      arch_test_bit
      arch/x86/include/asm/bitops.h:232
      _test_bit
      include/asm-generic/bitops/instrumented-non-atomic.h:142
      test_ti_thread_flag
      include/linux/thread_info.h:133
      test_tsk_thread_flag
      include/linux/sched.h:2102
      task_sigpending
      include/linux/sched/signal.h:384
      fatal_signal_pending
      include/linux/sched/signal.h:406
      ptrace_report_syscall
      include/linux/ptrace.h:427
      ptrace_report_syscall_exit
      include/linux/ptrace.h:477
      arch_ptrace_report_syscall_exit
      include/linux/entry-common.h:233
      syscall_exit_work
      include/linux/entry-common.h:266
      syscall_exit_to_user_mode_work
      include/linux/entry-common.h:297
      syscall_exit_to_user_mode
      include/linux/entry-common.h:334
      do_syscall_64
      arch/x86/entry/syscall_64.c:89


Kind regards,
-- 
Aaron Tomlin