Re: [Qemu-devel] [RFC PATCH V6 13/18] cpu: introduce async_run_safe_work_on_cpu.

[Paolo Bonzini <pbonzini@redhat.com>]

On 26/06/2015 16:47, fred.konrad@greensocs.com wrote:
> diff --git a/cpu-exec.c b/cpu-exec.c
> index de256d6..d6442cd 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c

Nice solution.  However I still have a few questions that need
clarification.

> @@ -382,6 +382,11 @@ int cpu_exec(CPUArchState *env)
>      volatile bool have_tb_lock = false;
>  #endif
>  
> +    if (async_safe_work_pending()) {
> +        cpu->exit_request = 1;
> +        return 0;
> +    }

Perhaps move this to cpu_can_run()?

>      if (cpu->halted) {
>          if (!cpu_has_work(cpu)) {
>              return EXCP_HALTED;
> diff --git a/cpus.c b/cpus.c
> index 5f13d73..aee445a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -75,7 +75,7 @@ bool cpu_is_stopped(CPUState *cpu)
>  
>  bool cpu_thread_is_idle(CPUState *cpu)
>  {
> -    if (cpu->stop || cpu->queued_work_first) {
> +    if (cpu->stop || cpu->queued_work_first || cpu->queued_safe_work_first) {
>          return false;
>      }
>      if (cpu_is_stopped(cpu)) {
> @@ -892,6 +892,69 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
>      qemu_cpu_kick(cpu);
>  }
>  
> +void async_run_safe_work_on_cpu(CPUState *cpu, void (*func)(void *data),
> +                                void *data)
> +{

Do you need a mutex to protect this data structure?  I would use one
even if not strictly necessary, to avoid introducing new BQL-protected
structures.

Also, can you add a count of how many such work items exist in the whole
system, in order to speed up async_safe_work_pending?

> +    struct qemu_work_item *wi;
> +
> +    wi = g_malloc0(sizeof(struct qemu_work_item));
> +    wi->func = func;
> +    wi->data = data;
> +    wi->free = true;
> +    if (cpu->queued_safe_work_first == NULL) {
> +        cpu->queued_safe_work_first = wi;
> +    } else {
> +        cpu->queued_safe_work_last->next = wi;
> +    }
> +    cpu->queued_safe_work_last = wi;
> +    wi->next = NULL;
> +    wi->done = false;
> +
> +    CPU_FOREACH(cpu) {
> +        qemu_cpu_kick_thread(cpu);
> +    }
> +}
> +
> +static void flush_queued_safe_work(CPUState *cpu)
> +{
> +    struct qemu_work_item *wi;
> +    CPUState *other_cpu;
> +
> +    if (cpu->queued_safe_work_first == NULL) {
> +        return;
> +    }
> +
> +    CPU_FOREACH(other_cpu) {
> +        if (other_cpu->tcg_executing != 0) {

This causes the thread to busy wait until everyone has exited, right?
Not a big deal, but worth a comment.

Paolo

> +            return;
> +        }
> +    }
> +
> +    while ((wi = cpu->queued_safe_work_first)) {
> +        cpu->queued_safe_work_first = wi->next;
> +        wi->func(wi->data);
> +        wi->done = true;
> +        if (wi->free) {
> +            g_free(wi);
> +        }
> +    }
> +    cpu->queued_safe_work_last = NULL;
> +    qemu_cond_broadcast(&qemu_work_cond);
> +}
> +
> +bool async_safe_work_pending(void)
> +{
> +    CPUState *cpu;
> +
> +    CPU_FOREACH(cpu) {
> +        if (cpu->queued_safe_work_first) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +