Re: [GIT PULL] ftrace: Check if pages were allocated before calling free_pages()

[Linus Torvalds]

On Wed, Mar 31, 2021 at 10:45 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> 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!!

Attachment: patch
Description: Binary data