Re: [Qemu-devel] [RFC PATCH 7/7] cpus: reclaim allocated vCPU objects

[Gu Zheng]

Hi Anshul,
Thanks for your test.
On 07/30/2014 10:31 PM, Anshul Makkar wrote:

> Hi,
> 
> I am testing the cpu-hotunplug  patches. I observed that after the
> deletion of the cpu with id = x, if I cpu-add the same cpu again id =
> x, then qemu exits with the error that file descriptor already exists.

Could you please offer the whole reproduce routine? In my test box, we
can add a removed cpu with the id. 

> 
> On debugging I found that if I give cpu-add <apic-id = x>, then
> qemu_kvm_cpu_thread_fn->kvm_init_vcpu is called which sends an IOCTL
> (KVM_CREATE_VCPU) to kvm to create a new fd. As the fd already exists
> in KVM as we never delete the fd from the kernel and just park it in
> separate list, it returns false and QEMU exits. In the above code
> flow, no where its being checked if we have the cpu with cpuid = x
> available in the parked list and we can reuse it.
> 
> Am I missing something or this bit is yet to be implmented.

Yes, it is implemented, in the same way as you mention above, please refer
to function kvm_get_vcpu().

Thanks,
Gu

> 
> Thanks
> Anshul Makkar
> 
> On Fri, Jul 18, 2014 at 4:09 AM, Gu Zheng <address@hidden> wrote:
>> Hi Anshul,
>> On 07/18/2014 12:24 AM, Anshul Makkar wrote:
>>
>>> Are we not going to introduce new command cpu_del for deleting the cpu ?
>>>
>>> I couldn't find any patch for addition of cpu_del command. Is this
>>> intentional and we intend to use device_del (and similarly device_add)
>>> for cpu hot(un)plug or just skipped to be added later. I have the
>>> patch for the same which I can release, if the intent is to add this
>>> command.
>>
>> The "device_add/device_del" interface is the approved way to support add/del 
>> cpu,
>> which is also more common and elegant than "cpu_add/del".
>> <http://wiki.qemu.org/Features/CPUHotplug>
>> so we intend to use device_del rather than the cpu_del.
>> And IMO, the cpu_add will be replaced by "device_add" sooner or later.
>>
>> Thanks,
>> Gu
>>
>>>
>>> Thanks
>>> Anshul Makkar
>>>
>>> On Fri, Jul 11, 2014 at 11:59 AM, Gu Zheng <address@hidden> wrote:
>>>> After ACPI get a signal to eject a vCPU, the vCPU must be
>>>> removed from CPU list，before the vCPU really removed,  then
>>>> release the all related vCPU objects.
>>>> But we do not close KVM vcpu fd, just record it into a list, in
>>>> order to reuse it.
>>>>
>>>> Signed-off-by: Chen Fan <address@hidden>
>>>> Signed-off-by: Gu Zheng <address@hidden>
>>>> ---
>>>>  cpus.c               |   37 ++++++++++++++++++++++++++++++++
>>>>  include/sysemu/kvm.h |    1 +
>>>>  kvm-all.c            |   57 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 94 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/cpus.c b/cpus.c
>>>> index 4dfb889..9a73407 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -786,6 +786,24 @@ void async_run_on_cpu(CPUState *cpu, void 
>>>> (*func)(void *data), void *data)
>>>>      qemu_cpu_kick(cpu);
>>>>  }
>>>>
>>>> +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
>>>> +{
>>>> +    CPU_REMOVE(cpu);
>>>> +
>>>> +    if (kvm_destroy_vcpu(cpu) < 0) {
>>>> +        fprintf(stderr, "kvm_destroy_vcpu failed.\n");
>>>> +        exit(1);
>>>> +    }
>>>> +
>>>> +    object_unparent(OBJECT(cpu));
>>>> +}
>>>> +
>>>> +static void qemu_tcg_destroy_vcpu(CPUState *cpu)
>>>> +{
>>>> +    CPU_REMOVE(cpu);
>>>> +    object_unparent(OBJECT(cpu));
>>>> +}
>>>> +
>>>>  static void flush_queued_work(CPUState *cpu)
>>>>  {
>>>>      struct qemu_work_item *wi;
>>>> @@ -877,6 +895,11 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>>>              }
>>>>          }
>>>>          qemu_kvm_wait_io_event(cpu);
>>>> +        if (cpu->exit && !cpu_can_run(cpu)) {
>>>> +            qemu_kvm_destroy_vcpu(cpu);
>>>> +            qemu_mutex_unlock(&qemu_global_mutex);
>>>> +            return NULL;
>>>> +        }
>>>>      }
>>>>
>>>>      return NULL;
>>>> @@ -929,6 +952,7 @@ static void tcg_exec_all(void);
>>>>  static void *qemu_tcg_cpu_thread_fn(void *arg)
>>>>  {
>>>>      CPUState *cpu = arg;
>>>> +    CPUState *remove_cpu = NULL;
>>>>
>>>>      qemu_tcg_init_cpu_signals();
>>>>      qemu_thread_get_self(cpu->thread);
>>>> @@ -961,6 +985,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>>>              }
>>>>          }
>>>>          qemu_tcg_wait_io_event();
>>>> +        CPU_FOREACH(cpu) {
>>>> +            if (cpu->exit && !cpu_can_run(cpu)) {
>>>> +                remove_cpu = cpu;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +        if (remove_cpu) {
>>>> +            qemu_tcg_destroy_vcpu(remove_cpu);
>>>> +            remove_cpu = NULL;
>>>> +        }
>>>>      }
>>>>
>>>>      return NULL;
>>>> @@ -1316,6 +1350,9 @@ static void tcg_exec_all(void)
>>>>                  break;
>>>>              }
>>>>          } else if (cpu->stop || cpu->stopped) {
>>>> +            if (cpu->exit) {
>>>> +                next_cpu = CPU_NEXT(cpu);
>>>> +            }
>>>>              break;
>>>>          }
>>>>      }
>>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>>> index 174ea36..88e2403 100644
>>>> --- a/include/sysemu/kvm.h
>>>> +++ b/include/sysemu/kvm.h
>>>> @@ -178,6 +178,7 @@ int kvm_has_intx_set_mask(void);
>>>>
>>>>  int kvm_init_vcpu(CPUState *cpu);
>>>>  int kvm_cpu_exec(CPUState *cpu);
>>>> +int kvm_destroy_vcpu(CPUState *cpu);
>>>>
>>>>  #ifdef NEED_CPU_H
>>>>
>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>> index 3ae30ee..25e2a43 100644
>>>> --- a/kvm-all.c
>>>> +++ b/kvm-all.c
>>>> @@ -74,6 +74,12 @@ typedef struct KVMSlot
>>>>
>>>>  typedef struct kvm_dirty_log KVMDirtyLog;
>>>>
>>>> +struct KVMParkedVcpu {
>>>> +    unsigned long vcpu_id;
>>>> +    int kvm_fd;
>>>> +    QLIST_ENTRY(KVMParkedVcpu) node;
>>>> +};
>>>> +
>>>>  struct KVMState
>>>>  {
>>>>      KVMSlot *slots;
>>>> @@ -108,6 +114,7 @@ struct KVMState
>>>>      QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) 
>>>> msi_hashtab[KVM_MSI_HASHTAB_SIZE];
>>>>      bool direct_msi;
>>>>  #endif
>>>> +    QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
>>>>  };
>>>>
>>>>  KVMState *kvm_state;
>>>> @@ -226,6 +233,53 @@ static int kvm_set_user_memory_region(KVMState *s, 
>>>> KVMSlot *slot)
>>>>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>>>>  }
>>>>
>>>> +int kvm_destroy_vcpu(CPUState *cpu)
>>>> +{
>>>> +    KVMState *s = kvm_state;
>>>> +    long mmap_size;
>>>> +    struct KVMParkedVcpu *vcpu = NULL;
>>>> +    int ret = 0;
>>>> +
>>>> +    DPRINTF("kvm_destroy_vcpu\n");
>>>> +
>>>> +    mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>>>> +    if (mmap_size < 0) {
>>>> +        ret = mmap_size;
>>>> +        DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    ret = munmap(cpu->kvm_run, mmap_size);
>>>> +    if (ret < 0) {
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    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);
>>>> +err:
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>>> +{
>>>> +    struct KVMParkedVcpu *cpu;
>>>> +
>>>> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>>>> +        if (cpu->vcpu_id == vcpu_id) {
>>>> +            int kvm_fd;
>>>> +
>>>> +            QLIST_REMOVE(cpu, node);
>>>> +            kvm_fd = cpu->kvm_fd;
>>>> +            g_free(cpu);
>>>> +            return kvm_fd;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>>> +}
>>>> +
>>>>  int kvm_init_vcpu(CPUState *cpu)
>>>>  {
>>>>      KVMState *s = kvm_state;
>>>> @@ -234,7 +288,7 @@ int kvm_init_vcpu(CPUState *cpu)
>>>>
>>>>      DPRINTF("kvm_init_vcpu\n");
>>>>
>>>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu));
>>>> +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>>>>      if (ret < 0) {
>>>>          DPRINTF("kvm_create_vcpu failed\n");
>>>>          goto err;
>>>> @@ -1404,6 +1458,7 @@ int kvm_init(MachineClass *mc)
>>>>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>>>>      QTAILQ_INIT(&s->kvm_sw_breakpoints);
>>>>  #endif
>>>> +    QLIST_INIT(&s->kvm_parked_vcpus);
>>>>      s->vmfd = -1;
>>>>      s->fd = qemu_open("/dev/kvm", O_RDWR);
>>>>      if (s->fd == -1) {
>>>> --
>>>> 1.7.7
>>>>
>>> .
>>>
>>
>>
> .
>