On 05/08/2024 10:00, David Hildenbrand wrote:
> On 04.08.24 21:02, Usama Arif wrote:
>>
>>
>> On 30/07/2024 16:14, David Hildenbrand wrote:
>>> On 30.07.24 14:46, Usama Arif wrote:
>>>> From: Yu Zhao <yuzhao@xxxxxxxxxx>
>>>>
>>>> If a tail page has only two references left, one inherited from the
>>>> isolation of its head and the other from lru_add_page_tail() which we
>>>> are about to drop, it means this tail page was concurrently zapped.
>>>> Then we can safely free it and save page reclaim or migration the
>>>> trouble of trying it.
>>>>
>>>> Signed-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
>>>> Tested-by: Shuang Zhai <zhais@xxxxxxxxxx>
>>>> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
>>>> ---
>>>> mm/huge_memory.c | 26 ++++++++++++++++++++++++++
>>>> 1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 0167dc27e365..76a3b6a2b796 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2923,6 +2923,8 @@ static void __split_huge_page(struct page *page, struct list_head *list,
>>>> unsigned int new_nr = 1 << new_order;
>>>> int order = folio_order(folio);
>>>> unsigned int nr = 1 << order;
>>>> + LIST_HEAD(pages_to_free);
>>>> + int nr_pages_to_free = 0;
>>>> /* complete memcg works before add pages to LRU */
>>>> split_page_memcg(head, order, new_order);
>>>> @@ -3007,6 +3009,24 @@ static void __split_huge_page(struct page *page, struct list_head *list,
>>>> if (subpage == page)
>>>> continue;
>>>> folio_unlock(new_folio);
>>>> + /*
>>>> + * If a tail page has only two references left, one inherited
>>>> + * from the isolation of its head and the other from
>>>> + * lru_add_page_tail() which we are about to drop, it means this
>>>> + * tail page was concurrently zapped. Then we can safely free it
>>>> + * and save page reclaim or migration the trouble of trying it.
>>>> + */
>>>> + if (list && page_ref_freeze(subpage, 2)) {
>>>> + VM_BUG_ON_PAGE(PageLRU(subpage), subpage);
>>>> + VM_BUG_ON_PAGE(PageCompound(subpage), subpage);
>>>> + VM_BUG_ON_PAGE(page_mapped(subpage), subpage);
>>>> +
>>>
>>> No VM_BUG_*, VM_WARN is good enough.
>>>
>>>> + ClearPageActive(subpage);
>>>> + ClearPageUnevictable(subpage);
>>>> + list_move(&subpage->lru, &pages_to_free);
>>>
>>> Most checks here should operate on new_folio instead of subpage.
>>>
>>>
>> Do you mean instead of doing the PageLRU, PageCompound and page_mapped check on the subpage, there should be checks on new_folio?
>> If new_folio is a large folio, then it could be that only some of the subpages were zapped?
>
> We do a:
>
> struct folio *new_folio = page_folio(subpage);
>
> Then:
>
> PageLRU() will end up getting translated to folio_test_lru(page_folio(subpage))
>
> page_mapped() will end up getting translated to
> folio_mapped(page_folio(subpage))
>
> PageCompound() is essentially a folio_test_large() check.
>
> So what stops us from doing
>
> VM_WARN_ON_ONCE_FOLIO(folio_test_lru(new_folio), new_folio);
> VM_WARN_ON_ONCE_FOLIO(folio_test_large(new_folio), new_folio);
> VM_WARN_ON_ONCE_FOLIO(folio_mapped(new_folio), new_folio);
>
> folio_clear_active(new_folio);
> folio_clear_unevictable(new_folio);
> ...
>
> ?
>
> The page_ref_freeze() should make sure that we don't have a tail page of
> a large folio. Tail pages would have a refcount of 0.
>
> Or what am I missing?
>
Yes you are right. For some reason I was thinking tail pages would be able to reach this path.
