Zi Yan <ziy@xxxxxxxxxx> writes:
> On 8 Aug 2024, at 21:25, Huang, Ying wrote:
>
>> Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> 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
