Re: [PATCH 1/9] accel/kvm: Extract common KVM vCPU {creation,parking} code

["Alex Bennée" <alex.bennee@linaro.org>]

Salil Mehta <salil.mehta@huawei.com> writes:

> Hi Alex,
> Thanks for taking pains to review it.
>
>> From: Alex Bennée <alex.bennee@linaro.org>
<snip>
>> Salil Mehta <salil.mehta@huawei.com> writes:
>> 
>> > KVM vCPU creation is done once during the initialization of the VM when Qemu
>> > threads are spawned. This is common to all the architectures.
>> >
>> > Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but
>> > the KVM vCPU objects in the Host KVM are not destroyed and their representative
>> > KVM vCPU objects/context in Qemu are parked.
>> >
>> > Refactor common logic so that some APIs could be reused by vCPU Hotplug code.
>> >
>> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>> > ---
>> >  accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
>> >  include/sysemu/kvm.h |  2 ++
>> >  2 files changed, 49 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> > index ff1578bb32..57889c5f6c 100644
>> > --- a/accel/kvm/kvm-all.c
>> > +++ b/accel/kvm/kvm-all.c
>> > @@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock;
>> >  #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>> >
>> >  static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>> > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>> 
>> weird to have a forward static declaration here if you are declare in
>> the header later on.
>
> Yes, it is awkward. Actually, I had to move a function to get
> a diff of changes which were more readable. Without moving
> existing kvm_get_vcpu() function, change was not getting
> highlighted properly in the generated patch. This relocation
> of the function meant the order of declaration and its call
> got reversed. So I had to use above tactics, even though weird.
>
> Suggestions welcome to solve this?

You could have a code-motion commit ahead of this one to move things
into more useful places.

>
>> >  static inline void kvm_resample_fd_remove(int gsi)
>> >  {
>> > @@ -320,11 +321,51 @@ err:
>> >      return ret;
>> >  }
>> >
>> > +void kvm_park_vcpu(CPUState *cpu)
>> > +{
>> > +    unsigned long vcpu_id = cpu->cpu_index;
>> 
>> cpu_index is a plain int in CPUState:
>> 
>>     int cpu_index;
>> 
>> but is defined as an unsigned long in KVMParkedVcpu:
>> 
>>     unsigned long vcpu_id;
>> 
>> I'm not sure if this is just a historical anomaly but I suspect we
>> should fixup the discrepancy first so all users of cpu_index use the
>> same size.
>
>
> Yes, agreed. But it is out of scope of this patch.

Not so sure about that. Its adding another potential integer overflow
that would have to be cleared up later rather than fixing the
foundations before doing the refactor.

>> > +    struct KVMParkedVcpu *vcpu;
>> > +
>> > +    vcpu = g_malloc0(sizeof(*vcpu));
>> > +    vcpu->vcpu_id = vcpu_id;
>> > +    vcpu->kvm_fd = cpu->kvm_fd;
>> > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> > +}
>> > +
>> > +int kvm_create_vcpu(CPUState *cpu)
>> > +{
>> 
>> I don't think you get anything other than -1 on failure so at this point
>> you might as well return a bool.
>
> kvm_vm_ioctl() can return other values? The 'ret' value can then be set 
> using error_setg_errno() for example, in kvm_init_vcpu().

Ahh yes - although:

    if (ret == -1) {
        ret = -errno;
    }

So I think you end up inverting again at the error reporting stage.

> It is better to keep error handling standard for any future changes
> as well.

Well the "proper" way is to pass errp down because the -1 from
kvm_get_cpu() isn't the same as -errno from the kvm_vm_ioctl() so
passing that to error_setg_errno() for the former is incorrect as its
just a lookup failure and -1 doesn't mean anything other than fail in
that context.

So yes returning int can be valid but often bool + errp makes for neater
code.

>> 
>> > +    unsigned long vcpu_id = cpu->cpu_index;
>> 
>> Is this used?
>
> It is used but we can get away with this. I think it
> is a stray left over from shuffle.
>
> Thanks!
>
>> > +    KVMState *s = kvm_state;
>> > +    int ret;
>> > +
>> > +    DPRINTF("kvm_create_vcpu\n");
>> 
>> Please don't add new DPRINTFs - use tracepoints instead. Whether its
>> worth cleaning up up kvm-all first I leave up to you.
>
> I can definitely change for this code.
>
>
>> 
>> > +    /* check if the KVM vCPU already exist but is parked */
>> > +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>> > +    if (ret > 0) {
>> > +        goto found;
>> > +    }
>> > +
>> > +    /* create a new KVM vCPU */
>> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>> > +    if (ret < 0) {
>> > +        return ret;
>> > +    }
>> > +
>> > +found:
>> > +    cpu->vcpu_dirty = true;
>> > +    cpu->kvm_fd = ret;
>> > +    cpu->kvm_state = s;
>> > +    cpu->dirty_pages = 0;
>> > +    cpu->throttle_us_per_full = 0;
>> > +
>> > +    return 0;
>> > +}
>> > +
>> 
>> This is trivially nestable to avoid gotos:
>
>
> No issues. I can remove gotos. I was trying to reduce
> indentation.

I think 2 levels is fair enough. We have a lot of goto's in the code for
error clean-up paths although we are slowly cleaning them up now we have
things like g_autoptr and friends. 

>> 
>>   bool kvm_create_vcpu(CPUState *cpu)
>
> It is better to return ioctl value so that error can be
> set at the caller handling code.

Or pass down errp.

>
>>   {
>>       unsigned long vcpu_id = cpu->cpu_index;
>>       KVMState *s = kvm_state;
>>       int ret;
>> 
>>       /* check if the KVM vCPU already exist but is parked */
>>       ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>>       if (ret < 0) {
>>           /* not found, try to create a new KVM vCPU */
>>           ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>           if (ret < 0) {
>>               return false;
>>           }
>>       }
>
> Yes, can be done this way.
>
>> 
>>       cpu->vcpu_dirty = true;
>>       cpu->kvm_fd = ret;
>>       cpu->kvm_state = s;
>>       cpu->dirty_pages = 0;
>>       cpu->throttle_us_per_full = 0;
>> 
>>       return true;
>
> return value is better than Boolean.
>
>>   }
>> 
>> >  static int do_kvm_destroy_vcpu(CPUState *cpu)
>> >  {
>> >      KVMState *s = kvm_state;
>> >      long mmap_size;
>> > -    struct KVMParkedVcpu *vcpu = NULL;
>> >      int ret = 0;
>> >
>> >      DPRINTF("kvm_destroy_vcpu\n");
>> > @@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>> >          }
>> >      }
>> >
>> > -    vcpu = g_malloc0(sizeof(*vcpu));
>> > -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>> > -    vcpu->kvm_fd = cpu->kvm_fd;
>> > -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> > +    kvm_park_vcpu(cpu);
>> >  err:
>> >      return ret;
>> >  }
>> > @@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long
>> vcpu_id)
>> >          }
>> >      }
>> >
>> > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>> > +    return -1;
>> >  }
>> >
>> >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
>> > @@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>> 
>> Hmm it looks like no one cares about the return value here and the fact
>> that callers use &error_fatal should be enough to exit. You can then
>> just return early and avoid the error ladder.
>
> Maybe yes, but is that change really required?

Well it would simplify the goto jumps in the function that are only
there to ensure a value that is never looked at is generated. 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro