You could do something like this:

if (vcpu->vm->r_f != NULL) {
      vcpu_unlock();
      if (vcpu->vm->r_f != NULL) {
             .....
      }
 }

in other words, check the condition without the unlocking. I am not even sure that re-check after unlock is needed, since your patch checks only after unlock.

Yeah, it's better to lock and unlock in the if condition. Rechecking after unlock shouldn't be required because vm_handle_rendezvous will recheck it anyways.

In other places we only load the vm->rendezvous_func variable under the mtx_lock(&vm->rendezvous_mtx). Are we ok to look at it here without holding that lock? Is the vcpu lock sufficient?

We're just checking it here. We don't make use of it. If it's a false positive, vm_handle_rendezvous will recheck it again with rendezvous_mtx locked and exits early if it's NULL. So, it wouldn't lead to wrong behavior.

Btw. i've noticed that it doesn't seem necessary to call vcpu_unlock at all. However, I'm not sure whether it's a good idea to keep vcpu locked while calling vm_handle_rendezvous.

Please keep function declarations grouped together at the beginning of the file, e.g., together with vcpu_notify_event_locked().

We cannot keep the vcpu locked while calling vm_handle_rendezvous(): that function acquires a MTX_DEF mutex, and the vcpu mutex is a spin mutex (which holds interrupts disabled).

I agree that we should avoid unlocking the vcpu unnecessarily here. vcpu_set_state_locked() is called many thousands of times per second and we should avoid unnecessary work. So something like this:

if (__predict_false(atomic_load_ptr(&vcpu->vm_rendezvous_func) != NULL)) {
    vcpu_unlock(vcpu);
    ...
    vcpu_lock(vcpu);
}

More generally though, is it really safe to drop the vcpu lock here? I guess so: vcpu_set_state_locked() is currently always called from vcpu_set_state(), which locks the vcpu directly, or vcpu_require_state_locked(), which is always called immediately before or after msleep_spin(&vcpu->mtx). This deserves a comment though.

Shouldn't we recheck vcpu->state after reacquiring the lock? It might have transitioned to VCPU_IDLE, in which case we should not sleep.

We should assert that vm_handle_rendezvous() returns 0.

It would be better to introduce a new helper, vm_handle_rendezvous_all(), which effectively implements this loop, but without acquiring and dropping the rendezvous mutex for each vcpu.

That is,

1. add vm_handle_rendezvous_locked(), which is called with the rendezvous lock already acquired,
2. add vm_handle_rendezvous_all(), which acquires the rendezvous lock and calls vm_handle_rendezvous_locked() for each vCPU.

This way, we avoid a potential race. Suppose there are three vCPUs, 0, 1, 2, and vCPU thread 2 requests a rendezvous. vCPU thread 1 completes it and blocks. vCPU thread 0 tries to lock all vCPUs, then executes the callback for vCPU 0. Then, before it continues to vCPU 1, vCPU 2 issues another rendezvous. vCPU 0 will execute the callback for vCPU 1, then skip 2, and continue. But vCPU 2 will still be sleeping waiting for vCPU 0 to rendezvous.

I think this race is avoided if vCPU 0 does not drop the rendezvous lock while executing the loop.

Another problem though, are we sure it's safe to "steal" the rendezvous callback this way? Look at the rendezvous with vlapic_handle_init(), for example. vlapic_handle_init() does not acquire any locks, so there is nothing synchronizing this thread with the vCPU thread for that vCPU. What if that thread is also accessing the vlapic? The vlapic code assumes that all accesses are vCPU-local, I think.

Another problem though, are we sure it's safe to "steal" the rendezvous callback this way? Look at the rendezvous with vlapic_handle_init(), for example. vlapic_handle_init() does not acquire any locks, so there is nothing synchronizing this thread with the vCPU thread for that vCPU. What if that thread is also accessing the vlapic? The vlapic code assumes that all accesses are vCPU-local, I think.

We only have to call the rendezvous for the vCPU thread we're currently running on, e.g. if vCPU 0 requests a rendezvous and vCPU 1 calls vcpu_lock_all (and then vcpu_set_state_locked), we only have to call handle_rendezvous for vCPU 1 to resolve the deadlock. This would avoid "stealing" the rendezvous callback. So, ideally we can somehow get the vcpu struct of the current thread. Unfortunately, I don't know if that's possible.

Do we need to worry about a false negative — that is, rendezvous_func is null at the time it is checked, and a rendezvous begins instantly after we are satisfied it is null?

It will cause a quite big delay but msleep_spin has a timeout. So, after expiring the timeout, it should run the loop again and then sees the rendezvous.

Unfortunately, I don't know if that's possible.

I don't think it is. vcpu_lock_all() is called from the ioctl path, and some ioctls that use it are not invoked from a vCPU thread in general.

So let's take a step back and make some observations:

• it is safe to invoke the rendezvous callback for a vCPU if the vCPU state is IDLE,
• the state of a vCPU thread sleeping in vm_smp_rendezvous() will be FROZEN,
• if two threads call vcpu_lock_all() at the same time, they will be serialized since they lock the vCPUs in a consistent order,
• the deadlock arises when we try to set the state of a thread sleeping in vm_smp_rendezvous(), and that thread will stop sleeping when all /other/ vCPUs have executed their callback

So I propose the following to avoid the deadlock:

1. use an sx lock to serialize vcpu_lock_all() (i.e., make the existing vm_slock_vcpus() an exclusive lock)
2. in vcpu_lock_all(), lock all IDLE vCPUs first (i.e., set their state to FROZEN) and invoke the rendezvous call back for them
3. then, lock all the remaining vCPUs

I believe this will fix the deadlock as well. Moreover, it is safe to invoke the rendezvous callback on an IDLE vCPU.

I implemented my idea in https://reviews.freebsd.org/D52968 but I'm not set up to reproduce the deadlock. Could you test it?

I implemented my idea in https://reviews.freebsd.org/D52968 but I'm not set up to reproduce the deadlock. Could you test it?

Will test it. Unfortunately, my test system is unavailable at the moment. It will be back in a few days.