Hi Steven,
On Tue, Jan 06, 2026 at 04:18:44PM -0500, Steven Rostedt wrote:
> On Tue, 6 Jan 2026 12:35:33 -0800
> Guenter Roeck <[email protected]> wrote:
>
> > The pg_remaining calculation in ftrace_process_locs() assumes that
> > ENTRIES_PER_PAGE multiplied by 2^order equals the actual capacity of the
> > allocated page group. However, ENTRIES_PER_PAGE is PAGE_SIZE / ENTRY_SIZE
> > (integer division). When PAGE_SIZE is not a multiple of ENTRY_SIZE (e.g.
> > 4096 / 24 = 170 with remainder 16), high-order allocations (like 256 pages)
> > have significantly more capacity than 256 * 170. This leads to pg_remaining
> > being underestimated, which in turn makes skip (derived from skipped -
> > pg_remaining) larger than expected, causing the WARN(skip != remaining)
> > to trigger.
>
> Nice catch! I guess you have a machine that allows much higher order
> allocations than I do ;-)
>
I have actually seen the problem when the system tried to allocate 341
entries. 2 x 170 = 340, so the code allocated three pages, and things
went downhill from there.
Here is a specific example of the allocation sequence with debug
information.
ftrace_allocate_records: Requesting 52904 entries: 310 pages [order 8].
Alternate: 312 pages [order 8]
Allocated 43690 records, 9214 remaining
ftrace_allocate_records: Requesting 9214 entries: 54 pages [order 5].
Alternate: 55 pages [order 5]
Allocated 5461 records, 3753 remaining
ftrace_allocate_records: Requesting 3753 entries: 22 pages [order 4].
Alternate: 23 pages [order 4]
Allocated 2730 records, 1023 remaining
ftrace_allocate_records: Requesting 1023 entries: 6 pages [order 2]. Alternate:
7 pages [order 2]
Allocated 682 records, 341 remaining
ftrace_allocate_records: Requesting 341 entries: 2 pages [order 1]. Alternate:
3 pages [order 1]
Allocated 341 records, 0 remaining
The description above is just a more extreme example.
> >
> > Extra allocated pages for ftrace: 2 with 654 skipped
> > WARNING: CPU: 0 PID: 0 at kernel/trace/ftrace.c:7295
> > ftrace_process_locs+0x5bf/0x5e0
> >
> > A similar problem in ftrace_allocate_records() can result in allocating
> > too many pages. This can trigger the second warning in
> > ftrace_process_locs().
> >
> > Extra allocated pages for ftrace
> > WARNING: CPU: 0 PID: 0 at kernel/trace/ftrace.c:7276
> > ftrace_process_locs+0x548/0x580
> >
> > Use the actual capacity of a page group to determine if too many pages
> > have been allocated to solve the problem. Also use the actual capacity
> > of a page group to determine the number of pages needed to avoid over-
> > allocations in ftrace_allocate_records().
> >
> > Fixes: 4a3efc6baff93 ("ftrace: Update the mcount_loc check of skipped
> > entries")
> > Cc: Steven Rostedt <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > kernel/trace/ftrace.c | 25 +++++++++++++++++--------
> > 1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index ef2d5dca6f70..211ec7a04f7e 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -3844,7 +3844,7 @@ static int ftrace_allocate_records(struct ftrace_page
> > *pg, int count)
> > return -EINVAL;
> >
> > /* We want to fill as much as possible, with no empty pages */
> > - pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
> > + pages = DIV_ROUND_UP(count * ENTRY_SIZE, PAGE_SIZE);
> > order = fls(pages) - 1;
> >
> > again:
> > @@ -7308,24 +7308,33 @@ static int ftrace_process_locs(struct module *mod,
> > unsigned long skip;
> >
> > /* Count the number of entries unused and compare it to
> > skipped. */
> > - pg_remaining = (ENTRIES_PER_PAGE << pg->order) - pg->index;
> > + pg_remaining = (PAGE_SIZE << pg->order) / ENTRY_SIZE -
> > pg->index;
> >
> > if (!WARN(skipped < pg_remaining, "Extra allocated pages for
> > ftrace")) {
> > + unsigned long space = 0;
> >
> > skip = skipped - pg_remaining;
> >
> > - for (pg = pg_unuse; pg; pg = pg->next)
> > + for (pg = pg_unuse; pg; pg = pg->next) {
> > remaining += 1 << pg->order;
> > + /*
> > + * The capacity of a page group is
> > + * (PAGE_SIZE << order) / ENTRY_SIZE
> > + * Accumulate the total capacity of unused
> > pages.
> > + */
> > + space += (PAGE_SIZE << pg->order) / ENTRY_SIZE;
> > + }
> >
> > pages -= remaining;
>
> I think pages is meaningless here, as it was set in the beginning with:
>
> pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
>
> Which is incorrect. I wonder if we should set it via:
>
I know, I just didn't want to make more changes than absolutely
necessary.
> /*
> * Use ftrace_number_of_pages to determine how many pages were
> * allocated
> */
> pages = ftrace_number_of_pages;
>
> start_pg = ftrace_allocate_pages(count);
> if (!start_pg)
> return -ENOMEM;
>
> /* ftrace_allocate_pages() increments ftrace_number_of_pages */
> pages = ftrace_number_of_pages - pages;
>
That might work, assuming that the code updating ftrace_number_of_pages
is (mutex) protected. I don't immediately see that, and the
"mutex_lock(&ftrace_lock);" right after the above code makes me a bit
concerned.
> This will make pages equal the number of pages that were allocated. Then
> I'm not sure we need this extra logic.
>
Which extra logic do you refer to ? Everything in "if (pg_unused)"
except for the calls to synchronize_rcu() and ftrace_free_pages() ?
If so I'll be more than happy to drop that.
Thanks,
Guenter
