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