From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35193) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHvkM-0002tL-30 for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:20:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHvkH-0005vu-R9 for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:20:20 -0400 Received: from mail-lf0-x244.google.com ([2a00:1450:4010:c07::244]:35554) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHvkH-0005vi-DX for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:20:17 -0400 Received: by mail-lf0-x244.google.com with SMTP id w130so2307358lfd.2 for ; Tue, 28 Jun 2016 09:20:17 -0700 (PDT) References: <1464986428-6739-1-git-send-email-alex.bennee@linaro.org> <1464986428-6739-15-git-send-email-alex.bennee@linaro.org> From: Sergey Fedorov Message-ID: <5772A3BE.6050207@gmail.com> Date: Tue, 28 Jun 2016 19:20:14 +0300 MIME-Version: 1.0 In-Reply-To: <1464986428-6739-15-git-send-email-alex.bennee@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v3 14/19] tcg: remove global exit_request List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , mttcg@listserver.greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com Cc: mark.burton@greensocs.com, pbonzini@redhat.com, jan.kiszka@siemens.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite On 03/06/16 23:40, Alex Bennée wrote: > The only remaining use of the global exit_request flag is now to ensure > we exit the run_loop when we first start to process pending work. This > is just as easily done by setting the first_cpu->exit_request flag. > > We lightly re-factor the main vCPU thread to ensure cpu->exit_requests > cause us to exit the main loop and process any IO requests that might > come along. > > Signed-off-by: Alex Bennée > --- > cpu-exec-common.c | 2 -- > cpu-exec.c | 10 +++------- > cpus.c | 35 +++++++++++++++++++++++------------ > include/exec/exec-all.h | 2 -- > 4 files changed, 26 insertions(+), 23 deletions(-) > > diff --git a/cpu-exec-common.c b/cpu-exec-common.c > index f42d24a..1b4fb53 100644 > --- a/cpu-exec-common.c > +++ b/cpu-exec-common.c > @@ -23,8 +23,6 @@ > #include "exec/exec-all.h" > #include "exec/memory-internal.h" > > -bool exit_request; > - > /* exit the current TB from a signal handler. The host registers are > restored in a state compatible with the CPU emulator > */ > diff --git a/cpu-exec.c b/cpu-exec.c > index 68e804b..1613c63 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -541,9 +541,8 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, > /* Something asked us to stop executing > * chained TBs; just continue round the main > * loop. Whatever requested the exit will also > - * have set something else (eg exit_request or > - * interrupt_request) which we will handle > - * next time around the loop. But we need to > + * have set something else (eg interrupt_request) which we > + * will handle next time around the loop. But we need to > * ensure the tcg_exit_req read in generated code > * comes before the next read of cpu->exit_request > * or cpu->interrupt_request. > @@ -594,15 +593,12 @@ int cpu_exec(CPUState *cpu) > current_cpu = cpu; > > if (cpu_handle_halt(cpu)) { > + cpu->exit_request = true; Why do we need to do this here? > return EXCP_HALTED; > } > > rcu_read_lock(); > > - if (unlikely(atomic_mb_read(&exit_request))) { > - cpu->exit_request = 1; > - } > - > cc->cpu_exec_enter(cpu); > > /* Calculate difference between guest clock and host clock. > diff --git a/cpus.c b/cpus.c > index a139ad3..a84f02c 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1169,6 +1169,18 @@ static int64_t tcg_get_icount_limit(void) > } > } > > +static void handle_icount_deadline(void) > +{ > + if (use_icount) { > + int64_t deadline = > + qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL); > + > + if (deadline == 0) { > + qemu_clock_notify(QEMU_CLOCK_VIRTUAL); > + } > + } > +} > + > static int tcg_cpu_exec(CPUState *cpu) > { > int ret; > @@ -1276,11 +1288,11 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > timer_mod(kick_timer, TCG_KICK_FREQ); > } > > - /* process any pending work */ > - atomic_mb_set(&exit_request, 1); > - > cpu = first_cpu; > > + /* process any pending work */ > + atomic_mb_set(&cpu->exit_request, 1); Sometimes we use atomic_*() to operate on 'cpu->exit_request', sometimes not. I am wondering if there are some rules about that? Kind regards, Sergey > + > while (1) { > /* Account partial waits to QEMU_CLOCK_VIRTUAL. */ > qemu_account_warp_timer(); > @@ -1289,7 +1301,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > cpu = first_cpu; > } > > - for (; cpu != NULL && !exit_request; cpu = CPU_NEXT(cpu)) { > + while (cpu && !cpu->exit_request) { > atomic_mb_set(&tcg_current_rr_cpu, cpu); > > qemu_clock_enable(QEMU_CLOCK_VIRTUAL, > @@ -1303,22 +1315,21 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > } > } else if (cpu->stop || cpu->stopped) { > break; > + } else if (cpu->exit_request) { > + break; > } > > + cpu = CPU_NEXT(cpu); > } /* for cpu.. */ > /* Does not need atomic_mb_set because a spurious wakeup is okay. */ > atomic_set(&tcg_current_rr_cpu, NULL); > > - /* Pairs with smp_wmb in qemu_cpu_kick. */ > - atomic_mb_set(&exit_request, 0); > + if (cpu && cpu->exit_request) { > + atomic_mb_set(&cpu->exit_request, 0); > + } > > - if (use_icount) { > - int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL); > + handle_icount_deadline(); > > - if (deadline == 0) { > - qemu_clock_notify(QEMU_CLOCK_VIRTUAL); > - } > - } > qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus)); > } > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 31f4c38..7919aac 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -405,6 +405,4 @@ bool memory_region_is_unassigned(MemoryRegion *mr); > /* vl.c */ > extern int singlestep; > > -extern bool exit_request; > - > #endif