Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 31 Mar 2021 10:45:01 -0700 | Subject | Re: [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
| |