From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8Vfc-0007dt-RY for qemu-devel@nongnu.org; Fri, 26 Jun 2015 11:36:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8VfX-00045d-Vu for qemu-devel@nongnu.org; Fri, 26 Jun 2015 11:36:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8VfX-00045L-HT for qemu-devel@nongnu.org; Fri, 26 Jun 2015 11:35:55 -0400 References: <1435330053-18733-1-git-send-email-fred.konrad@greensocs.com> <1435330053-18733-14-git-send-email-fred.konrad@greensocs.com> From: Paolo Bonzini Message-ID: <558D7155.7040101@redhat.com> Date: Fri, 26 Jun 2015 17:35:49 +0200 MIME-Version: 1.0 In-Reply-To: <1435330053-18733-14-git-send-email-fred.konrad@greensocs.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH V6 13/18] cpu: introduce async_run_safe_work_on_cpu. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: fred.konrad@greensocs.com, qemu-devel@nongnu.org, mttcg@greensocs.com Cc: peter.maydell@linaro.org, a.spyridakis@virtualopensystems.com, mark.burton@greensocs.com, agraf@suse.de, alistair.francis@xilinx.com, guillaume.delbergue@greensocs.com, alex.bennee@linaro.org On 26/06/2015 16:47, fred.konrad@greensocs.com wrote: > diff --git a/cpu-exec.c b/cpu-exec.c > index de256d6..d6442cd 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c Nice solution. However I still have a few questions that need clarification. > @@ -382,6 +382,11 @@ int cpu_exec(CPUArchState *env) > volatile bool have_tb_lock =3D false; > #endif > =20 > + if (async_safe_work_pending()) { > + cpu->exit_request =3D 1; > + return 0; > + } Perhaps move this to cpu_can_run()? > if (cpu->halted) { > if (!cpu_has_work(cpu)) { > return EXCP_HALTED; > diff --git a/cpus.c b/cpus.c > index 5f13d73..aee445a 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -75,7 +75,7 @@ bool cpu_is_stopped(CPUState *cpu) > =20 > bool cpu_thread_is_idle(CPUState *cpu) > { > - if (cpu->stop || cpu->queued_work_first) { > + if (cpu->stop || cpu->queued_work_first || cpu->queued_safe_work_f= irst) { > return false; > } > if (cpu_is_stopped(cpu)) { > @@ -892,6 +892,69 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(= void *data), void *data) > qemu_cpu_kick(cpu); > } > =20 > +void async_run_safe_work_on_cpu(CPUState *cpu, void (*func)(void *data= ), > + void *data) > +{ Do you need a mutex to protect this data structure? I would use one even if not strictly necessary, to avoid introducing new BQL-protected structures. Also, can you add a count of how many such work items exist in the whole system, in order to speed up async_safe_work_pending? > + struct qemu_work_item *wi; > + > + wi =3D g_malloc0(sizeof(struct qemu_work_item)); > + wi->func =3D func; > + wi->data =3D data; > + wi->free =3D true; > + if (cpu->queued_safe_work_first =3D=3D NULL) { > + cpu->queued_safe_work_first =3D wi; > + } else { > + cpu->queued_safe_work_last->next =3D wi; > + } > + cpu->queued_safe_work_last =3D wi; > + wi->next =3D NULL; > + wi->done =3D false; > + > + CPU_FOREACH(cpu) { > + qemu_cpu_kick_thread(cpu); > + } > +} > + > +static void flush_queued_safe_work(CPUState *cpu) > +{ > + struct qemu_work_item *wi; > + CPUState *other_cpu; > + > + if (cpu->queued_safe_work_first =3D=3D NULL) { > + return; > + } > + > + CPU_FOREACH(other_cpu) { > + if (other_cpu->tcg_executing !=3D 0) { This causes the thread to busy wait until everyone has exited, right? Not a big deal, but worth a comment. Paolo > + return; > + } > + } > + > + while ((wi =3D cpu->queued_safe_work_first)) { > + cpu->queued_safe_work_first =3D wi->next; > + wi->func(wi->data); > + wi->done =3D true; > + if (wi->free) { > + g_free(wi); > + } > + } > + cpu->queued_safe_work_last =3D NULL; > + qemu_cond_broadcast(&qemu_work_cond); > +} > + > +bool async_safe_work_pending(void) > +{ > + CPUState *cpu; > + > + CPU_FOREACH(cpu) { > + if (cpu->queued_safe_work_first) { > + return true; > + } > + } > + > + return false; > +} > +