[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9162275d-12af-45d4-a004-adde8e4d63c2@gmail.com>
Date: Tue, 6 Aug 2024 10:58:45 +0100
From: Usama Arif <usamaarif642@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: hannes@...xchg.org, riel@...riel.com, shakeel.butt@...ux.dev,
roman.gushchin@...ux.dev, yuzhao@...gle.com, baohua@...nel.org,
ryan.roberts@....com, rppt@...nel.org, willy@...radead.org,
cerasuolodomenico@...il.com, corbet@....net, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, kernel-team@...a.com,
Shuang Zhai <zhais@...gle.com>, akpm@...ux-foundation.org, linux-mm@...ck.org
Subject: Re: [PATCH 3/6] mm: free zapped tail pages when splitting isolated
thp
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@...gle.com>
>>>>
>>>> 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@...gle.com>
>>>> Tested-by: Shuang Zhai <zhais@...gle.com>
>>>> Signed-off-by: Usama Arif <usamaarif642@...il.com>
>>>> ---
>>>> 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.
Powered by blists - more mailing lists