Re: [PATCH v3] ftrace: Do not over-allocate ftrace memory

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 12 Jan 2026 16:15:56 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> > v3: Restore the code calculating the number of unused pages due to skipped
> >      entries. Use the actual number of entries per page group when
> >      calculating the number of entries in unused page groups.
> > v2: Have ftrace_allocate_pages() return the number of allocated pages,
> >      and drop the page count calculation code as well as the associated
> >      warnings from ftrace_process_locs().  
> 
> Any additional feedback / comments / suggestions ?

Yeah, sorry got caught up in other work.


> > -static int ftrace_allocate_records(struct ftrace_page *pg, int count)
> > +static int ftrace_allocate_records(struct ftrace_page *pg, int count,
> > +				   unsigned long *num_pages)
> >   {
> >   	int order;
> >   	int pages;
> > @@ -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:
> > @@ -3859,6 +3859,7 @@ static int ftrace_allocate_records(struct ftrace_page *pg, int count)
> >   	}
> >   
> >   	ftrace_number_of_pages += 1 << order;
> > +	*num_pages += 1 << order;
> >   	ftrace_number_of_groups++;
> >   
> >   	cnt = (PAGE_SIZE << order) / ENTRY_SIZE;
> > @@ -3887,12 +3888,14 @@ static void ftrace_free_pages(struct ftrace_page *pages)
> >   }
> >   
> >   static struct ftrace_page *
> > -ftrace_allocate_pages(unsigned long num_to_init)
> > +ftrace_allocate_pages(unsigned long num_to_init, unsigned long *pages)

Small nit.

Can you rename this to num_pages? That way it is consistent with the above.
Both this function and ftrace_allocate_records() has this as a pointer,
whereas below it's an unsigned long. I rather have the pointer names be
consistent than having "pages" be a pointer here and an unsigned long where
it is called.

Thanks,

-- Steve


> >   {
> >   	struct ftrace_page *start_pg;
> >   	struct ftrace_page *pg;
> >   	int cnt;
> >   
> > +	*pages = 0;
> > +
> >   	if (!num_to_init)
> >   		return NULL;
> >   
> > @@ -3906,7 +3909,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
> >   	 * waste as little space as possible.
> >   	 */
> >   	for (;;) {
> > -		cnt = ftrace_allocate_records(pg, num_to_init);
> > +		cnt = ftrace_allocate_records(pg, num_to_init, pages);
> >   		if (cnt < 0)
> >   			goto free_pages;
> >   
> > @@ -7192,8 +7195,6 @@ static int ftrace_process_locs(struct module *mod,
> >   	if (!count)
> >   		return 0;
> >   
> > -	pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
> > -
> >   	/*
> >   	 * Sorting mcount in vmlinux at build time depend on
> >   	 * CONFIG_BUILDTIME_MCOUNT_SORT, while mcount loc in
> > @@ -7206,7 +7207,7 @@ static int ftrace_process_locs(struct module *mod,
> >   		test_is_sorted(start, count);
> >   	}
> >   
> > -	start_pg = ftrace_allocate_pages(count);
> > +	start_pg = ftrace_allocate_pages(count, &pages);
> >   	if (!start_pg)
> >   		return -ENOMEM;
> >   
> > @@ -7305,27 +7306,27 @@ static int ftrace_process_locs(struct module *mod,
> >   	/* We should have used all pages unless we skipped some */
> >   	if (pg_unuse) {
> >   		unsigned long pg_remaining, remaining = 0;
> > -		unsigned long skip;
> > +		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")) {
> >   
> >   			skip = skipped - pg_remaining;
> >   
> > -			for (pg = pg_unuse; pg; pg = pg->next)
> > +			for (pg = pg_unuse; pg && skip > 0; pg = pg->next) {
> >   				remaining += 1 << pg->order;
> > +				skip -= (PAGE_SIZE << pg->order) / ENTRY_SIZE;
> > +			}
> >   
> >   			pages -= remaining;
> >   
> > -			skip = DIV_ROUND_UP(skip, ENTRIES_PER_PAGE);
> > -
> >   			/*
> >   			 * Check to see if the number of pages remaining would
> >   			 * just fit the number of entries skipped.
> >   			 */
> > -			WARN(skip != remaining, "Extra allocated pages for ftrace: %lu with %lu skipped",
> > +			WARN(pg || skip > 0, "Extra allocated pages for ftrace: %lu with %lu skipped",
> >   			     remaining, skipped);
> >   		}
> >   		/* Need to synchronize with ftrace_location_range() */