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(-)

-- 
2.17.1

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.
>