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;
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:
signature.asc
Description: OpenPGP digital signature
