Re: [Qemu-devel] [RFC PATCH V7 01/19] cpus: protect queued_work_* with work_mutex.

[Paolo Bonzini]

On 10/08/2015 18:04, Frederic Konrad wrote:
> On 10/08/2015 17:59, Paolo Bonzini wrote:
>>
>> On 10/08/2015 17:26, address@hidden wrote:
>>>   +    qemu_mutex_lock(&cpu->work_mutex);
>>>       while ((wi = cpu->queued_work_first)) {
>>>           cpu->queued_work_first = wi->next;
>>> +        qemu_mutex_unlock(&cpu->work_mutex);
>>>           wi->func(wi->data);
>>> +        qemu_mutex_lock(&cpu->work_mutex);
>>>           wi->done = true;
>> This should be atomic_mb_set
> 
> Isn't that protected by the mutex?

This use is not protected by the mutex:

@@ -853,6 +855,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), 
void *data)
     cpu->queued_work_last = &wi;
     wi.next = NULL;
     wi.done = false;
+    qemu_mutex_unlock(&cpu->work_mutex);
 
     qemu_cpu_kick(cpu);
     while (!wi.done) {

Paolo

 Or maybe it's used somewhere else?
>>
>>>           if (wi->free) {
>>>               g_free(wi);
>>>           }
>>>       }
>>>       cpu->queued_work_last = NULL;
>> ... and I'm a bit afraid of leaving the state of the list inconsistent,
>> so I'd move this after the cpu->queued_work_first assignment.  Otherwise
>> the patch looks good, I'm queuing it for 2.5.
>>
>> Paolo
>>
>>> +    qemu_mutex_unlock(&cpu->work_mutex);
>>> +
>