[PATCH v3 0/2] use update_mmu_tlb() on the second thread

[PATCH v3 0/2] use update_mmu_tlb() on the second thread

[Qi Zheng]

v1: https://lore.kernel.org/lkml/20220924053239.91661-1-zhengqi.arch@bytedance.com/
v2: https://lore.kernel.org/linux-mm/20220926115621.13849-1-zhengqi.arch@bytedance.com/

Changelog in v2 -> v3:
 - implement update_mmu_tlb() for LoongArch (suggested by Bibo)

Changelog in v1 -> v2:
 - change the subject and commit message (David)

Qi Zheng (2):
  mm: use update_mmu_tlb() on the second thread
  LoongArch: update local TLB if PTE entry exists

 arch/loongarch/include/asm/pgtable.h | 3 +++
 mm/memory.c                          | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[Qi Zheng]

As message in commit 7df676974359 ("mm/memory.c: Update local TLB
if PTE entry exists") said, we should update local TLB only on the
second thread. So in the do_anonymous_page() here, we should use
update_mmu_tlb() instead of update_mmu_cache() on the second thread.

Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index 118e5f023597..9e11c783ba0e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4122,7 +4122,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
 			&vmf->ptl);
 	if (!pte_none(*vmf->pte)) {
-		update_mmu_cache(vma, vmf->address, vmf->pte);
+		update_mmu_tlb(vma, vmf->address, vmf->pte);
 		goto release;
 	}
 
-- 
2.20.1

[PATCH v3 2/2] LoongArch: update local TLB if PTE entry exists

[Qi Zheng]

Currently, the implementation of update_mmu_tlb() is empty if
__HAVE_ARCH_UPDATE_MMU_TLB is not defined. Then if two threads
concurrently fault at the same page, the second thread that did
not win the race will give up and do nothing. In the LoongArch
architecture, this second thread will trigger another fault,
and only updates its local TLB.

Instead of triggering another fault, it's better to implement
update_mmu_tlb() to directly update the local TLB of the second
thread. Just do it.

Suggested-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
---
 arch/loongarch/include/asm/pgtable.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
index 8ea57e2f0e04..946704bee599 100644
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -412,6 +412,9 @@ static inline void update_mmu_cache(struct vm_area_struct *vma,
 	__update_tlb(vma, address, ptep);
 }
 
+#define __HAVE_ARCH_UPDATE_MMU_TLB
+#define update_mmu_tlb	update_mmu_cache
+
 static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
 			unsigned long address, pmd_t *pmdp)
 {
-- 
2.20.1

Re: [PATCH v3 2/2] LoongArch: update local TLB if PTE entry exists

[Huacai Chen]

Hi, all,

Should this patch go via mm tree or loongarch tree? If via mm tree, then

Acked-by: Huacai Chen <chenhuacai@loongson.cn>

On Thu, Sep 29, 2022 at 7:23 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>
> Currently, the implementation of update_mmu_tlb() is empty if
> __HAVE_ARCH_UPDATE_MMU_TLB is not defined. Then if two threads
> concurrently fault at the same page, the second thread that did
> not win the race will give up and do nothing. In the LoongArch
> architecture, this second thread will trigger another fault,
> and only updates its local TLB.
>
> Instead of triggering another fault, it's better to implement
> update_mmu_tlb() to directly update the local TLB of the second
> thread. Just do it.
>
> Suggested-by: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> ---
>  arch/loongarch/include/asm/pgtable.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> index 8ea57e2f0e04..946704bee599 100644
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -412,6 +412,9 @@ static inline void update_mmu_cache(struct vm_area_struct *vma,
>         __update_tlb(vma, address, ptep);
>  }
>
> +#define __HAVE_ARCH_UPDATE_MMU_TLB
> +#define update_mmu_tlb update_mmu_cache
> +
>  static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
>                         unsigned long address, pmd_t *pmdp)
>  {
> --
> 2.20.1
>
>

Re: [PATCH v3 2/2] LoongArch: update local TLB if PTE entry exists

[Qi Zheng]

On 2022/9/29 19:42, Huacai Chen wrote:
> Hi, all,
> 
> Should this patch go via mm tree or loongarch tree? If via mm tree, then

Both are fine for me. Hi Andrew, can you help to apply this patch
series?

> 
> Acked-by: Huacai Chen <chenhuacai@loongson.cn>

Thanks. :)

> 
> On Thu, Sep 29, 2022 at 7:23 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>>
>> Currently, the implementation of update_mmu_tlb() is empty if
>> __HAVE_ARCH_UPDATE_MMU_TLB is not defined. Then if two threads
>> concurrently fault at the same page, the second thread that did
>> not win the race will give up and do nothing. In the LoongArch
>> architecture, this second thread will trigger another fault,
>> and only updates its local TLB.
>>
>> Instead of triggering another fault, it's better to implement
>> update_mmu_tlb() to directly update the local TLB of the second
>> thread. Just do it.
>>
>> Suggested-by: Bibo Mao <maobibo@loongson.cn>
>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>> ---
>>   arch/loongarch/include/asm/pgtable.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
>> index 8ea57e2f0e04..946704bee599 100644
>> --- a/arch/loongarch/include/asm/pgtable.h
>> +++ b/arch/loongarch/include/asm/pgtable.h
>> @@ -412,6 +412,9 @@ static inline void update_mmu_cache(struct vm_area_struct *vma,
>>          __update_tlb(vma, address, ptep);
>>   }
>>
>> +#define __HAVE_ARCH_UPDATE_MMU_TLB
>> +#define update_mmu_tlb update_mmu_cache
>> +
>>   static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
>>                          unsigned long address, pmd_t *pmdp)
>>   {
>> --
>> 2.20.1
>>
>>

-- 
Thanks,
Qi

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[David Hildenbrand]

On 29.09.22 13:23, Qi Zheng wrote:
> As message in commit 7df676974359 ("mm/memory.c: Update local TLB
> if PTE entry exists") said, we should update local TLB only on the
> second thread. So in the do_anonymous_page() here, we should use
> update_mmu_tlb() instead of update_mmu_cache() on the second thread.
> 

Maybe mention here "This only affects performance, but not correctness."

Acked-by: David Hildenbrand <david@redhat.com>

> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> ---
>   mm/memory.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 118e5f023597..9e11c783ba0e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4122,7 +4122,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>   	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>   			&vmf->ptl);
>   	if (!pte_none(*vmf->pte)) {
> -		update_mmu_cache(vma, vmf->address, vmf->pte);
> +		update_mmu_tlb(vma, vmf->address, vmf->pte);
>   		goto release;
>   	}
>   

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[Qi Zheng]

On 2022/9/30 16:30, David Hildenbrand wrote:
> On 29.09.22 13:23, Qi Zheng wrote:
>> As message in commit 7df676974359 ("mm/memory.c: Update local TLB
>> if PTE entry exists") said, we should update local TLB only on the
>> second thread. So in the do_anonymous_page() here, we should use
>> update_mmu_tlb() instead of update_mmu_cache() on the second thread.
>>
> 
> Maybe mention here "This only affects performance, but not correctness."

Oh, this is better. Hi Andrew, do I need to resend the v4?

> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks.

> 
>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>   mm/memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 118e5f023597..9e11c783ba0e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4122,7 +4122,7 @@ static vm_fault_t do_anonymous_page(struct 
>> vm_fault *vmf)
>>       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>>               &vmf->ptl);
>>       if (!pte_none(*vmf->pte)) {
>> -        update_mmu_cache(vma, vmf->address, vmf->pte);
>> +        update_mmu_tlb(vma, vmf->address, vmf->pte);
>>           goto release;
>>       }
> 

-- 
Thanks,
Qi

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[David Hildenbrand]

On 30.09.22 10:43, Qi Zheng wrote:
> 
> 
> On 2022/9/30 16:30, David Hildenbrand wrote:
>> On 29.09.22 13:23, Qi Zheng wrote:
>>> As message in commit 7df676974359 ("mm/memory.c: Update local TLB
>>> if PTE entry exists") said, we should update local TLB only on the
>>> second thread. So in the do_anonymous_page() here, we should use
>>> update_mmu_tlb() instead of update_mmu_cache() on the second thread.
>>>
>>
>> Maybe mention here "This only affects performance, but not correctness."
> 
> Oh, this is better. Hi Andrew, do I need to resend the v4?
> 

I assume he can squash it, most probably no need to resend. :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[Qi Zheng]

On 2022/9/30 16:44, David Hildenbrand wrote:
> On 30.09.22 10:43, Qi Zheng wrote:
>>
>>
>> On 2022/9/30 16:30, David Hildenbrand wrote:
>>> On 29.09.22 13:23, Qi Zheng wrote:
>>>> As message in commit 7df676974359 ("mm/memory.c: Update local TLB
>>>> if PTE entry exists") said, we should update local TLB only on the
>>>> second thread. So in the do_anonymous_page() here, we should use
>>>> update_mmu_tlb() instead of update_mmu_cache() on the second thread.
>>>>
>>>
>>> Maybe mention here "This only affects performance, but not correctness."
>>
>> Oh, this is better. Hi Andrew, do I need to resend the v4?
>>
> 
> I assume he can squash it, most probably no need to resend. :)

Got it. Both are fine for me. :)

> 

-- 
Thanks,
Qi

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[Andrew Morton]

On Fri, 30 Sep 2022 10:44:21 +0200 David Hildenbrand <david@redhat.com> wrote:

> > Oh, this is better. Hi Andrew, do I need to resend the v4?
> > 
> 
> I assume he can squash it, most probably no need to resend. :)


From: Qi Zheng <zhengqi.arch@bytedance.com>
Subject: mm: use update_mmu_tlb() on the second thread
Date: Thu, 29 Sep 2022 19:23:17 +0800

As message in commit 7df676974359 ("mm/memory.c: Update local TLB if PTE
entry exists") said, we should update local TLB only on the second thread.
So in the do_anonymous_page() here, we should use update_mmu_tlb()
instead of update_mmu_cache() on the second thread.

As David pointed out, this is a performance improvement, not a
correctness fix.

Link: https://lkml.kernel.org/r/20220929112318.32393-2-zhengqi.arch@bytedance.com
Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: David Hildenbrand <david@redhat.com>
Cc: Bibo Mao <maobibo@loongson.cn>
Cc: Chris Zankel <chris@zankel.net>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memory.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/mm/memory.c~mm-use-update_mmu_tlb-on-the-second-thread
+++ a/mm/memory.c
@@ -4136,7 +4136,7 @@ static vm_fault_t do_anonymous_page(stru
 	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
 			&vmf->ptl);
 	if (!pte_none(*vmf->pte)) {
-		update_mmu_cache(vma, vmf->address, vmf->pte);
+		update_mmu_tlb(vma, vmf->address, vmf->pte);
 		goto release;
 	}
 
_

Re: [PATCH v3 1/2] mm: use update_mmu_tlb() on the second thread

[Qi Zheng]

On 2022/10/1 06:31, Andrew Morton wrote:
> On Fri, 30 Sep 2022 10:44:21 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>>> Oh, this is better. Hi Andrew, do I need to resend the v4?
>>>
>>
>> I assume he can squash it, most probably no need to resend. :)
> 
> 
> From: Qi Zheng <zhengqi.arch@bytedance.com>
> Subject: mm: use update_mmu_tlb() on the second thread
> Date: Thu, 29 Sep 2022 19:23:17 +0800
> 
> As message in commit 7df676974359 ("mm/memory.c: Update local TLB if PTE
> entry exists") said, we should update local TLB only on the second thread.
> So in the do_anonymous_page() here, we should use update_mmu_tlb()
> instead of update_mmu_cache() on the second thread.
> 
> As David pointed out, this is a performance improvement, not a
> correctness fix.
> 
> Link: https://lkml.kernel.org/r/20220929112318.32393-2-zhengqi.arch@bytedance.com
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Cc: Bibo Mao <maobibo@loongson.cn>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Huacai Chen <chenhuacai@loongson.cn>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>   mm/memory.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/mm/memory.c~mm-use-update_mmu_tlb-on-the-second-thread
> +++ a/mm/memory.c
> @@ -4136,7 +4136,7 @@ static vm_fault_t do_anonymous_page(stru
>   	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>   			&vmf->ptl);
>   	if (!pte_none(*vmf->pte)) {
> -		update_mmu_cache(vma, vmf->address, vmf->pte);
> +		update_mmu_tlb(vma, vmf->address, vmf->pte);
>   		goto release;
>   	}
>   
> _

Thank you very much! :)

> 

-- 
Thanks,
Qi