On Tue, Dec 10, 2019 at 11:07:31AM +0100, Paolo Bonzini wrote: > > I'm thinking whether I can start > > to use this information in the next post on solving an issue I > > encountered with the waitqueue. > > > > Current waitqueue is still problematic in that it could wait even with > > the mmu lock held when with vcpu context. > > I think the idea of the soft limit is that the waiting just cannot > happen. That is, the number of dirtied pages _outside_ the guest (guest > accesses are taken care of by PML, and are subtracted from the soft > limit) cannot exceed hard_limit - (soft_limit + pml_size). So the question go backs to, whether this is guaranteed somehow? Or do you prefer us to keep the warn_on_once until it triggers then we can analyze (which I doubt..)? One thing to mention is that for with-vcpu cases, we probably can even stop KVM_RUN immediately as long as either the per-vm or per-vcpu ring reaches the softlimit, then for vcpu case it should be easier to guarantee that. What I want to know is the rest of cases like ioctls or even something not from the userspace (which I think I should read more later..). If the answer is yes, I'd be more than glad to drop the waitqueue. > > > The issue is KVM_RESET_DIRTY_RINGS needs the mmu lock to manipulate > > the write bits, while it's the only interface to also wake up the > > dirty ring sleepers. They could dead lock like this: > > > > main thread vcpu thread > > =========== =========== > > kvm page fault > > mark_page_dirty_in_slot > > mmu lock taken > > mark dirty, ring full > > queue on waitqueue > > (with mmu lock) > > KVM_RESET_DIRTY_RINGS > > take mmu lock <------------ deadlock here > > reset ring gfns > > wakeup dirty ring sleepers > > > > And if we see if the mark_page_dirty_in_slot() is not with a vcpu > > context (e.g. kvm_mmu_page_fault) but with an ioctl context (those > > cases we'll use per-vm dirty ring) then it's probably fine. > > > > My planned solution: > > > > - When kvm_get_running_vcpu() != NULL, we postpone the waitqueue waits > > until we finished handling this page fault, probably in somewhere > > around vcpu_enter_guest, so that we can do wait_event() after the > > mmu lock released > > I think this can cause a race: > > vCPU 1 vCPU 2 host > --------------------------------------------------------------- > mark page dirty > write to page > treat page as not dirty > add page to ring > > where vCPU 2 skips the clean-page slow path entirely. If we're still with the rule in userspace that we first do RESET then collect and send the pages (just like what we've discussed before), then IMHO it's fine to have vcpu2 to skip the slow path? Because RESET happens at "treat page as not dirty", then if we are sure that we only collect and send pages after that point, then the latest "write to page" data from vcpu2 won't be lost even if vcpu2 is not blocked by vcpu1's ring full? Maybe we can also consider to let mark_page_dirty_in_slot() return a value, then the upper layer could have a chance to skip the spte update if mark_page_dirty_in_slot() fails to mark the dirty bit, so it can return directly with RET_PF_RETRY. Thanks, -- Peter Xu
