| Message ID | 20181019010625.25294-49-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | per-CPU locks | expand |
On 19/10/2018 03:06, Emilio G. Cota wrote: > Soon we will call cpu_has_work without the BQL. > > Cc: David Gibson <david@gibson.dropbear.id.au> > Cc: Alexander Graf <agraf@suse.de> > Cc: qemu-ppc@nongnu.org > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > target/ppc/translate_init.inc.c | 77 +++++++++++++++++++++++++++++++-- > 1 file changed, 73 insertions(+), 4 deletions(-) > Perhaps we should instead define both ->cpu_has_work and ->cpu_has_work_with_iothread_lock members, and move the generic unlock/relock code to common code? Paolo
On Fri, Oct 19, 2018 at 08:58:31 +0200, Paolo Bonzini wrote: > On 19/10/2018 03:06, Emilio G. Cota wrote: > > Soon we will call cpu_has_work without the BQL. > > > > Cc: David Gibson <david@gibson.dropbear.id.au> > > Cc: Alexander Graf <agraf@suse.de> > > Cc: qemu-ppc@nongnu.org > > Signed-off-by: Emilio G. Cota <cota@braap.org> > > --- > > target/ppc/translate_init.inc.c | 77 +++++++++++++++++++++++++++++++-- > > 1 file changed, 73 insertions(+), 4 deletions(-) > > > > Perhaps we should instead define both ->cpu_has_work and > ->cpu_has_work_with_iothread_lock members, and move the generic > unlock/relock code to common code? I like this. How does the appended look? Thanks, Emilio ---8<--- [PATCH] cpu: introduce cpu_has_work_with_iothread_lock It will gain some users soon. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qom/cpu.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/include/qom/cpu.h b/include/qom/cpu.h index ad8859d014..d9e6f5d4d2 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -26,6 +26,7 @@ #include "exec/memattrs.h" #include "qapi/qapi-types-run-state.h" #include "qemu/bitmap.h" +#include "qemu/main-loop.h" #include "qemu/rcu_queue.h" #include "qemu/queue.h" #include "qemu/thread.h" @@ -86,6 +87,8 @@ struct TranslationBlock; * @reset_dump_flags: #CPUDumpFlags to use for reset logging. * @has_work: Callback for checking if there is work to do. Called with the * CPU lock held. + * @has_work_with_iothread_lock: Callback for checking if there is work to do. + * Called with both the BQL and the CPU lock held. * @do_interrupt: Callback for interrupt handling. * @do_unassigned_access: Callback for unassigned access handling. * (this is deprecated: new targets should use do_transaction_failed instead) @@ -157,6 +160,7 @@ typedef struct CPUClass { void (*reset)(CPUState *cpu); int reset_dump_flags; bool (*has_work)(CPUState *cpu); + bool (*has_work_with_iothread_lock)(CPUState *cpu); void (*do_interrupt)(CPUState *cpu); CPUUnassignedAccess do_unassigned_access; void (*do_unaligned_access)(CPUState *cpu, vaddr addr, @@ -774,6 +778,40 @@ CPUState *cpu_create(const char *typename); */ const char *parse_cpu_model(const char *cpu_model); +/* do not call directly; use cpu_has_work instead */ +static inline bool cpu_has_work_bql(CPUState *cpu) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + bool has_cpu_lock = cpu_mutex_locked(cpu); + bool has_bql = qemu_mutex_iothread_locked(); + bool ret; + + if (has_bql) { + if (has_cpu_lock) { + return cc->has_work_with_iothread_lock(cpu); + } + cpu_mutex_lock(cpu); + ret = cc->has_work_with_iothread_lock(cpu); + cpu_mutex_unlock(cpu); + return ret; + } + + if (has_cpu_lock) { + /* avoid deadlock by acquiring the locks in order */ + cpu_mutex_unlock(cpu); + } + qemu_mutex_lock_iothread(); + cpu_mutex_lock(cpu); + + ret = cc->has_work_with_iothread_lock(cpu); + + qemu_mutex_unlock_iothread(); + if (!has_cpu_lock) { + cpu_mutex_unlock(cpu); + } + return ret; +} + /** * cpu_has_work: * @cpu: The vCPU to check. @@ -787,6 +825,10 @@ static inline bool cpu_has_work(CPUState *cpu) CPUClass *cc = CPU_GET_CLASS(cpu); bool ret; + if (cc->has_work_with_iothread_lock) { + return cpu_has_work_bql(cpu); + } + g_assert(cc->has_work); if (cpu_mutex_locked(cpu)) { return cc->has_work(cpu);
On 10/20/18 5:31 PM, Emilio G. Cota wrote: > I like this. How does the appended look? > > Thanks, > > Emilio > ---8<--- > > [PATCH] cpu: introduce cpu_has_work_with_iothread_lock I might just inline cpu_has_work_bql into the one caller. You could even share has_cpu_lock with the code there. r~
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c index 6827db14b6..a206715873 100644 --- a/target/ppc/translate_init.inc.c +++ b/target/ppc/translate_init.inc.c @@ -18,6 +18,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#include "qemu/main-loop.h" #include "disas/bfd.h" #include "exec/gdbstub.h" #include "kvm_ppc.h" @@ -8440,11 +8441,13 @@ static bool ppc_pvr_match_power7(PowerPCCPUClass *pcc, uint32_t pvr) return false; } -static bool cpu_has_work_POWER7(CPUState *cs) +static bool cpu_has_work_POWER7_locked(CPUState *cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + g_assert(qemu_mutex_iothread_locked()); + if (cpu_halted(cs)) { if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) { return false; @@ -8474,6 +8477,21 @@ static bool cpu_has_work_POWER7(CPUState *cs) } } +static bool cpu_has_work_POWER7(CPUState *cs) +{ + if (!qemu_mutex_iothread_locked()) { + bool ret; + + cpu_mutex_unlock(cs); + qemu_mutex_lock_iothread(); + cpu_mutex_lock(cs); + ret = cpu_has_work_POWER7_locked(cs); + qemu_mutex_unlock_iothread(); + return ret; + } + return cpu_has_work_POWER7_locked(cs); +} + POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); @@ -8594,11 +8612,13 @@ static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr) return false; } -static bool cpu_has_work_POWER8(CPUState *cs) +static bool cpu_has_work_POWER8_locked(CPUState *cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + g_assert(qemu_mutex_iothread_locked()); + if (cpu_halted(cs)) { if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) { return false; @@ -8636,6 +8656,21 @@ static bool cpu_has_work_POWER8(CPUState *cs) } } +static bool cpu_has_work_POWER8(CPUState *cs) +{ + if (!qemu_mutex_iothread_locked()) { + bool ret; + + cpu_mutex_unlock(cs); + qemu_mutex_lock_iothread(); + cpu_mutex_lock(cs); + ret = cpu_has_work_POWER8_locked(cs); + qemu_mutex_unlock_iothread(); + return ret; + } + return cpu_has_work_POWER8_locked(cs); +} + POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); @@ -8786,11 +8821,13 @@ static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr) return false; } -static bool cpu_has_work_POWER9(CPUState *cs) +static bool cpu_has_work_POWER9_locked(CPUState *cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + g_assert(qemu_mutex_iothread_locked()); + if (cpu_halted(cs)) { if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) { return false; @@ -8829,6 +8866,21 @@ static bool cpu_has_work_POWER9(CPUState *cs) } } +static bool cpu_has_work_POWER9(CPUState *cs) +{ + if (!qemu_mutex_iothread_locked()) { + bool ret; + + cpu_mutex_unlock(cs); + qemu_mutex_lock_iothread(); + cpu_mutex_lock(cs); + ret = cpu_has_work_POWER9_locked(cs); + qemu_mutex_unlock_iothread(); + return ret; + } + return cpu_has_work_POWER9_locked(cs); +} + POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); @@ -10231,14 +10283,31 @@ static void ppc_cpu_set_pc(CPUState *cs, vaddr value) cpu->env.nip = value; } -static bool ppc_cpu_has_work(CPUState *cs) +static bool ppc_cpu_has_work_locked(CPUState *cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + g_assert(qemu_mutex_iothread_locked()); + return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD); } +static bool ppc_cpu_has_work(CPUState *cs) +{ + if (!qemu_mutex_iothread_locked()) { + bool ret; + + cpu_mutex_unlock(cs); + qemu_mutex_lock_iothread(); + cpu_mutex_lock(cs); + ret = ppc_cpu_has_work_locked(cs); + qemu_mutex_unlock_iothread(); + return ret; + } + return ppc_cpu_has_work_locked(cs); +} + /* CPUClass::reset() */ static void ppc_cpu_reset(CPUState *s) {
Soon we will call cpu_has_work without the BQL. Cc: David Gibson <david@gibson.dropbear.id.au> Cc: Alexander Graf <agraf@suse.de> Cc: qemu-ppc@nongnu.org Signed-off-by: Emilio G. Cota <cota@braap.org> --- target/ppc/translate_init.inc.c | 77 +++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-)