On 10/08/2015 18:04, Frederic Konrad wrote: > On 10/08/2015 17:59, Paolo Bonzini wrote: >> >> On 10/08/2015 17:26, fred.kon...@greensocs.com 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); >>> + >