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

[Paolo Bonzini]

On 05/09/2016 17:08, Alex Bennée wrote:
> 
> 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?

Actually I had it that way in the "first version" (the one that I
promised last Monday).  The questions are:

1) is it so different to have 1 vs. 2 mini allocations

2) is this a fast path

but it's no big deal, I can certainly change back.

Paolo

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