Messages in this thread | | | Date | Thu, 9 Mar 2023 14:47:24 +0800 | Subject | Re: [PATCH] ftrace: Add ftrace_page to list after the index is calculated | From | Chen Zhongjin <> |
| |
On 2023/3/8 22:53, Steven Rostedt wrote: > >> - } else { >> - if (!ftrace_pages) >> - goto out; >> - >> - if (WARN_ON(ftrace_pages->next)) { >> - /* Hmm, we have free pages? */ >> - while (ftrace_pages->next) >> - ftrace_pages = ftrace_pages->next; >> - } >> - >> - ftrace_pages->next = start_pg; > Basically, what you are saying is that once we add ftrace_pages->next to > point to the new start_pg, it becomes visible to others and that could be a > problem. And moving this code around is not really going to solve that, as > then we would need to add memory barriers. > >> - } >> - >> p = start; >> pg = start_pg; >> while (p < end) { >> @@ -6855,6 +6841,21 @@ static int ftrace_process_locs(struct module *mod, >> /* We should have used all pages */ >> WARN_ON(pg->next); >> >> + /* Add pages to ftrace_pages list */ >> + if (!mod) { >> + WARN_ON(ftrace_pages || ftrace_pages_start); >> + /* First initialization */ >> + ftrace_pages_start = start_pg; >> + } else { >> + if (WARN_ON(ftrace_pages->next)) { >> + /* Hmm, we have free pages? */ >> + while (ftrace_pages->next) >> + ftrace_pages = ftrace_pages->next; >> + } >> + >> + ftrace_pages->next = start_pg; >> + } >> + >> /* Assign the last page to ftrace_pages */ >> ftrace_pages = pg; >> > Why not just test for it? > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 29baa97d0d53..9b2803c7a18f 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -1564,7 +1564,8 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end) > key.flags = end; /* overload flags, as it is unsigned long */ > > for (pg = ftrace_pages_start; pg; pg = pg->next) { > - if (end < pg->records[0].ip || > + if (pg->index == 0 || > + end < pg->records[0].ip || > start >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE)) > continue; > rec = bsearch(&key, pg->records, pg->index, > > > -- Steve Thanks for review!
At first I'm worried that it will cause lookup_rec to return an incorrect result if it issearching a module address going to be added. But now I found that records are added at MODULE_STATE_UNFORMED, the module address won't be searched at this point in any case.
Let's do it this way. I'll send another patch.
| |