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 <guz.f...@cn.fujitsu.com> 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 <guz.f...@cn.fujitsu.com> 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 <guz.f...@cn.fujitsu.com> 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 <chen.fan.f...@cn.fujitsu.com> >>>>> Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com> >>>>> --- >>>>> 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 >>>>> >>>> . >>>> >>> >>> >> . >> > >