Re: [PATCH] kvm: Fix memory slot generation updates

2012-12-24 Thread Alex Williamson 
On Sun, 2012-12-23 at 10:55 +0200, Gleb Natapov wrote:
 On Fri, Dec 21, 2012 at 08:20:16AM -0700, Alex Williamson wrote:
  Previous patch kvm: Minor memory slot optimization overlooked the
  generation field of the memory slots.  Re-using the original memory
  slots left us with with two slightly different memory slots with the
  same generation.  To fix this, make update_memslots() take a new
  parameter to specify the last generation.  This also makes generation
  management more explicit to avoid such problems in the future.
  
  Reported-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
  Signed-off-by: Alex Williamson alex.william...@redhat.com
 Applied, thanks. What about this small cleanup on to of the patch:

Nice cleanup

Reviewed-by: Alex Williamson alex.william...@redhat.com

 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 14cbae8..e45c20c 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -702,6 +702,17 @@ static int check_memory_region_flags(struct 
 kvm_userspace_memory_region *mem)
   return 0;
  }
  
 +static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
 + struct kvm_memslots *slots, struct kvm_memory_slot *new)
 +{
 + struct kvm_memslots *old_memslots = kvm-memslots;
 +
 + update_memslots(slots, new, kvm-memslots-generation);
 + rcu_assign_pointer(kvm-memslots, slots);
 + synchronize_srcu_expedited(kvm-srcu);
 + return old_memslots; 
 +}
 +
  /*
   * Allocate some memory and give it an address in the guest physical address
   * space.
 @@ -820,11 +831,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
   slot = id_to_memslot(slots, mem-slot);
   slot-flags |= KVM_MEMSLOT_INVALID;
  
 - update_memslots(slots, NULL, kvm-memslots-generation);
 + old_memslots = install_new_memslots(kvm, slots, NULL);
  
 - old_memslots = kvm-memslots;
 - rcu_assign_pointer(kvm-memslots, slots);
 - synchronize_srcu_expedited(kvm-srcu);
   /* slot was deleted or moved, clear iommu mapping */
   kvm_iommu_unmap_pages(kvm, old);
   /* From this point no new shadow pages pointing to a deleted,
 @@ -868,10 +876,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
   memset(new.arch, 0, sizeof(new.arch));
   }
  
 - update_memslots(slots, new, kvm-memslots-generation);
 - old_memslots = kvm-memslots;
 - rcu_assign_pointer(kvm-memslots, slots);
 - synchronize_srcu_expedited(kvm-srcu);
 + old_memslots = install_new_memslots(kvm, slots, new);
  
   kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
  
 --
   Gleb.



--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: Fix memory slot generation updates

2012-12-23 Thread Gleb Natapov 
On Fri, Dec 21, 2012 at 08:20:16AM -0700, Alex Williamson wrote:
 Previous patch kvm: Minor memory slot optimization overlooked the
 generation field of the memory slots.  Re-using the original memory
 slots left us with with two slightly different memory slots with the
 same generation.  To fix this, make update_memslots() take a new
 parameter to specify the last generation.  This also makes generation
 management more explicit to avoid such problems in the future.
 
 Reported-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
 Signed-off-by: Alex Williamson alex.william...@redhat.com
Applied, thanks. What about this small cleanup on to of the patch:


diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 14cbae8..e45c20c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -702,6 +702,17 @@ static int check_memory_region_flags(struct 
kvm_userspace_memory_region *mem)
return 0;
 }
 
+static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
+   struct kvm_memslots *slots, struct kvm_memory_slot *new)
+{
+   struct kvm_memslots *old_memslots = kvm-memslots;
+
+   update_memslots(slots, new, kvm-memslots-generation);
+   rcu_assign_pointer(kvm-memslots, slots);
+   synchronize_srcu_expedited(kvm-srcu);
+   return old_memslots; 
+}
+
 /*
  * Allocate some memory and give it an address in the guest physical address
  * space.
@@ -820,11 +831,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
slot = id_to_memslot(slots, mem-slot);
slot-flags |= KVM_MEMSLOT_INVALID;
 
-   update_memslots(slots, NULL, kvm-memslots-generation);
+   old_memslots = install_new_memslots(kvm, slots, NULL);
 
-   old_memslots = kvm-memslots;
-   rcu_assign_pointer(kvm-memslots, slots);
-   synchronize_srcu_expedited(kvm-srcu);
/* slot was deleted or moved, clear iommu mapping */
kvm_iommu_unmap_pages(kvm, old);
/* From this point no new shadow pages pointing to a deleted,
@@ -868,10 +876,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
memset(new.arch, 0, sizeof(new.arch));
}
 
-   update_memslots(slots, new, kvm-memslots-generation);
-   old_memslots = kvm-memslots;
-   rcu_assign_pointer(kvm-memslots, slots);
-   synchronize_srcu_expedited(kvm-srcu);
+   old_memslots = install_new_memslots(kvm, slots, new);
 
kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
 
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: Fix memory slot generation updates

2012-12-23 Thread Takuya Yoshikawa 
On Sun, 23 Dec 2012 10:55:06 +0200
Gleb Natapov g...@redhat.com wrote:

 Applied, thanks. What about this small cleanup on to of the patch:

Looks nice!  Hope to see this applied as well.

Thanks,
Takuya

 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 14cbae8..e45c20c 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -702,6 +702,17 @@ static int check_memory_region_flags(struct 
 kvm_userspace_memory_region *mem)
   return 0;
  }
  
 +static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
 + struct kvm_memslots *slots, struct kvm_memory_slot *new)
 +{
 + struct kvm_memslots *old_memslots = kvm-memslots;
 +
 + update_memslots(slots, new, kvm-memslots-generation);
 + rcu_assign_pointer(kvm-memslots, slots);
 + synchronize_srcu_expedited(kvm-srcu);
 + return old_memslots; 
 +}
 +
  /*
   * Allocate some memory and give it an address in the guest physical address
   * space.
 @@ -820,11 +831,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
   slot = id_to_memslot(slots, mem-slot);
   slot-flags |= KVM_MEMSLOT_INVALID;
  
 - update_memslots(slots, NULL, kvm-memslots-generation);
 + old_memslots = install_new_memslots(kvm, slots, NULL);
  
 - old_memslots = kvm-memslots;
 - rcu_assign_pointer(kvm-memslots, slots);
 - synchronize_srcu_expedited(kvm-srcu);
   /* slot was deleted or moved, clear iommu mapping */
   kvm_iommu_unmap_pages(kvm, old);
   /* From this point no new shadow pages pointing to a deleted,
 @@ -868,10 +876,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
   memset(new.arch, 0, sizeof(new.arch));
   }
  
 - update_memslots(slots, new, kvm-memslots-generation);
 - old_memslots = kvm-memslots;
 - rcu_assign_pointer(kvm-memslots, slots);
 - synchronize_srcu_expedited(kvm-srcu);
 + old_memslots = install_new_memslots(kvm, slots, new);
  
   kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
  
 --
   Gleb.


-- 
Takuya Yoshikawa takuya.yoshik...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] kvm: Fix memory slot generation updates

2012-12-21 Thread Alex Williamson 
Previous patch kvm: Minor memory slot optimization overlooked the
generation field of the memory slots.  Re-using the original memory
slots left us with with two slightly different memory slots with the
same generation.  To fix this, make update_memslots() take a new
parameter to specify the last generation.  This also makes generation
management more explicit to avoid such problems in the future.

Reported-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
 include/linux/kvm_host.h |3 ++-
 virt/kvm/kvm_main.c  |9 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 32fdc45..cbe0d68 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -428,7 +428,8 @@ void kvm_exit(void);
 
 void kvm_get_kvm(struct kvm *kvm);
 void kvm_put_kvm(struct kvm *kvm);
-void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new);
+void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new,
+u64 last_generation);
 
 static inline struct kvm_memslots *kvm_memslots(struct kvm *kvm)
 {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index bd31096..14cbae8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -672,7 +672,8 @@ static void sort_memslots(struct kvm_memslots *slots)
slots-id_to_index[slots-memslots[i].id] = i;
 }
 
-void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new)
+void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new,
+u64 last_generation)
 {
if (new) {
int id = new-id;
@@ -684,7 +685,7 @@ void update_memslots(struct kvm_memslots *slots, struct 
kvm_memory_slot *new)
sort_memslots(slots);
}
 
-   slots-generation++;
+   slots-generation = last_generation + 1;
 }
 
 static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
@@ -819,7 +820,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
slot = id_to_memslot(slots, mem-slot);
slot-flags |= KVM_MEMSLOT_INVALID;
 
-   update_memslots(slots, NULL);
+   update_memslots(slots, NULL, kvm-memslots-generation);
 
old_memslots = kvm-memslots;
rcu_assign_pointer(kvm-memslots, slots);
@@ -867,7 +868,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
memset(new.arch, 0, sizeof(new.arch));
}
 
-   update_memslots(slots, new);
+   update_memslots(slots, new, kvm-memslots-generation);
old_memslots = kvm-memslots;
rcu_assign_pointer(kvm-memslots, slots);
synchronize_srcu_expedited(kvm-srcu);

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html