On Thu, Aug 15, 2024 at 4:09 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > (This is preexisting in reexecute_instruction() and goes away in patch 18, if > > I'm pre-reading that part of the series correctly). > > > > Bonus points for opportunistically adding a READ_ONCE() here and in > > kvm_mmu_track_write(). > > Hmm, right, this one should have a READ_ONCE(), but I don't see any reason to > add one in kvm_mmu_track_write(). If the compiler was crazy and generate multiple > loads between the smp_mb() and write_lock(), _and_ the value transitioned from > 1->0, reading '0' on the second go is totally fine because it means the last > shadow page was zapped. Amusingly, it'd actually be "better" in that it would > avoid unnecessary taking mmu_lock. Your call, but I have started leaning towards always using READ_ONCE(), similar to all atomic_t accesses are done with atomic_read(); that is, just as much as a marker for cross-thread lock-free accesses, in addition to limiting the compiler's optimizations. tools/memory-model/Documentation/access-marking.txt also suggests using READ_ONCE() and WRITE_ONCE() always except in special cases. They are also more friendly to KCSAN (though I have never used it). This of course has the issue of being yet another unfinished transition. > Obviously the READ_ONCE() would be harmless, but IMO it would be more confusing > than helpful, e.g. would beg the question of why kvm_vcpu_exit_request() doesn't > wrap vcpu->mode with READ_ONCE(). Heh, though arguably vcpu->mode should be > wrapped with READ_ONCE() since it's a helper and could be called multiple times > without any code in between that would guarantee a reload. Indeed, who said I wouldn't change that one as well? :) Paolo
