linux-kernel - Re: [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus

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, 11 Apr 2017 13:34:49 +0800
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Andrew Jones <drjones@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Christoffer Dall <cdall@...aro.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Cornelia Huck <cornelia.huck@...ibm.com>,
        James Hogan <james.hogan@...tec.com>,
        Paul Mackerras <paulus@...abs.org>
Subject: Re: [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request

On 10/04/2017 19:14, Andrew Jones wrote:
> Any reason we don't want kvm_vcpu_kick() to also get the
> if (!(req & KVM_REQUEST_NO_WAKEUP)) optimization condition?

Because what we want is kvm_make_request to do the kick instead,
"if (!(req & KVM_REQUEST_NO_WAKEUP))", I think.

> I did some
> grepping, and don't see any kicks of the requests that have been marked as
> NO_WAKEUP, so nothing should change by adding it now.  But the consistency
> would be nice for the doc I'm writing.
> 
> Also, the condition in kvm_vcpu_kick() looks like overkill
> 
>  cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)
> 
> How could vcpu->cpu ever be any offline/invalid cpu, other than -1?  The
> condition in kvm_make_all_cpus_request() makes more sense to me
> 
>  cpu != -1 && cpu != me
> 
> I guess a lot this stuff is planned for a larger requests rework, when
> kicks get integrated with requests?

Yes, this is more or less what I meant above.

> I'm a bit anxious, though, as it
> changes how I document stuff now, and even how I approach the ARM series.
> For example, if kvm_make_request() already integrated kvm_vcpu_kick(),
> which means also adding the smp_mb__after_atomic(), like
> kvm_make_all_cpus_request() has, then I wouldn't need to add the smp_mb()
> to kvm_arch_vcpu_should_kick().

kvm_arch_vcpu_should_kick() does cmpxchg, which already includes a
memory barrier when it succeeds, so you need not add smp_mb() there.
And indeed by integrating kicks and requests we know that all callers of
kvm_arch_vcpu_should_kick() already do an atomic +
smp_mb__after_atomic(), so there's even less reason to worry about
memory barriers.

kvm_arch_vcpu_should_kick() could then use cmpxchg_relaxed if it helps
ARM, and you could even split the loop in two to limit the number of
memory barriers:

        kvm_for_each_vcpu(i, vcpu, kvm) {
                set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
	smp_mb__after_atomic();
	/* now kick and/or wakeup */

It won't make a difference in practice because there's something wrong
if kvm_make_all_cpus_request is a hot spot, but it's readable code and
it makes sense.

In any case, as soon as your patches get in, whoever does the cleanup
also has the honor of updating the docs.  Radim could also get extra
karma for putting your documentation at the beginning of this series,
and updating it at the same time. :)

Paolo