On Wed, Nov 02, 2022 at 03:58:43PM +0000, Marc Zyngier wrote: > 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(). Right, I mixed it up again. :( It's genuine RMW indeed (unlike _set/_read) but I forgot it needs to have a retval to have the memory barriers. Quotting atomic_t.rst: ---8<--- ORDERING (go read memory-barriers.txt first) -------- The rule of thumb: - non-RMW operations are unordered; - RMW operations that have no return value are unordered; - RMW operations that have a return value are fully ordered; - RMW operations that are conditional are unordered on FAILURE, otherwise the above rules apply. ---8<--- Bit clear unordered. > > > > > 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? Sounds good here. Might be slightly off-topic: I didn't quickly spot how do we guarantee two threads doing KVM_RUN ioctl on the same vcpu fd concurrently. I know that's insane and could have corrupted things, but I just want to make sure e.g. even a malicious guest app won't be able to trigger host warnings. -- Peter Xu
