Re: [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request

[Paolo Bonzini]

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