| Message ID | 20200819182856.4893-1-robert.foley@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path | expand |
On Wed, 19 Aug 2020 14:28:49 -0400 Robert Foley <robert.foley@linaro.org> wrote: > The purpose of this change is to set the groundwork > so that an arch could move towards removing > the BQL from the cpu_handle_interrupt/exception paths. > > The BQL is a bottleneck in scaling to more cores. > And this cpu_handle_interrupt/exception path is one of > the key BQL users as measured by the QEMU sync profiling (qsp). > > As the first step in removing the BQL from this path, we will make > changes to the core/common functions of cpu_handle_interrupt/exception > to drop the holding of the BQL. The holding of the BQL is pushed down > to the per-arch implementation code. I have only skimmed the patches I was cc:ed on so far, but the series seems sane to me in general. > > This patch goes through several transitions of the code in order to > maintain correctness (bisectability). In order to maintain > bisectability across these steps some patches need to touch many > files across different arches, however most of the changes are trivial. > > The general order of the changes is below where each step > represents one patch. > > 1) rename all *_do_interrupt functions to *_do_interrupt_locked I'm wondering whether this renaming could be done in an automated way (e.g. via Coccinelle). Reviewing the method for the renaming is often easier than looking at a lot of similar code patterns. > > 2) add a new function *_do_interrupt that takes the BQL and calls > *_do_interrupt_locked, point ->do_interrupt to it, and remove > the BQL from cpu-exec.c's cpu_handle_exception. > > 3) modify the BQL critical sections around > ->cpu_exec_interrupt, so that the BQL critical section covers just the > call to ->cpu_exec_interrupt. > > 4/5) same as 1/2 for ->cpu_exec_interrupt. This removes the BQL > from cpu_handle_exception. The method of doing this in steps looks fine, although the patches produced are a bit unwieldy -- that's something we have to live with, I guess.
On Fri, 21 Aug 2020 at 06:56, Cornelia Huck <cohuck@redhat.com> wrote: > > > > As the first step in removing the BQL from this path, we will make > > changes to the core/common functions of cpu_handle_interrupt/exception > > to drop the holding of the BQL. The holding of the BQL is pushed down > > to the per-arch implementation code. > > I have only skimmed the patches I was cc:ed on so far, but the series > seems sane to me in general. Thanks for the feedback ! > > > > > This patch goes through several transitions of the code in order to > > maintain correctness (bisectability). In order to maintain > > bisectability across these steps some patches need to touch many > > files across different arches, however most of the changes are trivial. > > > > The general order of the changes is below where each step > > represents one patch. > > > > 1) rename all *_do_interrupt functions to *_do_interrupt_locked > > I'm wondering whether this renaming could be done in an automated way > (e.g. via Coccinelle). Reviewing the method for the renaming is often > easier than looking at a lot of similar code patterns. Good point. We will look into this. Thanks, -Rob Foley > > > > > 2) add a new function *_do_interrupt that takes the BQL and calls > > *_do_interrupt_locked, point ->do_interrupt to it, and remove > > the BQL from cpu-exec.c's cpu_handle_exception. > > > > 3) modify the BQL critical sections around > > ->cpu_exec_interrupt, so that the BQL critical section covers just the > > call to ->cpu_exec_interrupt. > > > > 4/5) same as 1/2 for ->cpu_exec_interrupt. This removes the BQL > > from cpu_handle_exception. > > The method of doing this in steps looks fine, although the patches > produced are a bit unwieldy -- that's something we have to live with, I > guess. >
The purpose of this change is to set the groundwork so that an arch could move towards removing the BQL from the cpu_handle_interrupt/exception paths. The BQL is a bottleneck in scaling to more cores. And this cpu_handle_interrupt/exception path is one of the key BQL users as measured by the QEMU sync profiling (qsp). As the first step in removing the BQL from this path, we will make changes to the core/common functions of cpu_handle_interrupt/exception to drop the holding of the BQL. The holding of the BQL is pushed down to the per-arch implementation code. This patch goes through several transitions of the code in order to maintain correctness (bisectability). In order to maintain bisectability across these steps some patches need to touch many files across different arches, however most of the changes are trivial. The general order of the changes is below where each step represents one patch. 1) rename all *_do_interrupt functions to *_do_interrupt_locked 2) add a new function *_do_interrupt that takes the BQL and calls *_do_interrupt_locked, point ->do_interrupt to it, and remove the BQL from cpu-exec.c's cpu_handle_exception. 3) modify the BQL critical sections around ->cpu_exec_interrupt, so that the BQL critical section covers just the call to ->cpu_exec_interrupt. 4/5) same as 1/2 for ->cpu_exec_interrupt. This removes the BQL from cpu_handle_exception. This approach of pushing the BQL down to the per arch functions was suggested by Paolo Bonzini. For reference, here are several key posts in the discussion, explaining the reasoning/benefits of this approach. https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html This patch series is based on the per-CPU locks patch: https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05314.html Our most recent WIP tree is here: https://github.com/rf972/qemu/tree/interrupts_v2.7 Robert Foley (7): target: rename all *_do_interupt functions to _do_interrupt_locked target/arm: add ARMCPUClass->do_interrupt_locked target/cris: add CRISCPUClass->do_interrupt_locked target: Push BQL on ->do_interrupt down into per-arch implementation accel/tcg: Change BQL critical section in cpu_handle_interrupt target: rename all *_cpu_exec_interrupt functions to *_cpu_exec_interrupt_locked target: Push BQL on ->cpu_exec_interrupt down into per-arch implementation accel/tcg/cpu-exec.c | 8 +------- hw/ppc/spapr_events.c | 2 +- target/alpha/helper.c | 22 +++++++++++++++++++--- target/arm/cpu-qom.h | 3 +++ target/arm/cpu.c | 16 +++++++++++++--- target/arm/cpu.h | 2 ++ target/arm/cpu_tcg.c | 19 +++++++++++++++---- target/arm/helper.c | 10 +++++++++- target/arm/m_helper.c | 2 +- target/avr/helper.c | 27 ++++++++++++++++++++++----- target/cris/cpu-qom.h | 3 +++ target/cris/cpu.c | 11 ++++++----- target/cris/cpu.h | 3 ++- target/cris/helper.c | 35 ++++++++++++++++++++++++++--------- target/hppa/int_helper.c | 22 +++++++++++++++++++--- target/i386/seg_helper.c | 20 ++++++++++++++++++-- target/lm32/helper.c | 22 +++++++++++++++++++--- target/m68k/op_helper.c | 22 +++++++++++++++++++--- target/microblaze/helper.c | 24 ++++++++++++++++++++---- target/mips/helper.c | 22 +++++++++++++++++++--- target/moxie/cpu.c | 2 +- target/moxie/cpu.h | 2 +- target/moxie/helper.c | 2 +- target/nios2/cpu.c | 13 +++++++++++-- target/nios2/cpu.h | 1 + target/nios2/helper.c | 13 +++++++++++-- target/openrisc/interrupt.c | 23 ++++++++++++++++++++--- target/ppc/cpu.h | 1 + target/ppc/excp_helper.c | 22 +++++++++++++++++++--- target/ppc/kvm.c | 2 +- target/riscv/cpu_helper.c | 24 +++++++++++++++++++++--- target/rx/cpu.h | 1 + target/rx/helper.c | 22 +++++++++++++++++++--- target/s390x/excp_helper.c | 24 ++++++++++++++++++++---- target/s390x/internal.h | 1 + target/sh4/helper.c | 25 +++++++++++++++++++++---- target/sparc/cpu.c | 13 +++++++++++-- target/sparc/cpu.h | 1 + target/sparc/int32_helper.c | 9 ++++++++- target/sparc/int64_helper.c | 9 ++++++++- target/tilegx/cpu.c | 23 ++++++++++++++++++++--- target/unicore32/cpu.h | 1 + target/unicore32/helper.c | 13 +++++++++++-- target/unicore32/softmmu.c | 9 ++++++++- target/xtensa/exc_helper.c | 25 +++++++++++++++++++++---- 45 files changed, 476 insertions(+), 100 deletions(-)