Re: [RFC PATCH 4/9] kvm_main.c: split logic in kvm_set_memslots

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/9/22 12:45, Emanuele Giuseppe Esposito wrote:
+/*
+ * Takes kvm->slots_arch_lock, and releases it only if
+ * invalid_slot allocation or kvm_prepare_memory_region failed.
+*/

Much simpler: "kvm->slots_arch_lock is taken on a successful return."

This is a small change in phrasing, but it makes a lot more sense: on success you are preparing for the final commit operation, otherwise you just want the caller to return your errno value.

[...]

+/* Must be called with kvm->slots_arch_lock held, but releases it. */
s/but/and/. Even better, "and releases it before returning". "But" tells the reader that something strange is going on, "and" tells it that something simple is going on.

I would also rename the functions along the lines of my review to patch 3, to highlight that these function prepare/commit a *change* to a memslot.

+static void kvm_finish_memslot(struct kvm *kvm,
+			       struct kvm_internal_memory_region_list *batch)
+{
+	struct kvm_memory_slot *invalid_slot = batch->invalid;
+	struct kvm_memory_slot *old = batch->old;
+	struct kvm_memory_slot *new = batch->new;
+	enum kvm_mr_change change = batch->change;

lockdep_assert_held(&kvm->slots_arch_lock);

/*
  	 * For DELETE and MOVE, the working slot is now active as the INVALID
@@ -1883,6 +1898,18 @@ static int kvm_set_memslot(struct kvm *kvm,
  	 * responsible for knowing that new->arch may be stale.
  	 */
  	kvm_commit_memory_region(kvm, batch);
+}
+
+static int kvm_set_memslot(struct kvm *kvm,
+			   struct kvm_internal_memory_region_list *batch)
+{
+	int r;
+
+	r = kvm_prepare_memslot(kvm, batch);
+	if (r)
+		return r;
+
+	kvm_finish_memslot(kvm, batch);
return 0;

Ok, these are the functions I hinted at in the review of the previous patch, so we're not far away. You should be able to move the kvm_set_memslot call to kvm_set_memory_region in patch 3, and then replace it with the two calls here directly in kvm_set_memory_region.

Paolo




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux