[01/10] KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()
diff mbox series

Message ID 20200410231707.7128-2-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: selftests: Add KVM_SET_MEMORY_REGION tests
Related show

Commit Message

Sean Christopherson April 10, 2020, 11:16 p.m. UTC
The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
directly instead of doing an extra lookup.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Wainer dos Santos Moschetta April 13, 2020, 6:26 p.m. UTC | #1
On 4/10/20 8:16 PM, Sean Christopherson wrote:
> The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
> directly instead of doing an extra lookup.


Most of (if not all) vcpu related functions in kvm_util.c receives an 
id, so this change creates an inconsistency.

Disregarding the above comment, the changes look good to me. So:

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 8a3523d4434f..9a783c20dd26 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>    *
>    * Input Args:
>    *   vm - Virtual Machine
> - *   vcpuid - VCPU ID
> + *   vcpu - VCPU to remove
>    *
>    * Output Args: None
>    *
> @@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>    *
>    * Within the VM specified by vm, removes the VCPU given by vcpuid.
>    */
> -static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
> +static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
>   {
> -	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
>   	int ret;
>   
>   	ret = munmap(vcpu->state, sizeof(*vcpu->state));
> @@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
>   	int ret;
>   
>   	while (vmp->vcpu_head)
> -		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
> +		vm_vcpu_rm(vmp, vmp->vcpu_head);
>   
>   	ret = close(vmp->fd);
>   	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
Sean Christopherson April 13, 2020, 9:26 p.m. UTC | #2
On Mon, Apr 13, 2020 at 03:26:55PM -0300, Wainer dos Santos Moschetta wrote:
> 
> On 4/10/20 8:16 PM, Sean Christopherson wrote:
> >The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
> >directly instead of doing an extra lookup.
> 
> 
> Most of (if not all) vcpu related functions in kvm_util.c receives an id, so
> this change creates an inconsistency.

Ya, but taking the id is done out of "necessity", as everything is public
and for whatever reason the design of the selftest framework is to not
expose 'struct vcpu' outside of the utils.  vm_vcpu_rm() is internal only,
IMO pulling the id out of the vcpu just to lookup the same vcpu is a waste
of time.

FWIW, I think the whole vcpuid thing is a bad interface, almost all the
tests end up defining an arbitrary number for the sole VCPU_ID, i.e. the
vcpuid interface just adds a pointless layer of obfuscation.  I haven't
looked through all the tests, but returning the vcpu and making the struct
opaque, same as kvm_vm, seems like it would yield more readable code with
less overhead.

While I'm on a soapbox, hiding 'struct vcpu' and 'struct kvm_vm' also seems
rather silly, but at least that doesn't directly lead to funky code.

> Disregarding the above comment, the changes look good to me. So:
> 
> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> 
> 
> >
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >---
> >  tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> >diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> >index 8a3523d4434f..9a783c20dd26 100644
> >--- a/tools/testing/selftests/kvm/lib/kvm_util.c
> >+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> >@@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> >   *
> >   * Input Args:
> >   *   vm - Virtual Machine
> >- *   vcpuid - VCPU ID
> >+ *   vcpu - VCPU to remove
> >   *
> >   * Output Args: None
> >   *
> >@@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> >   *
> >   * Within the VM specified by vm, removes the VCPU given by vcpuid.
> >   */
> >-static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
> >+static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
> >  {
> >-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> >  	int ret;
> >  	ret = munmap(vcpu->state, sizeof(*vcpu->state));
> >@@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
> >  	int ret;
> >  	while (vmp->vcpu_head)
> >-		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
> >+		vm_vcpu_rm(vmp, vmp->vcpu_head);
> >  	ret = close(vmp->fd);
> >  	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
>
Andrew Jones April 14, 2020, 8:25 a.m. UTC | #3
On Mon, Apr 13, 2020 at 02:26:59PM -0700, Sean Christopherson wrote:
> On Mon, Apr 13, 2020 at 03:26:55PM -0300, Wainer dos Santos Moschetta wrote:
> > 
> > On 4/10/20 8:16 PM, Sean Christopherson wrote:
> > >The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
> > >directly instead of doing an extra lookup.
> > 
> > 
> > Most of (if not all) vcpu related functions in kvm_util.c receives an id, so
> > this change creates an inconsistency.
> 
> Ya, but taking the id is done out of "necessity", as everything is public
> and for whatever reason the design of the selftest framework is to not
> expose 'struct vcpu' outside of the utils.  vm_vcpu_rm() is internal only,
> IMO pulling the id out of the vcpu just to lookup the same vcpu is a waste
> of time.

Agreed

> 
> FWIW, I think the whole vcpuid thing is a bad interface, almost all the
> tests end up defining an arbitrary number for the sole VCPU_ID, i.e. the
> vcpuid interface just adds a pointless layer of obfuscation.  I haven't
> looked through all the tests, but returning the vcpu and making the struct
> opaque, same as kvm_vm, seems like it would yield more readable code with
> less overhead.

Agreed

> 
> While I'm on a soapbox, hiding 'struct vcpu' and 'struct kvm_vm' also seems
> rather silly, but at least that doesn't directly lead to funky code.

Agreed. While the concept has been slowly growing on me, I think accessor
functions for each of the structs members are growing even faster...

Thanks,
drew

> 
> > Disregarding the above comment, the changes look good to me. So:
> > 
> > Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > 
> > 
> > >
> > >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > >---
> > >  tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > >diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > >index 8a3523d4434f..9a783c20dd26 100644
> > >--- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > >+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > >@@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> > >   *
> > >   * Input Args:
> > >   *   vm - Virtual Machine
> > >- *   vcpuid - VCPU ID
> > >+ *   vcpu - VCPU to remove
> > >   *
> > >   * Output Args: None
> > >   *
> > >@@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> > >   *
> > >   * Within the VM specified by vm, removes the VCPU given by vcpuid.
> > >   */
> > >-static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
> > >+static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
> > >  {
> > >-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> > >  	int ret;
> > >  	ret = munmap(vcpu->state, sizeof(*vcpu->state));
> > >@@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
> > >  	int ret;
> > >  	while (vmp->vcpu_head)
> > >-		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
> > >+		vm_vcpu_rm(vmp, vmp->vcpu_head);
> > >  	ret = close(vmp->fd);
> > >  	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
> > 
>
Wainer dos Santos Moschetta April 14, 2020, 1:02 p.m. UTC | #4
On 4/14/20 5:25 AM, Andrew Jones wrote:
> On Mon, Apr 13, 2020 at 02:26:59PM -0700, Sean Christopherson wrote:
>> On Mon, Apr 13, 2020 at 03:26:55PM -0300, Wainer dos Santos Moschetta wrote:
>>> On 4/10/20 8:16 PM, Sean Christopherson wrote:
>>>> The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
>>>> directly instead of doing an extra lookup.
>>>
>>> Most of (if not all) vcpu related functions in kvm_util.c receives an id, so
>>> this change creates an inconsistency.
>> Ya, but taking the id is done out of "necessity", as everything is public
>> and for whatever reason the design of the selftest framework is to not
>> expose 'struct vcpu' outside of the utils.  vm_vcpu_rm() is internal only,
>> IMO pulling the id out of the vcpu just to lookup the same vcpu is a waste
>> of time.
> Agreed


Thanks Sean and Andrew for your comments. I'm not in position to 
change/propose any design of kvm selftests but even though I aimed to 
foster this discussion.

So, please, consider my Reviewed-by...

- Wainer


>
>> FWIW, I think the whole vcpuid thing is a bad interface, almost all the
>> tests end up defining an arbitrary number for the sole VCPU_ID, i.e. the
>> vcpuid interface just adds a pointless layer of obfuscation.  I haven't
>> looked through all the tests, but returning the vcpu and making the struct
>> opaque, same as kvm_vm, seems like it would yield more readable code with
>> less overhead.
> Agreed
>
>> While I'm on a soapbox, hiding 'struct vcpu' and 'struct kvm_vm' also seems
>> rather silly, but at least that doesn't directly lead to funky code.
> Agreed. While the concept has been slowly growing on me, I think accessor
> functions for each of the structs members are growing even faster...
>
> Thanks,
> drew
>
>>> Disregarding the above comment, the changes look good to me. So:
>>>
>>> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>
>>>
>>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>>> ---
>>>>   tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
>>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> index 8a3523d4434f..9a783c20dd26 100644
>>>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> @@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>>>>    *
>>>>    * Input Args:
>>>>    *   vm - Virtual Machine
>>>> - *   vcpuid - VCPU ID
>>>> + *   vcpu - VCPU to remove
>>>>    *
>>>>    * Output Args: None
>>>>    *
>>>> @@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>>>>    *
>>>>    * Within the VM specified by vm, removes the VCPU given by vcpuid.
>>>>    */
>>>> -static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
>>>> +static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
>>>>   {
>>>> -	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
>>>>   	int ret;
>>>>   	ret = munmap(vcpu->state, sizeof(*vcpu->state));
>>>> @@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
>>>>   	int ret;
>>>>   	while (vmp->vcpu_head)
>>>> -		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
>>>> +		vm_vcpu_rm(vmp, vmp->vcpu_head);
>>>>   	ret = close(vmp->fd);
>>>>   	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
Andrew Jones April 14, 2020, 4:02 p.m. UTC | #5
On Fri, Apr 10, 2020 at 04:16:58PM -0700, Sean Christopherson wrote:
> The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
> directly instead of doing an extra lookup.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 8a3523d4434f..9a783c20dd26 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>   *
>   * Input Args:
>   *   vm - Virtual Machine
> - *   vcpuid - VCPU ID
> + *   vcpu - VCPU to remove
>   *
>   * Output Args: None
>   *
> @@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
>   *
>   * Within the VM specified by vm, removes the VCPU given by vcpuid.
>   */
> -static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
> +static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
>  {
> -	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
>  	int ret;
>  
>  	ret = munmap(vcpu->state, sizeof(*vcpu->state));
> @@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
>  	int ret;
>  
>  	while (vmp->vcpu_head)
> -		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
> +		vm_vcpu_rm(vmp, vmp->vcpu_head);
>  
>  	ret = close(vmp->fd);
>  	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
> -- 
> 2.26.0
>

Reviewed-by: Andrew Jones <drjones@redhat.com>
Paolo Bonzini April 15, 2020, 3:11 p.m. UTC | #6
On 13/04/20 23:26, Sean Christopherson wrote:
> FWIW, I think the whole vcpuid thing is a bad interface, almost all the
> tests end up defining an arbitrary number for the sole VCPU_ID, i.e. the
> vcpuid interface just adds a pointless layer of obfuscation.  I haven't
> looked through all the tests, but returning the vcpu and making the struct
> opaque, same as kvm_vm, seems like it would yield more readable code with
> less overhead.

Yes, I agree.  This was in the original Google submission, I didn't like
it either but I didn't feel like changing it and I wouldn't mind if
someone does the work...

Paolo

Patch
diff mbox series

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 8a3523d4434f..9a783c20dd26 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -393,7 +393,7 @@  struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
  *
  * Input Args:
  *   vm - Virtual Machine
- *   vcpuid - VCPU ID
+ *   vcpu - VCPU to remove
  *
  * Output Args: None
  *
@@ -401,9 +401,8 @@  struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
  *
  * Within the VM specified by vm, removes the VCPU given by vcpuid.
  */
-static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
+static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
 {
-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
 	int ret;
 
 	ret = munmap(vcpu->state, sizeof(*vcpu->state));
@@ -427,7 +426,7 @@  void kvm_vm_release(struct kvm_vm *vmp)
 	int ret;
 
 	while (vmp->vcpu_head)
-		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
+		vm_vcpu_rm(vmp, vmp->vcpu_head);
 
 	ret = close(vmp->fd);
 	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"