From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [RFC PATCH 0/2] Expose available KVM free memory slot count to help avoid aborts Date: Wed, 23 Feb 2011 14:46:45 -0700 Message-ID: <1298497605.18387.70.camel@x201> References: <20110121233040.22262.68117.stgit@s20.home> <20110124093241.GA28654@amt.cnet> <4D3D89B1.30300@siemens.com> <1295883899.3230.9.camel@x201> <1295933876.3230.46.camel@x201> <20110131191834.GA27154@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Jan Kiszka , "kvm@vger.kernel.org" , "ddutile@redhat.com" , "mst@redhat.com" , "avi@redhat.com" , "chrisw@redhat.com" To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34534 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932748Ab1BWVqt (ORCPT ); Wed, 23 Feb 2011 16:46:49 -0500 In-Reply-To: <20110131191834.GA27154@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 2011-01-31 at 17:18 -0200, Marcelo Tosatti wrote: > On Mon, Jan 24, 2011 at 10:37:56PM -0700, Alex Williamson wrote: > > On Mon, 2011-01-24 at 08:44 -0700, Alex Williamson wrote: > > > I'll look at how we might be > > > able to allocate slots on demand. Thanks, > > > > Here's a first cut just to see if this looks agreeable. This allows the > > slot array to grow on demand. This works with current userspace, as > > well as userspace trivially modified to double KVMState.slots and > > hotplugging enough pci-assign devices to exceed the previous limit (w/ & > > w/o ept). Hopefully I got the rcu bits correct. Does this look like > > the right path? If so, I'll work on removing the fixed limit from > > userspace next. Thanks, > > > > Alex > > > > > > kvm: Allow memory slot array to grow on demand > > > > Remove fixed KVM_MEMORY_SLOTS limit, allowing the slot array > > to grow on demand. Private slots are now allocated at the > > front instead of the end. Only x86 seems to use private slots, > > so this is now zero for all other archs. The memslots pointer > > is already updated using rcu, so changing the size off the > > array when it's replaces is straight forward. x86 also keeps > > a bitmap of slots used by a kvm_mmu_page, which requires a > > shadow tlb flush whenever we increase the number of slots. > > This forces the pages to be rebuilt with the new bitmap size. > > > > Signed-off-by: Alex Williamson > > --- > > > > arch/ia64/include/asm/kvm_host.h | 4 -- > > arch/ia64/kvm/kvm-ia64.c | 2 + > > arch/powerpc/include/asm/kvm_host.h | 3 -- > > arch/s390/include/asm/kvm_host.h | 3 -- > > arch/x86/include/asm/kvm_host.h | 3 +- > > arch/x86/include/asm/vmx.h | 6 ++- > > arch/x86/kvm/mmu.c | 7 +++- > > arch/x86/kvm/x86.c | 6 ++- > > include/linux/kvm_host.h | 7 +++- > > virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++----------- > > 10 files changed, 63 insertions(+), 43 deletions(-) [snip] > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index fd67bcd..32f023c 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c [snip] > > @@ -752,12 +753,19 @@ skip_lpage: > > > > if (!npages) { > > r = -ENOMEM; > > - slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > > + > > + nmemslots = (mem->slot >= kvm->memslots->nmemslots) ? > > + mem->slot + 1 : kvm->memslots->nmemslots; > > + > > + slots = kzalloc(sizeof(struct kvm_memslots) + > > + nmemslots * sizeof(struct kvm_memory_slot), > > + GFP_KERNEL); > > if (!slots) > > goto out_free; > > - memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots)); > > - if (mem->slot >= slots->nmemslots) > > - slots->nmemslots = mem->slot + 1; > > + memcpy(slots, kvm->memslots, > > + sizeof(struct kvm_memslots) + kvm->memslots->nmemslots * > > + sizeof(struct kvm_memory_slot)); > > + slots->nmemslots = nmemslots; > > Other than the upper limit, should disallow increasing the number of > slots in case the new slot is being deleted (npages == 0). So none of > this additions to !npages case should be necessary. The existing code currently allows adding a slot with !npages. I'll create a small separate patch to make this behavioral change. > Also, must disallow shrinking the number of slots. Right, we only grow, never shrink. > > slots->generation++; > > slots->memslots[mem->slot].flags |= KVM_MEMSLOT_INVALID; > > > > @@ -787,12 +795,21 @@ skip_lpage: > > } > > > > r = -ENOMEM; > > - slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > > + > > + if (mem->slot >= kvm->memslots->nmemslots) { > > + nmemslots = mem->slot + 1; > > + flush = true; > > + } else > > + nmemslots = kvm->memslots->nmemslots; > > + > > + slots = kzalloc(sizeof(struct kvm_memslots) + > > + nmemslots * sizeof(struct kvm_memory_slot), > > + GFP_KERNEL); > > if (!slots) > > goto out_free; > > - memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots)); > > - if (mem->slot >= slots->nmemslots) > > - slots->nmemslots = mem->slot + 1; > > + memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots) + > > + kvm->memslots->nmemslots * sizeof(struct kvm_memory_slot)); > > + slots->nmemslots = nmemslots; > > slots->generation++; > > > > /* actual memory is freed via old in kvm_free_physmem_slot below */ > > @@ -808,6 +825,9 @@ skip_lpage: > > rcu_assign_pointer(kvm->memslots, slots); > > It should be: > > spin_lock(kvm->mmu_lock) > rcu_assign_pointer() > kvm_arch_flush_shadow() > spin_unlock(kvm->mmu_lock) > > But kvm_arch_flush_shadow() takes mmu_lock currently, so that needs > fixing. Hmm, I tried to follow the example in the !npages path just above this that does: rcu_assign_pointer() synchronize_srcu_expedited() kvm_arch_flush_shadow() Do we have an existing issue there with mmu_lock? Thanks, Alex