On Thu, Aug 15, 2024, Paolo Bonzini wrote: > 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. I opted to fix the kvm_vcpu_exit_request() case[*], and add the READ_ONCE() to this patch, but left kvm_mmu_track_write() as-is. My reasoning, and what I think makes for a decent policy, is that while I 100% agree lockless accesses need _some_ form of protection/documentation, I think adding READ_ONCE() (and WRITE_ONCE()) on top adds confusion and makes the actual requirement unclear. In other words, if there's already an smp_rmb() or smp_wmb() (or similar), then don't add READ/WRITE_ONCE() (unless that's also necesary for some reason) because doing so detracts from the barriers that are actually necessary. [*] https://lore.kernel.org/all/20240828232013.768446-1-seanjc@xxxxxxxxxx > > 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 >
