diff mbox series

LoongArch: KVM: always make pte yong in page map's fast path

Message ID 20240613003217.129303-1-jiaqingtong97@gmail.com (mailing list archive)
State New, archived
Headers show
Series LoongArch: KVM: always make pte yong in page map's fast path | expand

Commit Message

Jia Qingtong June 13, 2024, 12:32 a.m. UTC 
From: Jia Qingtong <jiaqingtong97@gmail.com>

It seems redundant to check if pte is yong before the call to
kvm_pte_mkyoung in kvm_map_page_fast.
Just remove the check.

Signed-off-by: Jia Qingtong <jiaqingtong97@gmail.com>
---
 arch/loongarch/kvm/mmu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)


base-commit: eb36e520f4f1b690fd776f15cbac452f82ff7bfa

Comments

Bibo Mao June 13, 2024, 1:27 a.m. UTC | #1 
Hi Qingtong,

On 2024/6/13 上午8:32, jiaqingtong97@gmail.com wrote:
> From: Jia Qingtong <jiaqingtong97@gmail.com>
> 
> It seems redundant to check if pte is yong before the call to
> kvm_pte_mkyoung in kvm_map_page_fast.
> Just remove the check.
> 
> Signed-off-by: Jia Qingtong <jiaqingtong97@gmail.com>
> ---
>   arch/loongarch/kvm/mmu.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 98883aa23ab8..a46befcf85dc 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -551,10 +551,8 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>   	}
>   
>   	/* Track access to pages marked old */
> -	new = *ptep;
> -	if (!kvm_pte_young(new))
> -		new = kvm_pte_mkyoung(new);
> -		/* call kvm_set_pfn_accessed() after unlock */
> +	new = kvm_pte_mkyoung(*ptep);
> +	/* call kvm_set_pfn_accessed() after unlock */
Thanks for noticing this.

The logic is right, however it does not improve performance to update 
pte entry unconditionally since pte entry is frequently accessed by HW 
and multpile CPUs. There may be cache thrashing issue, just maybe from 
my own point -:)

It is the same such as test_and_set_bit implementation on x86 or other 
architectures, it is not unconditionally logic or on x86.

Regards
Bibo Mao
>   
>   	if (write && !kvm_pte_dirty(new)) {
>   		if (!kvm_pte_write(new)) {
> 
> base-commit: eb36e520f4f1b690fd776f15cbac452f82ff7bfa
>
Bibo Mao June 13, 2024, 1:45 a.m. UTC | #2 
On 2024/6/13 上午8:32, jiaqingtong97@gmail.com wrote:
> From: Jia Qingtong <jiaqingtong97@gmail.com>
> 
> It seems redundant to check if pte is yong before the call to
> kvm_pte_mkyoung in kvm_map_page_fast.
> Just remove the check.
> 
> Signed-off-by: Jia Qingtong <jiaqingtong97@gmail.com>
> ---
>   arch/loongarch/kvm/mmu.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 98883aa23ab8..a46befcf85dc 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -551,10 +551,8 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>   	}
>   
>   	/* Track access to pages marked old */
> -	new = *ptep;
> -	if (!kvm_pte_young(new))
> -		new = kvm_pte_mkyoung(new);
> -		/* call kvm_set_pfn_accessed() after unlock */
> +	new = kvm_pte_mkyoung(*ptep);
> +	/* call kvm_set_pfn_accessed() after unlock */
Sorry, please ignore my previous comments.
It is to modify local variable, rather than update pte entry.

Reviewed-by: Bibo Mao <maobibo@loongson.cn>

>   
>   	if (write && !kvm_pte_dirty(new)) {
>   		if (!kvm_pte_write(new)) {
> 
> base-commit: eb36e520f4f1b690fd776f15cbac452f82ff7bfa
>
Huacai Chen July 5, 2024, 9:39 a.m. UTC | #3 
Applied, thanks.

Huacai

On Thu, Jun 13, 2024 at 9:45 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/6/13 上午8:32, jiaqingtong97@gmail.com wrote:
> > From: Jia Qingtong <jiaqingtong97@gmail.com>
> >
> > It seems redundant to check if pte is yong before the call to
> > kvm_pte_mkyoung in kvm_map_page_fast.
> > Just remove the check.
> >
> > Signed-off-by: Jia Qingtong <jiaqingtong97@gmail.com>
> > ---
> >   arch/loongarch/kvm/mmu.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> > index 98883aa23ab8..a46befcf85dc 100644
> > --- a/arch/loongarch/kvm/mmu.c
> > +++ b/arch/loongarch/kvm/mmu.c
> > @@ -551,10 +551,8 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
> >       }
> >
> >       /* Track access to pages marked old */
> > -     new = *ptep;
> > -     if (!kvm_pte_young(new))
> > -             new = kvm_pte_mkyoung(new);
> > -             /* call kvm_set_pfn_accessed() after unlock */
> > +     new = kvm_pte_mkyoung(*ptep);
> > +     /* call kvm_set_pfn_accessed() after unlock */
> Sorry, please ignore my previous comments.
> It is to modify local variable, rather than update pte entry.
>
> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
>
> >
> >       if (write && !kvm_pte_dirty(new)) {
> >               if (!kvm_pte_write(new)) {
> >
> > base-commit: eb36e520f4f1b690fd776f15cbac452f82ff7bfa
> >
>
diff mbox series

Patch

diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 98883aa23ab8..a46befcf85dc 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -551,10 +551,8 @@  static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
 	}
 
 	/* Track access to pages marked old */
-	new = *ptep;
-	if (!kvm_pte_young(new))
-		new = kvm_pte_mkyoung(new);
-		/* call kvm_set_pfn_accessed() after unlock */
+	new = kvm_pte_mkyoung(*ptep);
+	/* call kvm_set_pfn_accessed() after unlock */
 
 	if (write && !kvm_pte_dirty(new)) {
 		if (!kvm_pte_write(new)) {