Re: [PATCH] ftrace: Fix possible warning on checking all pages used in ftrace_process_locs()
From: Zheng Yejian <hidden>
Date: 2023-07-11 06:21:32
Also in:
lkml
On 2023/7/10 22:46, Steven Rostedt wrote:
On Tue, 11 Jul 2023 05:29:58 +0800 Zheng Yejian [off-list ref] wrote:quoted
As comments in ftrace_process_locs(), there may be NULL pointers in mcount_loc section: > Some architecture linkers will pad between > the different mcount_loc sections of different > object files to satisfy alignments. > Skip any NULL pointers. After 20e5227e9f55 ("ftrace: allow NULL pointers in mcount_loc"), NULL pointers will be accounted when allocating ftrace pages but skipped before adding into ftrace pages, this may result in some pages not being used. Then after 706c81f87f84 ("ftrace: Remove extra helper functions"), warning may occur at: WARN_ON(pg->next); So we may need to skip NULL pointers before allocating ftrace pages. Fixes: 706c81f87f84 ("ftrace: Remove extra helper functions") Signed-off-by: Zheng Yejian <redacted> --- kernel/trace/ftrace.c | 10 ++++++++++ 1 file changed, 10 insertions(+)diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 3740aca79fe7..5b474165df31 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c@@ -6485,6 +6485,16 @@ static int ftrace_process_locs(struct module *mod, if (!count) return 0; + p = start; + while (p < end) { + /* + * Refer to conments below, there may be NULL pointers, + * skip them before allocating pages + */ + addr = ftrace_call_adjust(*p++); + if (!addr) + count--; + }My main concern about this is the added overhead during boot to process this. There's 10s of thousands of functions, so this loop will be 10s of thousands. I also don't like that this is an unconditional loop (meaning it executes even when it is unnecessary to do so).
Agreed! The added overhead probably superfluousin in most cases.
quoted hunk ↗ jump to hunk
quoted
/* * Sorting mcount in vmlinux at build time depend on * CONFIG_BUILDTIME_MCOUNT_SORT, while mcount loc inHow about something like this? -- Stevediff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index b24c573934af..acd033371721 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c@@ -6474,6 +6474,7 @@ static int ftrace_process_locs(struct module *mod, struct ftrace_page *start_pg; struct ftrace_page *pg; struct dyn_ftrace *rec; + unsigned long skipped = 0; unsigned long count; unsigned long *p; unsigned long addr;@@ -6536,8 +6537,10 @@ static int ftrace_process_locs(struct module *mod, * object files to satisfy alignments. * Skip any NULL pointers. */ - if (!addr) + if (!addr) { + skipped++; continue; + } end_offset = (pg->index+1) * sizeof(pg->records[0]); if (end_offset > PAGE_SIZE << pg->order) {@@ -6551,12 +6554,24 @@ static int ftrace_process_locs(struct module *mod, rec->ip = addr; } - /* We should have used all pages */ - WARN_ON(pg->next); - /* Assign the last page to ftrace_pages */ ftrace_pages = pg; + /* We should have used all pages unless we skipped some */ + if (pg->next) { + WARN_ON(!skipped); + while (ftrace_pages->next) { + pg = ftrace_pages->next; + ftrace_pages->next = pg->next; + if (pg->records) { + free_pages((unsigned long)pg->records, pg->order); + ftrace_number_of_pages -= 1 << pg->order; + } + kfree(pg); + ftrace_number_of_groups--; + }
Do we only need to free the pages that not being used?
+ } + /* * We only need to disable interrupts on start up * because we are modifying code that an interrupt