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

[Anshul Makkar]

Hi Gu,

Thanks for clarifying.

Ah I missed that bit of the patch. Sorry about that and for making noise.

Yes, now cpu-hotplug and unplug works fine. Next week I plan to run a
series of automated and stress test. Will keep the group posted about
the results.

Thanks
Anshul Makkar

On Fri, Aug 1, 2014 at 6:42 AM, Gu Zheng <address@hidden> wrote:
> 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
>>>>>
>>>> .
>>>>
>>>
>>>
>> .
>>
>
>