lkml.org 
[lkml]   [2021]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT PULL] ftrace: Check if pages were allocated before calling free_pages()
On Wed, Mar 31, 2021 at 6:27 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> order = get_count_order(pg->size / ENTRIES_PER_PAGE);
> - free_pages((unsigned long)pg->records, order);
> + if (order >= 0)
> + free_pages((unsigned long)pg->records, order);

Honestly, looking at that code, every single use of
"get_count_order()" seems really really confusing.

And really confused.

The comments are garbage, the code is odd, it's just all very very strange.

See here in ftrace_allocate_records():

pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
order = get_count_order(pages);

/*
* We want to fill as much as possible. No more than a page
* may be empty.
*/
if (!is_power_of_2(pages))
order--;

can you actually explain what the logic is here?

The 'get_count_order()' function will return the smallest order that
the value fits in. But then if the value wasn't a power of two, you
subtract 1. If I understand the code correctly, that's just the same
as "what is the highest bit set", isn't it?

So afaik, what you *actually* want is just

pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
order = fls(pages)-1;

isn't it?

Did I misunderstand what the code wants to do? The "No more than a
page may be empty" seems wrong - you really mean "no empty pages".

I dunno. Maybe I'm wrong, but that code is really confusing (which is
why I may be wrong). It doesn't seem to make any sense at all to use
"get_count_order()" and then modify the end result.

Linus

\
 
 \ /
  Last update: 2021-03-31 19:47    [W:0.068 / U:0.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site