On Wed, 02 Nov 2022 14:29:26 +0000, Peter Xu <peterx@xxxxxxxxxx> wrote: > > On Tue, Nov 01, 2022 at 07:39:25PM +0000, Sean Christopherson wrote: > > > @@ -142,13 +144,17 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring) > > > > > > kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > > > > + if (!kvm_dirty_ring_soft_full(ring)) > > > + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); > > > + > > > > Marc, Peter, and/or Paolo, can you confirm that clearing the > > request here won't cause ordering problems? Logically, this makes > > perfect sense (to me, since I suggested it), but I'm mildly > > concerned I'm overlooking an edge case where KVM could end up with > > a soft-full ring but no pending request. > > I don't see an ordering issue here, as long as kvm_clear_request() is using > atomic version of bit clear, afaict that's genuine RMW and should always > imply a full memory barrier (on any arch?) between the soft full check and > the bit clear. At least for x86 the lock prefix was applied. No, clear_bit() is not a full barrier. It only atomic, and thus completely unordered (see Documentation/atomic_bitops.txt). If you want a full barrier, you need to use test_and_clear_bit(). > > However I don't see anything stops a simple "race" to trigger like below: > > recycle thread vcpu thread > -------------- ----------- > if (!dirty_ring_soft_full) <--- not full > dirty_ring_push(); > if (dirty_ring_soft_full) <--- full due to the push > set_request(SOFT_FULL); > clear_request(SOFT_FULL); <--- can wrongly clear the request? > Hmmm, well spotted. That's another ugly effect of the recycle thread playing with someone else's toys. > But I don't think that's a huge matter, as it'll just let the vcpu to have > one more chance to do another round of KVM_RUN. Normally I think it means > there can be one more dirty GFN (perhaps there're cases that it can push >1 > gfns for one KVM_RUN cycle? I never figured out the details here, but > still..) pushed to the ring so closer to the hard limit, but we have had a > buffer zone of KVM_DIRTY_RING_RSVD_ENTRIES (64) entries. So I assume > that's still fine, but maybe worth a short comment here? > > I never know what's the maximum possible GFNs being dirtied for a KVM_RUN > cycle. It would be good if there's an answer to that from anyone. This is dangerous, and I'd rather not go there. It is starting to look like we need the recycle thread to get out of the way. And to be honest: + if (!kvm_dirty_ring_soft_full(ring)) + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); seems rather superfluous. Only clearing the flag in the vcpu entry path feels much saner, and I can't see anything that would break. Thoughts? M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
