From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZOpaX-0000ym-3S for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:06:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZOpaS-0003I7-MW for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:06:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58523) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZOpaS-0003I1-H0 for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:06:08 -0400 References: <1439220437-23957-1-git-send-email-fred.konrad@greensocs.com> <1439220437-23957-2-git-send-email-fred.konrad@greensocs.com> <55C8CA70.7090809@redhat.com> <55C8CB83.1020608@greensocs.com> From: Paolo Bonzini Message-ID: <55C8CBEA.5020008@redhat.com> Date: Mon, 10 Aug 2015 18:06:02 +0200 MIME-Version: 1.0 In-Reply-To: <55C8CB83.1020608@greensocs.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH V7 01/19] cpus: protect queued_work_* with work_mutex. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frederic Konrad , qemu-devel@nongnu.org, mttcg@greensocs.com Cc: mark.burton@greensocs.com, alex.bennee@linaro.org, a.rigo@virtualopensystems.com, guillaume.delbergue@greensocs.com 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.konrad@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); >>> + >