Re: [PATCH v8 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan <gshan@redhat.com>]

Hi Oliver,

On 11/8/22 9:13 AM, Oliver Upton wrote:
> On Tue, Nov 08, 2022 at 08:44:52AM +0800, Gavin Shan wrote:
>> Frankly, I don't expect the capability to be disabled. Similar to KVM_CAP_DIRTY_LOG_RING
>> or KVM_CAP_DIRTY_LOG_RING_ACQ_REL, it would a one-shot capability and only enablement is
>> allowed. The disablement was suggested by Oliver without providing a clarify, even I dropped
>> it for several times. I would like to see if there is particular reason why Oliver want
>> to disable the capability.
>>
>>      kvm->dirty_ring_with_bitmap = cap->args[0];
>>
>> If Oliver agrees that the capability needn't to be disabled. The whole chunk of
>> code can be squeezed into kvm_vm_ioctl_enable_dirty_log_ring_with_bitmap() to
>> make kvm_vm_ioctl_enable_cap_generic() a bit clean, as I said above.
> 
> Sorry, I don't believe there is much use in disabling the cap, and
> really that hunk just came from lazily matching the neigbhoring caps
> when sketching out some suggestions. Oops!
> 

Ok. It doesn't really matter too much except the comments seems conflicting.
Thanks for confirming it's unnecessary to disable the capability.

>> Sean and Oliver, could you help to confirm if the changes look good to you? :)
>>
>>      static int kvm_vm_ioctl_enable_dirty_log_ring_with_bitmap(struct kvm *kvm)
> 
> This function name is ridiculously long...
> 

Yeah, It seems I tempted to make the function name as comments :)

>>      {
>>          int i, r = 0;
>>
>>          if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
>>              !kvm->dirty_ring_size)
>>              return -EINVAL;
>>
>>          mutex_lock(&kvm->slots_lock);
>>
>>          /* We only allow it to set once */
>>          if (kvm->dirty_ring_with_bitmap) {
>>              r = -EINVAL;
>>              goto out_unlock;
>>          }
> 
> I don't believe this check is strictly necessary. Something similar to
> this makes sense with caps that take a numeric value (like
> KVM_CAP_DIRTY_LOG_RING), but this one is a one-way boolean.
> 

Yep, it's not required strictly since it can represent two states.

>>
>>          /*
>>           * Avoid a race between memslot creation and enabling the ring +
>>           * bitmap capability to guarantee that no memslots have been
>>           * created without a bitmap.
> 
> You'll want to pick up Sean's suggestion on the comment which, again, I
> drafted this in haste :-)
> 

Ok, no worries :)

>>           */
>>          for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
>>              if (!kvm_memslots_empty(__kvm_memslots(kvm, i))) {
>>                  r = -EINVAL;
>>                  goto out_unlock;
>>              }
>>          }
> 
> I'd much prefer you take Sean's suggestion and just create a helper to
> test that all memslots are empty. You avoid the insanely long function
> name and avoid the use of a goto statement. That is to say, leave the
> rest of the implementation inline in kvm_vm_ioctl_enable_cap_generic()
> 
> static bool kvm_are_all_memslots_empty(struct kvm *kvm)
> {
> 	int i;
> 
> 	lockdep_assert_held(&kvm->slots_lock);
> 
> 	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> 		if (!kvm_memslots_empty(__kvm_memslots(kvm, i)))
> 			return false;
> 	}
> 
> 	return true;
> }
> 
> static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
> 					   struct kvm_enable_cap *cap)
> {
> 	switch (cap->cap) {
> 
>       [...]
> 
> 	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP: {
> 		int r = -EINVAL;
> 
> 		if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
> 		    !kvm->dirty_ring_size)
> 		    	return r;
> 
> 		mutex_lock(&kvm->slots_lock);
> 
> 		/*
> 		 * For simplicity, allow enabling ring+bitmap if and only if
> 		 * there are no memslots, e.g. to ensure all memslots allocate a
> 		 * bitmap after the capability is enabled.
> 		 */
> 		if (kvm_are_all_memslots_empty(kvm)) {
> 			kvm->dirty_ring_with_bitmap = true;
> 			r = 0;
> 		}
> 
> 		mutex_unlock(&kvm->slots_lock);
> 		return r;
> 	}
> 
> }

Ok. Lets change the chunk as Sean suggested in v9, which should be posted soon.

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm