linux-kernel - Re: [PATCH v3 3/3] KVM: vCPU kick tax cut for running vCPU

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]

Date:   Tue, 19 Oct 2021 17:34:17 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Wanpeng Li <kernellwp@...il.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH v3 3/3] KVM: vCPU kick tax cut for running vCPU

On Tue, Oct 19, 2021, Paolo Bonzini wrote:
> On 19/10/21 10:12, Wanpeng Li wrote:
> > -	if (kvm_vcpu_wake_up(vcpu))
> > -		return;
> > +	me = get_cpu();
> > +
> > +	if (rcuwait_active(kvm_arch_vcpu_get_wait(vcpu)) && kvm_vcpu_wake_up(vcpu))
> > +		goto out;
> 
> This is racy.  You are basically doing the same check that rcuwait_wake_up
> does, but without the memory barrier before.

I was worried that was the case[*], but I didn't have the two hours it would have
taken me to verify there was indeed a problem :-)

The intent of the extra check was to avoid the locked instruction that comes with
disabling preemption via rcu_read_lock().  But thinking more, the extra op should
be little more than a basic arithmetic operation in the grand scheme on modern x86
since the cache line is going to be locked and written no matter what, either
immediately before or immediately after.

So with Paolo's other comment, maybe just this?  And if this doesn't provide the
desired performance boost, changes to the rcuwait behavior should go in separate
patch.

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9ec99f5b972c..ebc6d4f2fbfa 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3333,11 +3333,22 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
         * vCPU also requires it to leave IN_GUEST_MODE.
         */
        me = get_cpu();
+
+       /*
+        * Avoid the moderately expensive "should kick" operation if this pCPU
+        * is currently running the target vCPU, in which case it's a KVM bug
+        * if the vCPU is in the inner run loop.
+        */
+       if (vcpu == __this_cpu_read(kvm_running_vcpu) &&
+           !WARN_ON_ONCE(vcpu->mode == IN_GUEST_MODE))
+               goto out;
+
        if (kvm_arch_vcpu_should_kick(vcpu)) {
                cpu = READ_ONCE(vcpu->cpu);
                if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
                        smp_send_reschedule(cpu);
        }
+out:
        put_cpu();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);

[*] https://lkml.kernel.org/r/YWoOG40Ap0Islpu2@google.com