Re: [RFC PATCH 05/12] khugepaged: Generalize __collapse_huge_page_isolate()

[Ryan Roberts]

On 17/12/2024 06:41, Dev Jain wrote:
> 
> On 17/12/24 10:02 am, Matthew Wilcox wrote:
>> On Mon, Dec 16, 2024 at 10:20:58PM +0530, Dev Jain wrote:
>>>   {
>>> -    struct page *page = NULL;
>>> -    struct folio *folio = NULL;
>>> -    pte_t *_pte;
>>> +    unsigned int max_ptes_shared = khugepaged_max_ptes_shared >>
>>> (HPAGE_PMD_ORDER - order);
>>> +    unsigned int max_ptes_none = khugepaged_max_ptes_none >>
>>> (HPAGE_PMD_ORDER - order);
>>>       int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
>>> +    struct folio *folio = NULL;
>>> +    struct page *page = NULL;
>> why are you moving variables around unnecessarily?
> 
> In a previous work, I moved code around and David noted to arrange the declarations
> in reverse Xmas tree order. I guess (?) that was not spoiling git history, so if
> this feels like that, I will revert.
> 
>>
>>>       bool writable = false;
>>> +    pte_t *_pte;
>>>   -    for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
>>> +
>>> +    for (_pte = pte; _pte < pte + (1UL << order);
>> spurious blank line
> 
> My bad
> 
>>
>>
>> also you might first want to finish off the page->folio conversion in
>> this function first; we have a vm_normal_folio() now.
> 
> I did not add any code before we derive the folio...I'm sorry, I don't get what
> you mean...
> 

I think Matthew is suggesting helping out with the folio to page conversion work
while you are working on this function. I think it would amount to a patch that
does something like this:

----8<-----
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index ffc4d5aef991..d94e05754140 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -568,7 +568,6 @@ static int __collapse_huge_page_isolate(struct
vm_area_struct *vma,
        unsigned int max_ptes_none = khugepaged_max_ptes_none >>
(HPAGE_PMD_ORDER - order);
        int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
        struct folio *folio = NULL;
-       struct page *page = NULL;
        bool writable = false;
        pte_t *_pte;

@@ -597,13 +596,12 @@ static int __collapse_huge_page_isolate(struct
vm_area_struct *vma,
                        result = SCAN_PTE_UFFD_WP;
                        goto out;
                }
-               page = vm_normal_page(vma, address, pteval);
-               if (unlikely(!page) || unlikely(is_zone_device_page(page))) {
+               folio = vm_normal_folio(vma, address, pteval);
+               if (unlikely(!folio) || unlikely(folio_is_zone_device(folio))) {
                        result = SCAN_PAGE_NULL;
                        goto out;
                }

-               folio = page_folio(page);
                VM_BUG_ON_FOLIO(!folio_test_anon(folio), folio);

                if (order !=HPAGE_PMD_ORDER && folio_order(folio) >= order) {
----8<-----