| Message ID | 1464986428-6739-14-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/06/2016 22:40, Alex Bennée wrote: > +/* Kick the currently round-robin scheduled vCPU */ > +static void qemu_cpu_kick_rr_cpu(void) > +{ > + CPUState *cpu; > + do { > + cpu = atomic_mb_read(&tcg_current_rr_cpu); > + if (cpu) { > + cpu_exit(cpu); > + } > + } while (cpu != atomic_mb_read(&tcg_current_rr_cpu)); > +} Interesting way to get rid of the global exit_request. I like it, but could you get stuck on NULL tcg_current_rr_cpu now? I think you should redo these two patches like this: - rename qemu_cpu_kick_no_halt to qemu_cpu_kick_rr_cpu and tcg_current_cpu to tcg_current_rr_cpu; possibly move functions around - extract handle_icount_deadline - then everything else in patches 13 and 14, squashed Paolo
On 06/06/2016 18:05, Alex Bennée wrote: > > - rename qemu_cpu_kick_no_halt to qemu_cpu_kick_rr_cpu and > > tcg_current_cpu to tcg_current_rr_cpu; possibly move functions around > > That is this patch isn't it? There's some renaming left in patch 14 which complicates things. Paolo >> > - extract handle_icount_deadline > Sure I can do that separately. > >> > >> > - then everything else in patches 13 and 14, squashed > I was wondering after I posted if I should split all the current_cpu > stuff out as I don't think I need it straight away... >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 06/06/2016 18:05, Alex Bennée wrote: >> > - rename qemu_cpu_kick_no_halt to qemu_cpu_kick_rr_cpu and >> > tcg_current_cpu to tcg_current_rr_cpu; possibly move functions around >> >> That is this patch isn't it? > > There's some renaming left in patch 14 which complicates things. Is there? It looks all exit_request related to me. > > Paolo > >>> > - extract handle_icount_deadline >> Sure I can do that separately. >> >>> > >>> > - then everything else in patches 13 and 14, squashed >> I was wondering after I posted if I should split all the current_cpu >> stuff out as I don't think I need it straight away... >> -- Alex Bennée
On 06/06/2016 19:26, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 06/06/2016 18:05, Alex Bennée wrote: >>>> - rename qemu_cpu_kick_no_halt to qemu_cpu_kick_rr_cpu and >>>> tcg_current_cpu to tcg_current_rr_cpu; possibly move functions around >>> >>> That is this patch isn't it? >> >> There's some renaming left in patch 14 which complicates things. > > Is there? It looks all exit_request related to me. You're right sorry, it's the other way round. It's not that there's some renaming in patch 14, it's that there are semantic changes in this patch. I.e. this: - atomic_mb_set(&tcg_current_cpu, cpu); rcu_read_lock(); if (unlikely(atomic_mb_read(&exit_request))) { @@ -658,7 +657,5 @@ int cpu_exec(CPUState *cpu) /* fail safe : never use current_cpu outside cpu_exec() */ current_cpu = NULL; - /* Does not need atomic_mb_set because a spurious wakeup is okay. */ - atomic_set(&tcg_current_cpu, NULL); and the while-loop in qemu_cpu_kick_rr_cpu. Paolo >> >> Paolo >> >>>>> - extract handle_icount_deadline >>> Sure I can do that separately. >>> >>>>> >>>>> - then everything else in patches 13 and 14, squashed >>> I was wondering after I posted if I should split all the current_cpu >>> stuff out as I don't think I need it straight away... >>> > > > -- > Alex Bennée > >
Alex Bennée <alex.bennee@linaro.org> writes: > ..and make the definition local to cpus. In preparation for MTTCG the > concept of a global tcg_current_cpu will no longer make sense. However > we still need to keep track of it in the single-threaded case to be able > to exit quickly when required. > > qemu_cpu_kick_no_halt() moves and becomes qemu_cpu_kick_rr_cpu() to > emphasise its use-case. qemu_cpu_kick now kicks the relevant cpu as > well as qemu_kick_rr_cpu() which will become a no-op in MTTCG. I detected a failure case that a -smp 2 guest doesn't cleanly shutdown in RR mode. My command line: running command: ['/home/alex/lsrc/qemu/qemu.git/arm-softmmu/qemu-system-arm', '-machine', 'type=virt', '-display', 'none', '-smp', '1', '-m', '4096', '-cpu', 'cortex-a15', '-serial', 'telnet:127.0.0.1:4444', '-monitor', 'stdio', '-netdev', 'user,id=unet,hostfwd=tcp::2222-:22', '-device', 'virtio-net-device,netdev=unet', '-drive', 'file=/home/alex/lsrc/qemu/images/jessie-arm32.qcow2,id=myblock,index=0,if=none', '-device', 'virtio-blk-device,drive=myblock', '-append', 'console=ttyAMA0 systemd.unit=benchmark.service root=/dev/vda1', '-kernel', '/home/alex/lsrc/qemu/images/aarch32-current-linux-kernel-only.img', '-smp', '2'] (notty=False, with timeout) command timed out! run 1: ret=-1 (FALSE), time=20.000228 (0/1) 1 fails (N.B. the duplicate -smp is just an artefact of my shell script glue. The -smp 2 is the final smp that gets acted on. The benchmark.service is just a simple systemd target that shuts the image down again.) > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > cpu-exec-common.c | 1 - > cpu-exec.c | 3 --- > cpus.c | 41 ++++++++++++++++++++++++----------------- > include/exec/exec-all.h | 2 -- > 4 files changed, 24 insertions(+), 23 deletions(-) > > diff --git a/cpu-exec-common.c b/cpu-exec-common.c > index 132cd03..f42d24a 100644 > --- a/cpu-exec-common.c > +++ b/cpu-exec-common.c > @@ -24,7 +24,6 @@ > #include "exec/memory-internal.h" > > bool exit_request; > -CPUState *tcg_current_cpu; > > /* 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 ae81e8e..68e804b 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -597,7 +597,6 @@ int cpu_exec(CPUState *cpu) > return EXCP_HALTED; > } > > - atomic_mb_set(&tcg_current_cpu, cpu); > rcu_read_lock(); > > if (unlikely(atomic_mb_read(&exit_request))) { > @@ -658,7 +657,5 @@ int cpu_exec(CPUState *cpu) > /* fail safe : never use current_cpu outside cpu_exec() */ > current_cpu = NULL; > > - /* Does not need atomic_mb_set because a spurious wakeup is okay. */ > - atomic_set(&tcg_current_cpu, NULL); > return ret; > } > diff --git a/cpus.c b/cpus.c > index 12e04c9..a139ad3 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -942,6 +942,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data) > { > struct qemu_work_item wi; > > + /* Always true when using tcg RR scheduling from a vCPU context */ > if (qemu_cpu_is_self(cpu)) { > func(data); > return; > @@ -1216,15 +1217,29 @@ static int tcg_cpu_exec(CPUState *cpu) > * This is done explicitly rather than relying on side-effects > * elsewhere. > */ > -static void qemu_cpu_kick_no_halt(void); > #define TCG_KICK_FREQ (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + \ > NANOSECONDS_PER_SECOND / 10) > > +/* only used in single-thread tcg mode */ > +static CPUState *tcg_current_rr_cpu; > + > +/* Kick the currently round-robin scheduled vCPU */ > +static void qemu_cpu_kick_rr_cpu(void) > +{ > + CPUState *cpu; > + do { > + cpu = atomic_mb_read(&tcg_current_rr_cpu); > + if (cpu) { > + cpu_exit(cpu); > + } > + } while (cpu != atomic_mb_read(&tcg_current_rr_cpu)); > +} > + > static void kick_tcg_thread(void *opaque) > { > QEMUTimer *self = *(QEMUTimer **) opaque; > timer_mod(self, TCG_KICK_FREQ); > - qemu_cpu_kick_no_halt(); > + qemu_cpu_kick_rr_cpu(); > } > > static void *qemu_tcg_cpu_thread_fn(void *arg) > @@ -1275,6 +1290,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > } > > for (; cpu != NULL && !exit_request; cpu = CPU_NEXT(cpu)) { > + atomic_mb_set(&tcg_current_rr_cpu, cpu); > > qemu_clock_enable(QEMU_CLOCK_VIRTUAL, > (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0); > @@ -1290,6 +1306,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > } > > } /* 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); > @@ -1326,24 +1344,13 @@ static void qemu_cpu_kick_thread(CPUState *cpu) > #endif > } > > -static void qemu_cpu_kick_no_halt(void) > -{ > - CPUState *cpu; > - /* Ensure whatever caused the exit has reached the CPU threads before > - * writing exit_request. > - */ > - atomic_mb_set(&exit_request, 1); > - cpu = atomic_mb_read(&tcg_current_cpu); > - if (cpu) { > - cpu_exit(cpu); > - } > -} > - > void qemu_cpu_kick(CPUState *cpu) > { > qemu_cond_broadcast(cpu->halt_cond); > if (tcg_enabled()) { > - qemu_cpu_kick_no_halt(); > + cpu_exit(cpu); > + /* Also ensure current RR cpu is kicked */ > + qemu_cpu_kick_rr_cpu(); > } else { > qemu_cpu_kick_thread(cpu); > } > @@ -1384,7 +1391,7 @@ void qemu_mutex_lock_iothread(void) > atomic_dec(&iothread_requesting_mutex); > } else { > if (qemu_mutex_trylock(&qemu_global_mutex)) { > - qemu_cpu_kick_no_halt(); > + qemu_cpu_kick_rr_cpu(); > qemu_mutex_lock(&qemu_global_mutex); > } > atomic_dec(&iothread_requesting_mutex); > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index e30f02b..31f4c38 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -405,8 +405,6 @@ bool memory_region_is_unassigned(MemoryRegion *mr); > /* vl.c */ > extern int singlestep; > > -/* cpu-exec.c, accessed with atomic_mb_read/atomic_mb_set */ > -extern CPUState *tcg_current_cpu; > extern bool exit_request; > > #endif -- Alex Bennée
diff --git a/cpu-exec-common.c b/cpu-exec-common.c index 132cd03..f42d24a 100644 --- a/cpu-exec-common.c +++ b/cpu-exec-common.c @@ -24,7 +24,6 @@ #include "exec/memory-internal.h" bool exit_request; -CPUState *tcg_current_cpu; /* 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 ae81e8e..68e804b 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -597,7 +597,6 @@ int cpu_exec(CPUState *cpu) return EXCP_HALTED; } - atomic_mb_set(&tcg_current_cpu, cpu); rcu_read_lock(); if (unlikely(atomic_mb_read(&exit_request))) { @@ -658,7 +657,5 @@ int cpu_exec(CPUState *cpu) /* fail safe : never use current_cpu outside cpu_exec() */ current_cpu = NULL; - /* Does not need atomic_mb_set because a spurious wakeup is okay. */ - atomic_set(&tcg_current_cpu, NULL); return ret; } diff --git a/cpus.c b/cpus.c index 12e04c9..a139ad3 100644 --- a/cpus.c +++ b/cpus.c @@ -942,6 +942,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data) { struct qemu_work_item wi; + /* Always true when using tcg RR scheduling from a vCPU context */ if (qemu_cpu_is_self(cpu)) { func(data); return; @@ -1216,15 +1217,29 @@ static int tcg_cpu_exec(CPUState *cpu) * This is done explicitly rather than relying on side-effects * elsewhere. */ -static void qemu_cpu_kick_no_halt(void); #define TCG_KICK_FREQ (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + \ NANOSECONDS_PER_SECOND / 10) +/* only used in single-thread tcg mode */ +static CPUState *tcg_current_rr_cpu; + +/* Kick the currently round-robin scheduled vCPU */ +static void qemu_cpu_kick_rr_cpu(void) +{ + CPUState *cpu; + do { + cpu = atomic_mb_read(&tcg_current_rr_cpu); + if (cpu) { + cpu_exit(cpu); + } + } while (cpu != atomic_mb_read(&tcg_current_rr_cpu)); +} + static void kick_tcg_thread(void *opaque) { QEMUTimer *self = *(QEMUTimer **) opaque; timer_mod(self, TCG_KICK_FREQ); - qemu_cpu_kick_no_halt(); + qemu_cpu_kick_rr_cpu(); } static void *qemu_tcg_cpu_thread_fn(void *arg) @@ -1275,6 +1290,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) } for (; cpu != NULL && !exit_request; cpu = CPU_NEXT(cpu)) { + atomic_mb_set(&tcg_current_rr_cpu, cpu); qemu_clock_enable(QEMU_CLOCK_VIRTUAL, (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0); @@ -1290,6 +1306,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) } } /* 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); @@ -1326,24 +1344,13 @@ static void qemu_cpu_kick_thread(CPUState *cpu) #endif } -static void qemu_cpu_kick_no_halt(void) -{ - CPUState *cpu; - /* Ensure whatever caused the exit has reached the CPU threads before - * writing exit_request. - */ - atomic_mb_set(&exit_request, 1); - cpu = atomic_mb_read(&tcg_current_cpu); - if (cpu) { - cpu_exit(cpu); - } -} - void qemu_cpu_kick(CPUState *cpu) { qemu_cond_broadcast(cpu->halt_cond); if (tcg_enabled()) { - qemu_cpu_kick_no_halt(); + cpu_exit(cpu); + /* Also ensure current RR cpu is kicked */ + qemu_cpu_kick_rr_cpu(); } else { qemu_cpu_kick_thread(cpu); } @@ -1384,7 +1391,7 @@ void qemu_mutex_lock_iothread(void) atomic_dec(&iothread_requesting_mutex); } else { if (qemu_mutex_trylock(&qemu_global_mutex)) { - qemu_cpu_kick_no_halt(); + qemu_cpu_kick_rr_cpu(); qemu_mutex_lock(&qemu_global_mutex); } atomic_dec(&iothread_requesting_mutex); diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index e30f02b..31f4c38 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -405,8 +405,6 @@ bool memory_region_is_unassigned(MemoryRegion *mr); /* vl.c */ extern int singlestep; -/* cpu-exec.c, accessed with atomic_mb_read/atomic_mb_set */ -extern CPUState *tcg_current_cpu; extern bool exit_request; #endif
..and make the definition local to cpus. In preparation for MTTCG the concept of a global tcg_current_cpu will no longer make sense. However we still need to keep track of it in the single-threaded case to be able to exit quickly when required. qemu_cpu_kick_no_halt() moves and becomes qemu_cpu_kick_rr_cpu() to emphasise its use-case. qemu_cpu_kick now kicks the relevant cpu as well as qemu_kick_rr_cpu() which will become a no-op in MTTCG. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- cpu-exec-common.c | 1 - cpu-exec.c | 3 --- cpus.c | 41 ++++++++++++++++++++++++----------------- include/exec/exec-all.h | 2 -- 4 files changed, 24 insertions(+), 23 deletions(-)