lkml.org 
[lkml]   [2011]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2011-02-27 10:47    [W:0.151 / U:0.456 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site