Re: [RFC][PATCH 2/6] cpus: release allocated vcpu objects and exit vcpu thread

Re: [RFC][PATCH 2/6] cpus: release allocated vcpu objects and exit vcpu thread

[Andreas Färber]

Am 29.08.2013 04:09, schrieb Chen Fan:
> After ACPI get a signal to eject a vcpu, then it will notify
> the vcpu thread of needing to exit, before the vcpu exiting,
> will release the vcpu related objects.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  cpus.c               | 36 ++++++++++++++++++++++++++++++++++++
>  hw/acpi/piix4.c      | 16 ++++++++++++----
>  include/qom/cpu.h    |  9 +++++++++
>  include/sysemu/kvm.h |  1 +
>  kvm-all.c            | 26 ++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 70cc617..6b793cb 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -697,6 +697,30 @@ 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)
> +{
> +    CPUState *pcpu, *pcpu1;
> +
> +    pcpu = first_cpu;
> +    pcpu1 = NULL;
> +
> +    while (pcpu) {
> +        if (pcpu == cpu && pcpu1) {
> +            pcpu1->next_cpu = cpu->next_cpu;
> +            break;
> +        }
> +        pcpu1 = pcpu;
> +        pcpu = pcpu->next_cpu;
> +    }

No, no, no. :) I specifically posted the "QOM CPUState, part 12" series
early to avoid exactly such code appearing! Give me a few minutes to
apply that to qom-cpu and then please rebase your work on
git://github.com/afaerber/qemu-cpu.git qom-cpu
using QTAILQ macro and --subject-prefix="RFC qom-cpu v2" for the next
version of the series.

Also, why is this only in the KVM code path? Isn't this needed for TCG
as well?

> +
> +    if (kvm_destroy_vcpu(cpu) < 0) {
> +        fprintf(stderr, "kvm_destroy_vcpu failed.\n");
> +        exit(1);
> +    }
> +
> +    qdev_free(DEVICE(X86_CPU(cpu)));

DEVICE(cpu) should be sufficient.

> +}
> +
>  static void flush_queued_work(CPUState *cpu)
>  {
>      struct qemu_work_item *wi;
> @@ -788,6 +812,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;
> @@ -1080,6 +1109,13 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>      }
>  }
>  
> +void qemu_down_vcpu(CPUState *cpu)
> +{
> +    cpu->stop = true;
> +    cpu->exit = true;
> +    qemu_cpu_kick(cpu);
> +}
> +
>  void qemu_init_vcpu(CPUState *cpu)
>  {
>      cpu->nr_cores = smp_cores;
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 1aaa7a4..44bc809 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -611,10 +611,18 @@ static const MemoryRegionOps piix4_pci_ops = {
>      },
>  };
>  
> -static void acpi_piix_eject_vcpu(int64_t cpuid)
> +static void acpi_piix_eject_vcpu(PIIX4PMState *s, int64_t cpuid)
>  {
> -    /* TODO: eject a vcpu, release allocated vcpu and exit the vcpu pthread.  */
> -    PIIX4_DPRINTF("vcpu: %" PRIu64 " need to be ejected.\n", cpuid);
> +    CPUStatus *cpus = &s->gpe_cpu;
> +    CPUState *cs = NULL;
> +
> +    cs = qemu_get_cpu(cpuid);

Are you sure this is correct as 0-based index? Igor?

> +    if (cs == NULL) {
> +        return;
> +    }
> +
> +    cpus->old_sts[cpuid / 8] &= ~(1 << (cpuid % 8));
> +    qemu_down_vcpu(cs);
>  }
>  
>  static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> @@ -647,7 +655,7 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
>      }
>  
>      if (cpuid != 0) {
> -        acpi_piix_eject_vcpu(cpuid);
> +        acpi_piix_eject_vcpu(s, cpuid);
>      }
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 3e49936..fa8ec8a 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -180,6 +180,7 @@ struct CPUState {
>      bool created;
>      bool stop;
>      bool stopped;
> +    bool exit;
>      volatile sig_atomic_t exit_request;
>      volatile sig_atomic_t tcg_exit_req;
>      uint32_t interrupt_request;
> @@ -489,6 +490,14 @@ void cpu_exit(CPUState *cpu);
>  void cpu_resume(CPUState *cpu);
>  
>  /**
> + * qemu_down_vcpu:
> + * @cpu: The vCPU will to down.
> + *
> + * Down a vCPU.
> + */
> +void qemu_down_vcpu(CPUState *cpu);

The naming and documentation sounds wrong language-wise to me, but I am
not a native speaker either. Maybe "tear down" instead of "down"? Or
simply qemu_request_vcpu_removal() or something like that?

> +
> +/**
>   * qemu_init_vcpu:
>   * @cpu: The vCPU to initialize.
>   *
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index de74411..fd85605 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -158,6 +158,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 716860f..fda3601 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -225,6 +225,32 @@ static void kvm_reset_vcpu(void *opaque)
>      kvm_arch_reset_vcpu(cpu);
>  }
>  
> +int kvm_destroy_vcpu(CPUState *cpu)
> +{
> +    KVMState *s = kvm_state;
> +    long mmap_size;
> +    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;
> +    }
> +
> +    close(cpu->kvm_fd);
> +
> +err:
> +    return ret;
> +}

I always assumed we'd need a new ioctl to inform the kernel about our
intent to remove a vCPU before doing such cleanups? CC'ing kvm list.

Regards,
Andreas

> +
>  int kvm_init_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC][PATCH 2/6] cpus: release allocated vcpu objects and exit vcpu thread

[chenfan]

On Thu, 2013-08-29 at 07:10 +0200, Andreas Färber wrote:
> Am 29.08.2013 04:09, schrieb Chen Fan:
> > After ACPI get a signal to eject a vcpu, then it will notify
> > the vcpu thread of needing to exit, before the vcpu exiting,
> > will release the vcpu related objects.
> > 
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  cpus.c               | 36 ++++++++++++++++++++++++++++++++++++
> >  hw/acpi/piix4.c      | 16 ++++++++++++----
> >  include/qom/cpu.h    |  9 +++++++++
> >  include/sysemu/kvm.h |  1 +
> >  kvm-all.c            | 26 ++++++++++++++++++++++++++
> >  5 files changed, 84 insertions(+), 4 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 70cc617..6b793cb 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -697,6 +697,30 @@ 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)
> > +{
> > +    CPUState *pcpu, *pcpu1;
> > +
> > +    pcpu = first_cpu;
> > +    pcpu1 = NULL;
> > +
> > +    while (pcpu) {
> > +        if (pcpu == cpu && pcpu1) {
> > +            pcpu1->next_cpu = cpu->next_cpu;
> > +            break;
> > +        }
> > +        pcpu1 = pcpu;
> > +        pcpu = pcpu->next_cpu;
> > +    }
> 
> No, no, no. :) I specifically posted the "QOM CPUState, part 12" series
> early to avoid exactly such code appearing! Give me a few minutes to
> apply that to qom-cpu and then please rebase your work on
> git://github.com/afaerber/qemu-cpu.git qom-cpu
> using QTAILQ macro and --subject-prefix="RFC qom-cpu v2" for the next
> version of the series.
OK. Thanks.
> 
> Also, why is this only in the KVM code path? Isn't this needed for TCG
> as well?
I will add work for TCG support later.
> 
> > +
> > +    if (kvm_destroy_vcpu(cpu) < 0) {
> > +        fprintf(stderr, "kvm_destroy_vcpu failed.\n");
> > +        exit(1);
> > +    }
> > +
> > +    qdev_free(DEVICE(X86_CPU(cpu)));
> 
> DEVICE(cpu) should be sufficient.
> 
> > +}
> > +
> >  static void flush_queued_work(CPUState *cpu)
> >  {
> >      struct qemu_work_item *wi;
> > @@ -788,6 +812,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;
> > @@ -1080,6 +1109,13 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
> >      }
> >  }
> >  
> > +void qemu_down_vcpu(CPUState *cpu)
> > +{
> > +    cpu->stop = true;
> > +    cpu->exit = true;
> > +    qemu_cpu_kick(cpu);
> > +}
> > +
> >  void qemu_init_vcpu(CPUState *cpu)
> >  {
> >      cpu->nr_cores = smp_cores;
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 1aaa7a4..44bc809 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -611,10 +611,18 @@ static const MemoryRegionOps piix4_pci_ops = {
> >      },
> >  };
> >  
> > -static void acpi_piix_eject_vcpu(int64_t cpuid)
> > +static void acpi_piix_eject_vcpu(PIIX4PMState *s, int64_t cpuid)
> >  {
> > -    /* TODO: eject a vcpu, release allocated vcpu and exit the vcpu pthread.  */
> > -    PIIX4_DPRINTF("vcpu: %" PRIu64 " need to be ejected.\n", cpuid);
> > +    CPUStatus *cpus = &s->gpe_cpu;
> > +    CPUState *cs = NULL;
> > +
> > +    cs = qemu_get_cpu(cpuid);
> 
> Are you sure this is correct as 0-based index? Igor?
> 
> > +    if (cs == NULL) {
> > +        return;
> > +    }
> > +
> > +    cpus->old_sts[cpuid / 8] &= ~(1 << (cpuid % 8));
> > +    qemu_down_vcpu(cs);
> >  }
> >  
> >  static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> > @@ -647,7 +655,7 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> >      }
> >  
> >      if (cpuid != 0) {
> > -        acpi_piix_eject_vcpu(cpuid);
> > +        acpi_piix_eject_vcpu(s, cpuid);
> >      }
> >  }
> >  
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 3e49936..fa8ec8a 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -180,6 +180,7 @@ struct CPUState {
> >      bool created;
> >      bool stop;
> >      bool stopped;
> > +    bool exit;
> >      volatile sig_atomic_t exit_request;
> >      volatile sig_atomic_t tcg_exit_req;
> >      uint32_t interrupt_request;
> > @@ -489,6 +490,14 @@ void cpu_exit(CPUState *cpu);
> >  void cpu_resume(CPUState *cpu);
> >  
> >  /**
> > + * qemu_down_vcpu:
> > + * @cpu: The vCPU will to down.
> > + *
> > + * Down a vCPU.
> > + */
> > +void qemu_down_vcpu(CPUState *cpu);
> 
> The naming and documentation sounds wrong language-wise to me, but I am
> not a native speaker either. Maybe "tear down" instead of "down"? Or
> simply qemu_request_vcpu_removal() or something like that?
> 
> > +
> > +/**
> >   * qemu_init_vcpu:
> >   * @cpu: The vCPU to initialize.
> >   *
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index de74411..fd85605 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -158,6 +158,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 716860f..fda3601 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -225,6 +225,32 @@ static void kvm_reset_vcpu(void *opaque)
> >      kvm_arch_reset_vcpu(cpu);
> >  }
> >  
> > +int kvm_destroy_vcpu(CPUState *cpu)
> > +{
> > +    KVMState *s = kvm_state;
> > +    long mmap_size;
> > +    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;
> > +    }
> > +
> > +    close(cpu->kvm_fd);
> > +
> > +err:
> > +    return ret;
> > +}
> 
> I always assumed we'd need a new ioctl to inform the kernel about our
> intent to remove a vCPU before doing such cleanups? CC'ing kvm list.
I think, In kvm.h, there is not one '_IO' number reserved for removing a
vCPU. like KVM_REMOVE_VCPU or other. maybe the original idea for
releasing a vCPU does not need to use a new ioctl to free the allocated
vCPU.

Thanks for your review.
Chen

> 
> Regards,
> Andreas
> 
> > +
> >  int kvm_init_vcpu(CPUState *cpu)
> >  {
> >      KVMState *s = kvm_state;
> > 
> 
>