Date:   Mon, 9 Dec 2019 16:54:00 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <sean.j.christopherson@...el.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH RFC 04/15] KVM: Implement ring-based dirty memory tracking

On Wed, Dec 04, 2019 at 11:05:47AM +0100, Paolo Bonzini wrote:
> On 03/12/19 19:46, Sean Christopherson wrote:
> > On Tue, Dec 03, 2019 at 02:48:10PM +0100, Paolo Bonzini wrote:
> >> On 02/12/19 22:50, Sean Christopherson wrote:
> >>>>
> >>>> I discussed this with Paolo, but I think Paolo preferred the per-vm
> >>>> ring because there's no good reason to choose vcpu0 as what (1)
> >>>> suggested.  While if to choose (2) we probably need to lock even for
> >>>> per-cpu ring, so could be a bit slower.
> >>> Ya, per-vm is definitely better than dumping on vcpu0.  I'm hoping we can
> >>> find a third option that provides comparable performance without using any
> >>> per-vcpu rings.
> >>>
> >>
> >> The advantage of per-vCPU rings is that it naturally: 1) parallelizes
> >> the processing of dirty pages; 2) makes userspace vCPU thread do more
> >> work on vCPUs that dirty more pages.
> >>
> >> I agree that on the producer side we could reserve multiple entries in
> >> the case of PML (and without PML only one entry should be added at a
> >> time).  But I'm afraid that things get ugly when the ring is full,
> >> because you'd have to wait for all vCPUs to finish publishing the
> >> entries they have reserved.
> > 
> > Ah, I take it the intended model is that userspace will only start pulling
> > entries off the ring when KVM explicitly signals that the ring is "full"?
> 
> No, it's not.  But perhaps in the asynchronous case you can delay
> pushing the reserved entries to the consumer until a moment where no
> CPUs have left empty slots in the ring buffer (somebody must have done
> multi-producer ring buffers before).  In the ring-full case that is
> harder because it requires synchronization.
> 
> > Rather than reserve entries, what if vCPUs reserved an entire ring?  Create
> > a pool of N=nr_vcpus rings that are shared by all vCPUs.  To mark pages
> > dirty, a vCPU claims a ring, pushes the pages into the ring, and then
> > returns the ring to the pool.  If pushing pages hits the soft limit, a
> > request is made to drain the ring and the ring is not returned to the pool
> > until it is drained.
> > 
> > Except for acquiring a ring, which likely can be heavily optimized, that'd
> > allow parallel processing (#1), and would provide a facsimile of #2 as
> > pushing more pages onto a ring would naturally increase the likelihood of
> > triggering a drain.  And it might be interesting to see the effect of using
> > different methods of ring selection, e.g. pure round robin, LRU, last used
> > on the current vCPU, etc...
> 
> If you are creating nr_vcpus rings, and draining is done on the vCPU
> thread that has filled the ring, why not create nr_vcpus+1?  The current
> code then is exactly the same as pre-claiming a ring per vCPU and never
> releasing it, and using a spinlock to claim the per-VM ring.
> 
> However, we could build on top of my other suggestion to add
> slot->as_id, and wrap kvm_get_running_vcpu() with a nice API, mimicking
> exactly what you've suggested.  Maybe even add a scary comment around
> kvm_get_running_vcpu() suggesting that users only do so to avoid locking
> and wrap it with a nice API.  Similar to what get_cpu/put_cpu do with
> smp_processor_id.
> 
> 1) Add a pointer from struct kvm_dirty_ring to struct
> kvm_dirty_ring_indexes:
> 
> vcpu->dirty_ring->data = &vcpu->run->vcpu_ring_indexes;
> kvm->vm_dirty_ring->data = *kvm->vm_run->vm_ring_indexes;
> 
> 2) push the ring choice and locking to two new functions
> 
> struct kvm_ring *kvm_get_dirty_ring(struct kvm *kvm)
> {
> 	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> 
> 	if (vcpu && !WARN_ON_ONCE(vcpu->kvm != kvm)) {
> 		return &vcpu->dirty_ring;
> 	} else {
> 		/*
> 		 * Put onto per vm ring because no vcpu context.
> 		 * We'll kick vcpu0 if ring is full.
> 		 */
> 		spin_lock(&kvm->vm_dirty_ring->lock);
> 		return &kvm->vm_dirty_ring;
> 	}
> }
> 
> void kvm_put_dirty_ring(struct kvm *kvm,
> 			struct kvm_dirty_ring *ring)
> {
> 	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> 	bool full = kvm_dirty_ring_used(ring) >= ring->soft_limit;
> 
> 	if (ring == &kvm->vm_dirty_ring) {
> 		if (vcpu == NULL)
> 			vcpu = kvm->vcpus[0];
> 		spin_unlock(&kvm->vm_dirty_ring->lock);
> 	}
> 
> 	if (full)
> 		kvm_make_request(KVM_REQ_DIRTY_RING_FULL, vcpu);
> }
> 
> 3) simplify kvm_dirty_ring_push to
> 
> void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
> 			 u32 slot, u64 offset)
> {
> 	/* left as an exercise to the reader */
> }
> 
> and mark_page_dirty_in_ring to
> 
> static void mark_page_dirty_in_ring(struct kvm *kvm,
> 				    struct kvm_memory_slot *slot,
> 				    gfn_t gfn)
> {
> 	struct kvm_dirty_ring *ring;
> 
> 	if (!kvm->dirty_ring_size)
> 		return;
> 
> 	ring = kvm_get_dirty_ring(kvm);
> 	kvm_dirty_ring_push(ring, (slot->as_id << 16) | slot->id,
> 			    gfn - slot->base_gfn);
> 	kvm_put_dirty_ring(kvm, ring);
> }

I think I got the major point here.  Unless Sean has some better idea
in the future I'll go with this.

Just until recently I noticed that actually kvm_get_running_vcpu() has
a real benefit in that it gives a very solid result on whether we're
with the vcpu context, even more accurate than when we pass vcpu
pointers around (because sometimes we just passed the kvm pointer
along the stack even if we're with a vcpu context, just like what we
did with mark_page_dirty_in_slot).  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.

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

- For per-vm ring full, I'll do what we do now (wait_event() as long
  in mark_page_dirty_in_ring) assuming it should not be with the mmu
  lock held

To achieve above, I think I really need to know exactly on whether
we're with the vcpu context, where I suppose kvm_get_running_vcpu()
would work for me then, rather than checking against vcpu pointer
passed in.

I also wanted to let KVM_RUN return immediately if either per-vm ring
or per-vcpu ring reaches softlimit always, instead of continue
execution until the next dirty ring full event.

I'd be glad to receive any early comment before I move on to these.

Thanks!

-- 
Peter Xu

Powered by blists - more mailing lists