[PATCH] KVM: x86: move the can_unsync and prefetch checks outside of the loop

[PATCH] KVM: x86: move the can_unsync and prefetch checks outside of the loop

[Vihas Mak]

mmu_try_to_unsync_pages() performs !can_unsync check before attempting
to unsync any shadow pages.
This check is peformed inside the loop right now. 
It's redundant to perform it every iteration if can_unsync is true, as
can_unsync parameter isn't getting updated inside the loop.
Move the check outside of the loop.

Same is the case with prefetch.

Signed-off-by: Vihas Mak <makvihas@gmail.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/mmu/mmu.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1d275e9d7..53f4b8b07 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2586,6 +2586,11 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
 	if (kvm_slot_page_track_is_active(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE))
 		return -EPERM;
 
+	if (!can_unsync)
+		return -EPERM;
+
+	if (prefetch)
+		return -EEXIST;
 	/*
 	 * The page is not write-tracked, mark existing shadow pages unsync
 	 * unless KVM is synchronizing an unsync SP (can_unsync = false).  In
@@ -2593,15 +2598,9 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
 	 * allowing shadow pages to become unsync (writable by the guest).
 	 */
 	for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
-		if (!can_unsync)
-			return -EPERM;
-
 		if (sp->unsync)
 			continue;
 
-		if (prefetch)
-			return -EEXIST;
-
 		/*
 		 * TDP MMU page faults require an additional spinlock as they
 		 * run with mmu_lock held for read, not write, and the unsync
-- 
2.30.2

Re: [PATCH] KVM: x86: move the can_unsync and prefetch checks outside of the loop

[Paolo Bonzini]

On 1/7/22 09:25, Vihas Mak wrote:
> mmu_try_to_unsync_pages() performs !can_unsync check before attempting
> to unsync any shadow pages.
> This check is peformed inside the loop right now.
> It's redundant to perform it every iteration if can_unsync is true, as
> can_unsync parameter isn't getting updated inside the loop.
> Move the check outside of the loop.
> 
> Same is the case with prefetch.

The meaning changes if the loop does not execute at all.  Is this safe?

Paolo

> Signed-off-by: Vihas Mak<makvihas@gmail.com>
> Cc: Sean Christopherson<seanjc@google.com>
> Cc: Vitaly Kuznetsov<vkuznets@redhat.com>
> Cc: Wanpeng Li<wanpengli@tencent.com>
> Cc: Jim Mattson<jmattson@google.com>
> Cc: Joerg Roedel<joro@8bytes.org>
> ---
>   arch/x86/kvm/mmu/mmu.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 1d275e9d7..53f4b8b07 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2586,6 +2586,11 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
>   	if (kvm_slot_page_track_is_active(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE))
>   		return -EPERM;
>   
> +	if (!can_unsync)
> +		return -EPERM;
> +
> +	if (prefetch)
> +		return -EEXIST;
>   	/*
>   	 * The page is not write-tracked, mark existing shadow pages unsync
>   	 * unless KVM is synchronizing an unsync SP (can_unsync = false).  In
> @@ -2593,15 +2598,9 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
>   	 * allowing shadow pages to become unsync (writable by the guest).
>   	 */
>   	for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
> -		if (!can_unsync)
> -			return -EPERM;
> -
>   		if (sp->unsync)
>   			continue;
>   
> -		if (prefetch)
> -			return -EEXIST;
> -

Re: [PATCH] KVM: x86: move the can_unsync and prefetch checks outside of the loop

[Vihas Mak]

On Fri, Jan 07, 2022, Sean Christopherson wrote:
>> NAK, this change is functionally wrong.  The checks are inside the loop because
>> the flow fails if and only if there is at least one indirect, valid shadow pages
>> at the target gfn.  The @prefetch check is even more restrictive as it bails if
>> there is at least one indirect, valid, synchronized shadow page.
>> The can_unsync check could be "optimized" to
>>
>>       if (!can_unsync && kvm_gfn_has_indirect_valid_sp())
>>
>> but identifying whether or not there's a valid SP requires walking the list of
>> shadow pages for the gfn, so it's simpler to just handle the check in the loop.
>> And "optimized" in quotes because both checks will be well-predicted single-uop
>> macrofused TEST+Jcc on modern CPUs, whereas walking the list twice would be
>> relatively expensive if there are shadow pages for the gfn.


So this change isn't safe. I will look into the optimization suggested
by Sean. Sorry for this patch.


On Fri, Jan 7, 2022 at 11:46 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 1/7/22 09:25, Vihas Mak wrote:
> > mmu_try_to_unsync_pages() performs !can_unsync check before attempting
> > to unsync any shadow pages.
> > This check is peformed inside the loop right now.
> > It's redundant to perform it every iteration if can_unsync is true, as
> > can_unsync parameter isn't getting updated inside the loop.
> > Move the check outside of the loop.
> >
> > Same is the case with prefetch.
>
> The meaning changes if the loop does not execute at all.  Is this safe?
>
> Paolo
>
> > Signed-off-by: Vihas Mak<makvihas@gmail.com>
> > Cc: Sean Christopherson<seanjc@google.com>
> > Cc: Vitaly Kuznetsov<vkuznets@redhat.com>
> > Cc: Wanpeng Li<wanpengli@tencent.com>
> > Cc: Jim Mattson<jmattson@google.com>
> > Cc: Joerg Roedel<joro@8bytes.org>
> > ---
> >   arch/x86/kvm/mmu/mmu.c | 11 +++++------
> >   1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 1d275e9d7..53f4b8b07 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2586,6 +2586,11 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
> >       if (kvm_slot_page_track_is_active(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE))
> >               return -EPERM;
> >
> > +     if (!can_unsync)
> > +             return -EPERM;
> > +
> > +     if (prefetch)
> > +             return -EEXIST;
> >       /*
> >        * The page is not write-tracked, mark existing shadow pages unsync
> >        * unless KVM is synchronizing an unsync SP (can_unsync = false).  In
> > @@ -2593,15 +2598,9 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
> >        * allowing shadow pages to become unsync (writable by the guest).
> >        */
> >       for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
> > -             if (!can_unsync)
> > -                     return -EPERM;
> > -
> >               if (sp->unsync)
> >                       continue;
> >
> > -             if (prefetch)
> > -                     return -EEXIST;
> > -
>


--
Thanks,
Vihas

Re: [PATCH] KVM: x86: move the can_unsync and prefetch checks outside of the loop

[Sean Christopherson]

On Sat, Jan 08, 2022, Vihas Mak wrote:
> On Fri, Jan 07, 2022, Sean Christopherson wrote:
> >> NAK, this change is functionally wrong.  The checks are inside the loop because
> >> the flow fails if and only if there is at least one indirect, valid shadow pages
> >> at the target gfn.  The @prefetch check is even more restrictive as it bails if
> >> there is at least one indirect, valid, synchronized shadow page.
> >> The can_unsync check could be "optimized" to
> >>
> >>       if (!can_unsync && kvm_gfn_has_indirect_valid_sp())
> >>
> >> but identifying whether or not there's a valid SP requires walking the list of
> >> shadow pages for the gfn, so it's simpler to just handle the check in the loop.
> >> And "optimized" in quotes because both checks will be well-predicted single-uop
> >> macrofused TEST+Jcc on modern CPUs, whereas walking the list twice would be
> >> relatively expensive if there are shadow pages for the gfn.
> 
> 
> So this change isn't safe. I will look into the optimization suggested
> by Sean. Sorry for this patch.

Heh, I wasn't actually suggesting we do the "optimization".  I was pointing out
what the code would look like _if_ we wanted to move the checks out of the loop,
but I do not actually think we should make any changes, quite the opposite.  The
theoretical worst case if there no indirect valid SPs, but lots of direct and/or
invalid SPs is far worse than burning a few uops per loop.

The compiler is smart enough to handle the checks out of line, and I'm sure there
are other optimizations being made as well.  In other words, odds are very good
that trying to optimize the code will do more harm than good.