I think you don't need mutex here (&vm->rendezvous_mtx), because taking the mutex every time during check can be too expensive for VM with many VCPUs. So this code can introduce bottleneck.

Instead I would suggest to move that engine (rendezvous_req_cpus ) to atomic operations (CPU_SET_ATOMIC, etc) and barriers, it should be fast on reading (check) and safe.

This is a "point" of rendezvous. All vCPU threads are sitting here until the last is done.

vcpu_rendezvous_pending() will never have side effect on an old rendezvous_req_cpus value and therefore added OR/AND logic looks excessive.

In general it should work. But in terms of atomicity, you can see a little window between setting rendezvous_req_cpus and then setting vm->rendezvous_func = func. Setting order is wrong. Reader can check rendezvous_req_cpus and see the bit set and then see rendezvous_func is NULL.

When all code is protected by mutex or spinlock, all changes in that section are seen as atomic (consistent) . But if we change data under protected mutex, but read (access) data without mutex protection, it can be inconsistent.

As solution you can move ATOMIC operation after 'vm->rendezvous_func = func' assignment. That would protect ordering for data writing.

Another alternative for atomic is use memory barriers:
a) between changing rendezvous_func and cpuset (setting rendezvous_func must be first)
b) after CPU_ISSET() in vcpu_rendezvous_pending

Also CPU_OR_ATOMIC can be replaced by CPU_SET_ATOMIC - adding AND/OR is excessive because it is rendezvous.

There one minor mistake in your comment which causes the bug of bz. It's only guaranteed that a subset of all vcpu thread (those specified by rendezvous_req_cpus) are sitting here.

Yeah, I thought about it and you're right. Adding atomic isn't required here.

You're right but I don't understand why it should be required to use a memory barrier after CPU_ISSET() in vcpu_rendezvous_pendig.

Btw: CPU_SET_ATOMIC wouldn't work properly for vms with many vcpu. It only sets a single long value. So, you can only set 64 vcpus. Look at it's implementation:

#define	__BIT_SET_ATOMIC(_s, n, p)					\
	atomic_set_long(&(p)->__bits[__bitset_word(_s, n)],		\
	    __bitset_mask((_s), n))

OTOH, this probably needs to be something like CPU_EMPTY() instead of just checking the first word, or even the check you have here of checking the cpuid might be sensible. That would be a separate followup though. Until we have more vCPUs than bits in a pointer though (which we can't until my other series lands) this isn't yet a problem. I probably should fix this in my other series actually.

No, this shouldn't be CPU_EMPTY. This function should check if the vcpu identified by vcpuid has a pending rendevzous or not. So, this should only test if the bit at position vcpuid is set or not. That's exactly what CPU_ISSET does and it will work even with thousands of vcpus.

I had thought the current code was setting rptr to the cpuset, and CPU_EMPTY() would be a more correct version of checking for an empty mask in that case (to preserve existing semantics).

I do think if we want a comment anywhere we want it here as this is the one read outside of the rendezvous_mtx. The comment could just say that the check is racy, but that's ok as if we see a stale value we will get an IPI to force an exit, and if we see a stale value with it set, the handler will ignore the spurious rendezvous.

I don't know that we need quite this long a comment. We didn't have one before for clearing rendezvous_func above.

Oh, this is why the previous code worked (I had thought it was always pointing to the cpuset)

We generally use atomic_* now instead of these. Also, what you want here instead is probably atomic_thread_fence_rel or you want to use atomic_store_ptr_rel for setting vm->rendezvous_func.

OTOH, I don't know that you need this barrier at all since you've removed the assertion. The fact that the rendezvous handler grabs the rendezvous_mtx before checking any of these members means that we don't actually depend on any real ordering here. It probably is better to set rendezvous_req_cpus earlier as the existing code does now so that any vCPUs doing a VM enter concurrent with this have chance to see it and get ready for rendezvous without needing an IPI to force an exit.

I can add a comment.

The first intention of this patch was to avoid spurious rendezvous completely (and those fixing the issue of bz). This comment explains some details how it's done. As we are going to remove the rendezvous assertion, it's not required any more to avoid spurious rendezvous completely. So, this comment isn't neccessary.

Thanks for the explanation how to use barriers.

You're right as we are going to remove the rendezvous assertion.

What if use additional check on 'rendezvous_func' ? It should be faster than use only CPU_ISSET() on every vmexit that doesn't break vmx/svm loop. Of course that optimisation makes sense if there are a lot of such vmexits.

vcpu_rendezvous_pending()
{ 
 	return (*((uintptr_t *)(info->rptr)) != 0)  && CPU_ISSET(vcpuid, (cpuset_t *)info->rptrset);
}

rptr -> rendezvous_func
rptrset -> rendezvous_req_cpus

Not sure if it's worth. CPU_ISSET is a single lookup (_cpuset[bitoffset] & bitmask). So, it should be fast.
Well, I'm not sure if it's worth to commit this patch at all. Spurious rendezvous are rare and at the moment only occur when sending INIT/SIPI. OTOH, this patch is small, keeps the rendezvous simple and is a slight improvement.

@jhb or @markj may have more experience on that.

I don't think we need to overcomplicate the check with checking both things. Rendezvous should be rare. Modern guests should be using MSI interrupts and not I/O APIC interrupts in which case rendezvous will only occur during boot to start APs.

Perhaps more accurate to say the the races are harmless. They still exist, they are just all handled

We could also change the type of info->rptr to be cpuset_t * which would remove the need for casts. It's only used for this.