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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87cymizdvc.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Fri, 09 Aug 2024 09:25:27 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Kefeng Wang <wangkefeng.wang@...wei.com>
Cc: Zi Yan <ziy@...dia.com>,  David Hildenbrand <david@...hat.com>,  "Andrew
 Morton" <akpm@...ux-foundation.org>,  Baolin Wang
 <baolin.wang@...ux.alibaba.com>,  <linux-mm@...ck.org>,
  <linux-kernel@...r.kernel.org>,  <stable@...r.kernel.org>
Subject: Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is
 changed

Kefeng Wang <wangkefeng.wang@...wei.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;
>
>

--
Best Regards,
Huang, Ying