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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9 Aug 2024, at 3:52, Huang, Ying wrote:

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

Thanks. Will put a fixup below.

>
>        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,
>        ...
>

I will send a separate patch for this refactoring, since this patch
is meant for fixing commit b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault”).


Hi Andrew,

Can you fold the fixup below to this patch? Thanks.


From 645fa83b2eed14494755ed67e5c52a55656287ac Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@xxxxxxxxxx>
Date: Thu, 8 Aug 2024 22:06:41 -0400
Subject: [PATCH] fixup! fixup! mm/numa: no task_numa_fault() call if page
 table is changed

Based on suggestion from Ying.

Link: https://lore.kernel.org/linux-mm/87cymizdvc.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
---
 mm/huge_memory.c | 5 +++--
 mm/memory.c      | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4e4364a17e6d..0e79ccaaf5e4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1724,7 +1724,8 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		flags |= TNF_MIGRATED;
 		nid = target_nid;
-		goto out;
+		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
+		return 0;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
@@ -1743,7 +1744,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-out:
+
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
 	return 0;
diff --git a/mm/memory.c b/mm/memory.c
index d9b1dff9dc57..6c3aadf3e5ad 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5523,7 +5523,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,
@@ -5547,7 +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;
-- 
2.43.0




Best Regards,
Yan, Zi

Attachment: signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux