Re: [PATCH] mm/codetag: sub in advance when free non-compound high order pages

Vlastimil Babka
2025-05-05T14:55:38Z

On 5/5/25 16:31, David Wang wrote:
>
>
> At 2025-05-05 21:12:55, "Vlastimil Babka" <vbabka@suse.cz> wrote:
>>On 5/4/25 08:19, David Wang wrote:
>>> When page is non-compound, page[0] could be released by other
>>> thread right after put_page_testzero failed in current thread,
>>> pgalloc_tag_sub_pages afterwards would manipulate an invalid
>>> page for accounting remaining pages:
>>>
>>> [timeline] [thread1] [thread2]
>>> | alloc_page non-compound
>>> V
>>> | get_page, rf counter inc
>>> V
>>> | in ___free_pages
>>> | put_page_testzero fails
>>> V
>>> | put_page, page released
>>> V
>>> | in ___free_pages,
>>> | pgalloc_tag_sub_pages
>>> | manipulate an invalid page
>>> V
>>> V
>>>
>>> Move the tag page accounting ahead, and only account remaining pages
>>> for non-compound pages with non-zero order.
>>>
>>> Signed-off-by: David Wang <00107082@163.com>
>>
>>Hmm, I think the problem was introduced by 51ff4d7486f0 ("mm: avoid extra
>>mem_alloc_profiling_enabled() checks"). Previously we'd get the tag pointer
>>upfront and avoid the page use-after-free.
>
>
> Oh, you're right. I forgot to check history......
>
>
>>
>>It would likely be nicer to fix it by going back to that approach for
>>___free_pages(), while hopefully keeping the optimisations of 51ff4d7486f0
>>for the other call sites where it applies?
>
> After checking that commit, I kind of feels the changes in __free_pages are
> the major optimization of the commit....

We could have both pgalloc_tag_get() to use in __free_page() as before
51ff4d7486f0, and keep __pgalloc_tag_get() to use in pgalloc_tag_split() and
pgalloc_tag_swap().

I think __free_page() didn't benefit from the stated purpose of "avoiding
mem_alloc_profiling_enabled() ... which is often called after that check was
already done"

> What about revert that commit and make optimization by condition checks,
> similar to what this patch did?

The downside of the condition checks is they make the code more complex and
might actually increase overhead when mem_alloc_profiling_enabled() is
false, as those checks add non-static branches outside of the static branch
that's mem_alloc_profiling_enabled().

I think __free_pages() before 51ff4d7486f0 was quite ok.

- pgalloc_tag_get() is done unconditionally, but its code is all inside the
mem_alloc_profiling_enabled() static branch so that's a no-op when profiling
is not enabled

- pgalloc_tag_sub_pages() is also all behind the static branch inside. Also
it's a very rare path anyway, most freeing should go through the
put_page_testzero() being true.

> David
>