2017-08-17 21:17+0200, Alexander Graf:

On 17.08.17 16:54, Radim Kr?m?? wrote:

quoted

2017-08-17 09:04+0200, Alexander Graf:

quoted

What if we just sent a "vcpu move" request to all vcpus with the new pointer
after it moved? That way the vcpu thread itself would be responsible for the
migration to the new memory region. Only if all vcpus successfully moved,
keep rolling (and allow foreign get_vcpu again).

I'm not sure if I understood this.  You propose to cache kvm->vcpus in
vcpu->vcpus and do an extensions of this,

   int vcpu_create(...) {
     if (resize_needed(kvm->vcpus)) {
       old_vcpus = kvm->vcpus
       kvm->vcpus = make_bigger(kvm->vcpus)

if (kvm->vcpus != old_vcpus) :)

quoted

       kvm_make_all_cpus_request(kvm, KVM_REQ_UPDATE_VCPUS)

IIRC you'd need some manual bookkeeping to ensure that all users have
switched to the new array. Or set the KVM_REQUEST_WAIT flag :).

Absolutely.  I was thinking about synchronous execution, which might
need extra work to expedite halted VCPUs.  Letting the last user free it
is plausible and would need more protection against races.

quoted

       free(old_vcpus)
     }
     vcpu->vcpus = kvm->vcpus
   }

with added extra locking, (S)RCU, on accesses that do not come from
VCPUs (irqfd and VM ioctl)?

Well, in an ideal world we wouldn't have any users to vcpu structs outside
of the vcpus obviously. Every time we do, we should either reconsider
whether the design is smart and if we think it is, protect them accordingly.

And there would be no linear access to all VCPUs. :)

The main user of kvm->vcpus is kvm_for_each_vcpu(), which is well suited
for a list, so we can change the design of kvm_for_each_vcpu() to use a
list head in struct kvm_vcpu with head/tail in struct kvm.
(The list is trivial to make lockless as we only append.)

This would allow more flexibility with the remaining uses.

Maybe even hard code separate request mechanisms for the few cases where
it's reasonable?

All non-kvm_for_each_vcpu() seem to need accesss outside of VCPU scope.

We have few awkward accesses that can be handled keeping track of kvm
state and all remaining uses need some kind of "int -> struct kvm_vcpu"
mapping, where the integer is arbitrary.

All users of kvm_get_vcpu_by_id() need a vcpu_id mapping, but hijack
kvm->vcpus for O(1) access if lucky, with fallback to
kvm_for_each_vcpu().  Adding a vcpu_id mapping seems reasonable.

s390 __floating_irq_kick() and x86 kvm_irq_delivery_to_apic() are
keeping a bitmap for kvm->vcpus indices.  They want compact indices,
which cannot be provided by vcpu_id mapping.

I think that MIPS and ARM use the index in kvm->vcpus for userspace
communication, which looks dangerous as userspace shouldn't know the
position.  Not much we can do because of that.

quoted

quoted

That way we should be basically lock-less and scale well. For additional
icing, feel free to increase the vcpu array x2 every time it grows to not
run into the slow path too often.

Yeah, I skipped the growing as it was not necessary for the
illustration.

Sure.

I'm also not fully advocating my solution here, but wanted to make sure we
have it on the radar. I *think* this option has the least runtime overhead
and best readability score, as it sticks to the same frameworks we already
have and use throughout the code base ;).

That said, I'd love to get proven wrong.

Unless I missed some uses, the linked list for kvm_for_each_vcpu() and a
use-case specific protection for the rest looks better ... kvm->vcpus is
terribly overloaded.