Hi, > > > + > > + preempt_disable(); > > + /* > > + * Ensure the this vCPU isn't modifying another vCPU's run struct, which > > + * would open the door for races between concurrent calls to this > > + * function. > > + */ > > + if (WARN_ON_ONCE(vcpu != __this_cpu_read(kvm_running_vcpu))) > > + goto out; > > Meh, this is overkill IMO. The check in mark_page_dirty_in_slot() is an > abomination that I wish didn't exist, not a pattern that should be copied. If > we do keep this sanity check, it can simply be > > if (WARN_ON_ONCE(vcpu != kvm_get_running_vcpu())) > return; > > because as the comment for kvm_get_running_vcpu() explains, the returned vCPU > pointer won't change even if this task gets migrated to a different pCPU. If > this code were doing something with vcpu->cpu then preemption would need to be > disabled throughout, but that's not the case. > I think that this check is needed but without the WARN_ON_ONCE as per my other comment. Reason is that we really need to insulate the code against preemption kicking in before the call to preempt_disable() as the logic seems to need this check to avoid concurrency problems. (I don't think Anish simply copied this if-check from mark_page_dirty_in_slot)
