Re: [PATCH RFC 04/15] KVM: Implement ring-based dirty memory tracking

[Peter Xu]

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