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

[Vlastimil Babka <vbabka@suse.cz>]

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
>