Messages in this thread Patch in this message | | | Date | Sun, 27 Feb 2011 11:44:49 +0200 | From | Avi Kivity <> | Subject | Re: [RFC PATCH 2/3] kvm: Allow memory slot array to grow on demand |
| |
On 02/24/2011 08:08 PM, Alex Williamson wrote: > > > @@ -207,7 +206,7 @@ struct kvm_mmu_page { > > > * One bit set per slot which has memory > > > * in this shadow page. > > > */ > > > - DECLARE_BITMAP(slot_bitmap, KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS); > > > + unsigned long *slot_bitmap; > > > > What about > > > > union { > > DECLARE_BITMAP(direct_slot_bitmap, BITS_PER_LONG); > > unsigned long *indirect_slot_bitmap; > > }; > > > > to make the hackery below more explicit? > > Yeah, it need something to make the hackery go down easier. I was > actually thinking about: > > unsigned long *slot_bitmap; > DECLARE_BITMAP(direct_slot_bitmap, BITS_PER_LONG); > > Where we'd then just set: > > slot_bitmap =&direct_slot_bitmap; > > It wastes 8 bytes, and pushes the cache a little harder, but still helps > the locality and makes the usage more consistent.
unsigned long *sp_slot_bitmap(struct kvm_mmu_page *sp) { ... }
gives you the best of both worlds.
> > > > We don't support failing kvm_mmu_get_page(). See > > mmu_memory_cache_alloc() and mmu_topup_memory_caches(). > > Hmm, apparently my search stopped at __direct_map() calling > kvm_mmu_get_page() and handling an error.
That's dead code (was there from the very first commit into mmu.c).
> > > > > > r = -ENOMEM; > > > - slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > > > + > > > + if (mem->slot>= kvm->memslots->nmemslots) { > > > + nmemslots = mem->slot + 1; > > > + flush = true; > > > > Isn't flush here a little too agressive? Shouldn't we flush only if we > > cross the BITS_PER_LONG threshold? > > Perhaps, but is that overly exploiting our knowledge about the bitmap > implementation? I figured better to error too aggressively than too > lazy since this is a rare event already.
I'm worried about the screen-clearing using the vga window at 0xa[08]000. If that works without too much flushing, then we're fine.
On second thoughts we're likely fine even if we do flush, since it's in a tight loop so it takes very little work to reestablish the dropped sptes.
> > > @@ -1832,6 +1854,8 @@ static long kvm_vm_ioctl(struct file *filp, > > > sizeof kvm_userspace_mem)) > > > goto out; > > > > > > + kvm_userspace_mem.slot += KVM_PRIVATE_MEM_SLOTS; > > > + > > > > Slightly uneasy about this, but no real objection. > > If you have better ideas, let me know. This reminds me to ask about > this chunk: > > @@ -671,7 +674,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > > /* Check for overlaps */ > r = -EEXIST; > - for (i = 0; i< KVM_MEMORY_SLOTS; ++i) { > + for (i = KVM_PRIVATE_MEM_SLOTS; i< kvm->memslots->nmemslots; ++i) { > struct kvm_memory_slot *s =&kvm->memslots->memslots[i]; > > if (s == memslot || !s->npages) > > I kept the same behavior as previous, but it highlights that we're not > checking for overlaps between private slots and anything else. Existing > bug? Thanks,
Yes, possibly serious. Who knows what happens if we create a page using one slot and remove it via another?
Let's go write some Visual Basic.
-- error compiling committee.c: too many arguments to function
| |