Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan <ziy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 7041 bytes --]

On 8 Aug 2024, at 21:25, Huang, Ying wrote:

> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>
>> On 2024/8/8 22:21, Zi Yan wrote:
>>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>>
>>>> On 08.08.24 16:13, Zi Yan wrote:
>>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>>
>>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>>
>>>>>>>
>> ...
>>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>>
>>>>>> Then, just copy the 3 LOC.
>>>>>>
>>>>>> For mm/memory.c that would be:
>>>>>>
>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>>> --- a/mm/memory.c
>>>>>> +++ b/mm/memory.c
>>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -               goto out;
>>>>>> +               return 0;
>>>>>>           }
>>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>>                   if (unlikely(!vmf->pte))
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   }
>>>>>>                   goto out_map;
>>>>>>           }
>>>>>>    -out:
>>>>>>           if (nid != NUMA_NO_NODE)
>>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>>           return 0;
>>
>> Maybe drop this part too,
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 410ba50ca746..07343c1469e0 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>                 nid = target_nid;
>>                 flags |= TNF_MIGRATED;
>> +               goto out;
>>         } else {
>>                 flags |= TNF_MIGRATE_FAIL;
>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>                         return 0;
>>                 }
>> -               goto out_map;
>>         }
>>
>> -       if (nid != NUMA_NO_NODE)
>> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>> -       return 0;
>>  out_map:
>>         /*
>>          * Make it present again, depending on how arch implements
>
> IMHO, migration success is normal path, while migration failure is error
> processing path.  If so, it's better to use "goto" for error processing
> instead of normal path.
>
>> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address,
>>                 vmf->pte,
>>                                             writable);
>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>> +out:
>>         if (nid != NUMA_NO_NODE)
>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>         return 0;
>>
>>

How about calling task_numa_fault and return in the migration successful path?
(target_nid cannot be NUMA_NO_NODE, so if is not needed)

diff --git a/mm/memory.c b/mm/memory.c
index 3441f60d54ef..abdb73a68b80 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5526,7 +5526,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        if (!migrate_misplaced_folio(folio, vma, target_nid)) {
                nid = target_nid;
                flags |= TNF_MIGRATED;
-               goto out;
+               task_numa_fault(last_cpupid, nid, nr_pages, flags);
+               return 0;
        } else {
                flags |= TNF_MIGRATE_FAIL;
                vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5550,7 +5551,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
                                            writable);
        pte_unmap_unlock(vmf->pte, vmf->ptl);
-out:
        if (nid != NUMA_NO_NODE)
                task_numa_fault(last_cpupid, nid, nr_pages, flags);
        return 0;




Or move the make-present code inside migration failed branch? This one
does not duplicate code but others can jump into this branch.

diff --git a/mm/memory.c b/mm/memory.c
index 3441f60d54ef..c9b4e7099815 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5526,7 +5526,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        if (!migrate_misplaced_folio(folio, vma, target_nid)) {
                nid = target_nid;
                flags |= TNF_MIGRATED;
-               goto out;
        } else {
                flags |= TNF_MIGRATE_FAIL;
                vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5537,20 +5536,20 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                        pte_unmap_unlock(vmf->pte, vmf->ptl);
                        return 0;
                }
-       }
 out_map:
-       /*
-        * Make it present again, depending on how arch implements
-        * non-accessible ptes, some can allow access by kernel mode.
-        */
-       if (folio && folio_test_large(folio))
-               numa_rebuild_large_mapping(vmf, vma, folio, pte, ignore_writable,
-                                          pte_write_upgrade);
-       else
-               numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
-                                           writable);
-       pte_unmap_unlock(vmf->pte, vmf->ptl);
-out:
+               /*
+                * Make it present again, depending on how arch implements
+                * non-accessible ptes, some can allow access by kernel mode.
+                */
+               if (folio && folio_test_large(folio))
+                       numa_rebuild_large_mapping(vmf, vma, folio, pte,
+                                       ignore_writable, pte_write_upgrade);
+               else
+                       numa_rebuild_single_mapping(vmf, vma, vmf->address,
+                                       vmf->pte, writable);
+               pte_unmap_unlock(vmf->pte, vmf->ptl);
+       }
+
        if (nid != NUMA_NO_NODE)
                task_numa_fault(last_cpupid, nid, nr_pages, flags);
        return 0;


Of course, I will need to change mm/huge_memory.c as well.

Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]