Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 31 Mar 2021 11:03:21 -0700 | Subject | Re: [GIT PULL] ftrace: Check if pages were allocated before calling free_pages() |
| |
On Wed, Mar 31, 2021 at 10:45 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Honestly, looking at that code, every single use of > "get_count_order()" seems really really confusing.
Side note: I've pulled your fix, but I really think that the bug is almost entirely due to the code being so opaque and crazy hard to read. I think the real fix would be to make the code a lot clearer.
I found another bug in there, for example:
ftrace_number_of_pages -= 1 << order;
is also wrong if order is negative.
So I think the fix is something like the attached (TOTALLY UNTESTED!!) that does two things:
- make the "has it been allocated" test be about "pg->records" being non-NULL, which honestly seems to be a hell of a lot more logical.
- get rid of the crazy "size" field that then has to be converted into a "page order", and just keep it being the page order
- that makes all the calculations much simpler: the size of an allocation is (trivially) just
PAGE_SIZE << pg->order
and the test whether the allocation has filled up is to just see how much space we need:
end_offset = (pg->index+1) * sizeof(pg->records[0]);
and then you can compare that against that trivial allocation size.
Doesn't this make the code now make SENSE? Instead of that incomprehensible mess it was before?
I dunno. Maybe it's just my "pee in the snow" thing, but honestly, the fact that I seem to have found another bug wrt the whole 'ftrace_number_of_pages' handling really says that the code was garbage.
And maybe it's just me who doesn't understand the subtle perfection of the old code, and I'm being stupid. Feel free to educate me about it.
Final note: note the "TOTALLY UNTESTED" part of the patch. The patch CompilesForMe(tm), but please consider it a "how about something like this" rather than anything finished.
Also note that I did *not* change the initial "order" calculation itself in ftrace_allocate_records() in this patch. I left that particular oddity alone. Again, I *think* the math just ends up being
pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE); order = fls(pages)-1;
but the attached patch is not about that, it's about the crazy "pg->size" games.
Linus
PS. TOTALLY UNTESTED!! [unhandled content-type:application/octet-stream] | |