v1: https://lore.kernel.org/lkml/[email protected]/
v2: https://lore.kernel.org/linux-mm/[email protected]/
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
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 <[email protected]>
Signed-off-by: Qi Zheng <[email protected]>
---
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
Hi, all,
Should this patch go via mm tree or loongarch tree? If via mm tree, then
Acked-by: Huacai Chen <[email protected]>
On Thu, Sep 29, 2022 at 7:23 PM Qi Zheng <[email protected]> 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 <[email protected]>
> Signed-off-by: Qi Zheng <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
Reviewed-by: Muchun Song <[email protected]>
---
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
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 <[email protected]>
Thanks. :)
>
> On Thu, Sep 29, 2022 at 7:23 PM Qi Zheng <[email protected]> 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 <[email protected]>
>> Signed-off-by: Qi Zheng <[email protected]>
>> ---
>> 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
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 <[email protected]>
> Signed-off-by: Qi Zheng <[email protected]>
> Reviewed-by: Muchun Song <[email protected]>
> ---
> 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
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
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 <[email protected]>
Thanks.
>
>> Signed-off-by: Qi Zheng <[email protected]>
>> Reviewed-by: Muchun Song <[email protected]>
>> ---
>> 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
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
On Fri, 30 Sep 2022 10:44:21 +0200 David Hildenbrand <[email protected]> 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 <[email protected]>
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/[email protected]
Signed-off-by: Qi Zheng <[email protected]>
Reviewed-by: Muchun Song <[email protected]>
Acked-by: David Hildenbrand <[email protected]>
Cc: Bibo Mao <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: Max Filippov <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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;
}
_
On 2022/10/1 06:31, Andrew Morton wrote:
> On Fri, 30 Sep 2022 10:44:21 +0200 David Hildenbrand <[email protected]> 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 <[email protected]>
> 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/[email protected]
> Signed-off-by: Qi Zheng <[email protected]>
> Reviewed-by: Muchun Song <[email protected]>
> Acked-by: David Hildenbrand <[email protected]>
> Cc: Bibo Mao <[email protected]>
> Cc: Chris Zankel <[email protected]>
> Cc: Huacai Chen <[email protected]>
> Cc: Max Filippov <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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