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.
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?
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.
--
Peter Xu
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
