Re: [RFC PATCH 2/3] kvm: Allow memory slot array to grow on demand

[Avi Kivity <avi@redhat.com>]

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