Re: [Qemu-devel] [PATCH 10/12] cpus-common: Introduce async_safe_run_on_cpu()

[Alex Bennée]

Paolo Bonzini <address@hidden> writes:

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  cpus-common.c     | 25 +++++++++++++++++++++++++
>  include/qom/cpu.h | 12 ++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/cpus-common.c b/cpus-common.c
> index 59c8dc8..88cf5ec 100644
> --- a/cpus-common.c
> +++ b/cpus-common.c
> @@ -144,6 +144,11 @@ void async_run_on_cpu(CPUState *cpu, run_on_cpu_func 
> func, void *data)
>      queue_work_on_cpu(cpu, wi);
>  }
>
> +typedef struct SafeWorkItem {
> +    run_on_cpu_func func;
> +    void *data;
> +} SafeWorkItem;
> +
>  /* Wait for pending exclusive operations to complete.  The exclusive lock
>     must be held.  */
>  static inline void exclusive_idle(void)
> @@ -208,6 +213,26 @@ void cpu_exec_end(CPUState *cpu)
>      qemu_mutex_unlock(&qemu_cpu_list_mutex);
>  }
>
> +static void async_safe_run_on_cpu_fn(CPUState *cpu, void *data)
> +{
> +    SafeWorkItem *w = data;
> +
> +    start_exclusive();
> +    w->func(cpu, w->data);
> +    end_exclusive();
> +    g_free(w);
> +}
> +
> +void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data)
> +{
> +    SafeWorkItem *w = g_new(SafeWorkItem, 1);

OK so I appreciate this approach is a neat way to embed safe work in the
existing queue but does it really offer that much more for yet another
dynamic allocation vs an extra flag to the WorkItem?

In previous iterations I can DoS QEMU with a guest that does heavy
cross-CPU TLB flushing which led to a storm of mini allocations (for the
list and associated structures). This caused the massive memory usage as
the queue backed up.

I appreciate it was a fairly special test case and I introduced other
mitigations in the base patches cputlb code to get around it however it
was the driver for me experimenting with the pre-allocated array for
holding work items.

> +
> +    w->func = func;
> +    w->data = data;
> +
> +    async_run_on_cpu(cpu, async_safe_run_on_cpu_fn, w);
> +}
> +
>  void process_queued_cpu_work(CPUState *cpu)
>  {
>      struct qemu_work_item *wi;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 0e04e8f..54a875e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -663,6 +663,18 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func, 
> void *data);
>  void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data);
>
>  /**
> + * async_safe_run_on_cpu:
> + * @cpu: The vCPU to run on.
> + * @func: The function to be executed.
> + * @data: Data to pass to the function.
> + *
> + * Schedules the function @func for execution on the vCPU @cpu 
> asynchronously,
> + * while all other vCPUs are sleeping.  @func is called with the CPU list 
> lock
> + * taken (and for system emulation the BQL); any other lock can be taken 
> safely.
> + */
> +void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data);
> +
> +/**
>   * qemu_get_cpu:
>   * @index: The address@hidden value of the CPU to obtain.
>   *


--
Alex Bennée