Re: [PATCH v1 bpf-next 15/18] resolve_btfids: Tag fmod_ret candidates as targets

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]

> resolve_btfids: Tag fmod_ret candidates as targets

TL;DR: Nits only in resolve_btfids fmodret.c: duplicated alias-walk code in
mark_traceable_function(), its unused bool return, and possibly obsolete
section names (.meminit, .cpuinit, .devinit) in section_cannot_be_attached().

> diff --git a/tools/bpf/resolve_btfids/fmodret.c b/tools/bpf/resolve_btfids/fmodret.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/fmodret.c

[ ... ]

> +static bool mark_traceable_function(struct elf_function_list *list,
> +				    Elf64_Section shndx, GElf_Addr address,
> +				    enum ftrace_site_kind kind)
> +{
> +	size_t left = 0;
> +	size_t right = list->nr;
> +	size_t first;
> +	size_t i;
> +	bool found = false;

[ ... ]

> +	if (first != SIZE_MAX) {
> +		while (first && list->functions[first - 1].shndx == shndx &&
> +		       list->functions[first - 1].value ==
> +						list->functions[first].value)
> +			first--;
> +		for (i = first; i < list->nr &&
> +		     list->functions[i].shndx == shndx &&
> +		     list->functions[i].value ==
> +					list->functions[first].value; i++) {
> +			list->functions[i].traceable = true;
> +			found = true;
> +		}
> +		return found;
> +	}
> +	if (kind == FTRACE_SITE_PATCHABLE || !left)
> +		return false;
> +
> +	/* The nearest preceding function owns the call site. */
> +	first = left - 1;
> +	if (list->functions[first].shndx != shndx)
> +		return false;
> +	while (first && list->functions[first - 1].shndx == shndx &&
> +	       list->functions[first - 1].value == list->functions[first].value)
> +		first--;
> +	for (i = first; i < list->nr &&
> +	     list->functions[i].shndx == shndx &&
> +	     list->functions[i].value == list->functions[first].value; i++) {
> +		if (!function_contains(&list->functions[i], address))
> +			continue;
> +		list->functions[i].traceable = true;
> +		found = true;
> +	}
> +	return found;
> +}

This isn't a bug, but mark_traceable_function() has two near-identical blocks
that walk back to the first alias at a (shndx, value) pair and then loop
forward over the aliases.  The second differs only by the
function_contains() test.

Could the walk back use function_at(list, shndx, list->functions[first].value),
with one forward loop that takes the containment test as a condition?  Then
the two copies would not need to be kept in sync.

Also, this isn't a bug, but is the bool return of mark_traceable_function()
meant for something, such as a -v count of unmatched ftrace sites?  None of
its callers in mark_relocated_ftrace_sites() and read_ftrace_site_range()
read the value, so could it return void and drop 'found'?

[ ... ]

> +static bool section_cannot_be_attached(const char *name)
> +{
> +	return section_prefix(name, ".init.text") ||
> +	       section_prefix(name, ".exit.text") ||
> +	       section_prefix(name, ".noinstr.text") ||
> +	       section_prefix(name, ".meminit.text") ||
> +	       section_prefix(name, ".memexit.text") ||
> +	       section_prefix(name, ".cpuinit.text") ||
> +	       section_prefix(name, ".cpuexit.text") ||
> +	       section_prefix(name, ".devinit.text") ||
> +	       section_prefix(name, ".devexit.text");
> +}

This isn't a bug, but are .meminit.text, .memexit.text, .cpuinit.text,
.cpuexit.text, .devinit.text and .devexit.text still emitted anywhere?

In include/linux/init.h, __meminit is now either empty or __init, and the
__cpuinit/__devinit annotations were removed.  Neither vmlinux.lds.h nor
scripts/module.lds.S places these sections, and modpost lists only
.init.text and .exit.text as init/exit text.

If they are gone, could section_cannot_be_attached() be reduced to
.init.text, .exit.text and .noinstr.text?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42294987431