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

[Gu Zheng]

Hi Anshul,

I rebased these two parts on latest QEMU tree (no functional change), and
will send out later.
So if you like, please refer to the new one, it will be easy for you to
review and test.

Thanks,
Gu
On 08/01/2014 11:34 PM, Anshul Makkar wrote:

> 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
>>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>> .
>>>
>>
>>
> .
>