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

["Huang, Ying" <ying.huang@intel.com>]

Zi Yan <ziy@nvidia.com> writes:

> 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;
>

This looks better for me, or change it further.

       if (migrate_misplaced_folio(folio, vma, target_nid))
               goto out_map_pt;

       nid = target_nid;
       flags |= TNF_MIGRATED;
       task_numa_fault(last_cpupid, nid, nr_pages, flags);

       return 0;

out_map_pt:
       flags |= TNF_MIGRATE_FAIL;
       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
       ...
        
>
>
> 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,
Huang, Ying