Re: [Qemu-devel] [RFC PATCH V2 3/3] cpus: introduce async_run_safe_work_on_cpu.

[Paolo Bonzini]

On 13/07/2015 18:20, Alex Bennée wrote:
>> +static void qemu_cpu_kick_thread(CPUState *cpu)
>> +{
>> +#ifndef _WIN32
>> +    int err;
>> +
>> +    err = pthread_kill(cpu->thread->thread, SIG_IPI);
>> +    if (err) {
>> +        fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>> +        exit(1);
>> +    }
>> +#else /* _WIN32 */
>> +    if (!qemu_cpu_is_self(cpu)) {
>> +        CONTEXT tcgContext;
>> +
>> +        if (SuspendThread(cpu->hThread) == (DWORD)-1) {
>> +            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
>> +                    GetLastError());
>> +            exit(1);
>> +        }
>> +
>> +        /* On multi-core systems, we are not sure that the thread is 
>> actually
>> +         * suspended until we can get the context.
>> +         */
>> +        tcgContext.ContextFlags = CONTEXT_CONTROL;
>> +        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
>> +            continue;
>> +        }
>> +
>> +        cpu_signal(0);
>> +
>> +        if (ResumeThread(cpu->hThread) == (DWORD)-1) {
>> +            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
>> +                    GetLastError());
>> +            exit(1);
>> +        }
>> +    }
>> +#endif
> 
> I'm going to go out on a limb and guess these sort of implementation
> specifics should be in the posix/win32 utility files, that is unless
> Glib abstracts enough of it for us.

As you found later, this part of the patch is just moving code around.
However, getting ultimately rid of this is basically the reason why I'm
interested in MTTCG. :)

>> +}
>> +
>>  void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
>>  {
>>      struct qemu_work_item wi;
>> @@ -894,6 +933,76 @@ 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)
>> +{
>> +    struct qemu_work_item *wi;
>> +
>> +    wi = g_malloc0(sizeof(struct qemu_work_item));
>> +    wi->func = func;
>> +    wi->data = data;
> 
> Is there anything that prevents the user calling the function with the
> same payload for multiple CPUs?

Why?  The data could be read only, or could be known to outlive the CPUs.

>> +    wi->free = true;
>> +
>> +    qemu_mutex_lock(&cpu->work_mutex);
>> +    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;
> 
> I'm surprised we haven't added some helpers to the qemu_work_queue API
> for all this identical boilerplate but whatever...

Yes, it should be using QSIMPLEQ.  Can be done later.

It is indeed reasonable, but it should be used with great care.

Paolo